diff options
| author | 2023-04-11 19:05:58 +0000 | |
|---|---|---|
| committer | 2023-04-13 00:27:16 +0000 | |
| commit | f5932e1b4a55e0275cbee0411f6ca390dab267e7 (patch) | |
| tree | 40b0198e3ed0701022c6b66395b7b7dc83a43494 | |
| parent | 99e08bc3eb40f8e6adcd51b94a2518da32c7875a (diff) | |
Introduce a finer lock for JobNotificationCoordinator.
Introduce a new lock for local objects in JobNotificationCoordinator
which are not dependent on other JobScheduler objects. This will allow
for external services like NotificationManagerService to query the
relevant information in JobNotificationCoordinator without JS having to
hold the global lock and causing a deadlock.
Also update NonNull annotations.
Bug: 277694892
Test: atest JobNotificationCoordinatorTest
Test: atest NotificationTest
Test: atest NotificationManagerTest
Change-Id: I17013deb4c71b70a560c8f27e6186396012362e3
6 files changed, 161 insertions, 46 deletions
diff --git a/apex/jobscheduler/framework/java/com/android/server/job/JobSchedulerInternal.java b/apex/jobscheduler/framework/java/com/android/server/job/JobSchedulerInternal.java index 64b242334a8a..fd8ddbcf3809 100644 --- a/apex/jobscheduler/framework/java/com/android/server/job/JobSchedulerInternal.java +++ b/apex/jobscheduler/framework/java/com/android/server/job/JobSchedulerInternal.java @@ -69,8 +69,8 @@ public interface JobSchedulerInternal { * @return {@code true} if the given notification channel is associated with any user-initiated * jobs. */ - boolean isNotificationChannelAssociatedWithAnyUserInitiatedJobs(String notificationChannel, - int userId, String packageName); + boolean isNotificationChannelAssociatedWithAnyUserInitiatedJobs( + @NonNull String notificationChannel, int userId, @NonNull String packageName); /** * Report a snapshot of sync-related jobs back to the sync manager diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java b/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java index 8bd3d127c21b..b9b825c9f75c 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java @@ -1925,16 +1925,14 @@ class JobConcurrencyManager { return null; } - @GuardedBy("mLock") boolean isNotificationAssociatedWithAnyUserInitiatedJobs(int notificationId, int userId, - String packageName) { + @NonNull String packageName) { return mNotificationCoordinator.isNotificationAssociatedWithAnyUserInitiatedJobs( notificationId, userId, packageName); } - @GuardedBy("mLock") - boolean isNotificationChannelAssociatedWithAnyUserInitiatedJobs(String notificationChannel, - int userId, String packageName) { + boolean isNotificationChannelAssociatedWithAnyUserInitiatedJobs( + @NonNull String notificationChannel, int userId, @NonNull String packageName) { return mNotificationCoordinator.isNotificationChannelAssociatedWithAnyUserInitiatedJobs( notificationChannel, userId, packageName); } diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobNotificationCoordinator.java b/apex/jobscheduler/service/java/com/android/server/job/JobNotificationCoordinator.java index f6e00ec24b33..d94674b5cab0 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobNotificationCoordinator.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobNotificationCoordinator.java @@ -27,9 +27,12 @@ import android.content.pm.UserPackage; import android.os.UserHandle; import android.util.ArrayMap; import android.util.ArraySet; +import android.util.IntArray; import android.util.Slog; +import android.util.SparseArrayMap; import android.util.SparseSetArray; +import com.android.internal.annotations.GuardedBy; import com.android.server.LocalServices; import com.android.server.job.controllers.JobStatus; import com.android.server.notification.NotificationManagerInternal; @@ -38,6 +41,14 @@ class JobNotificationCoordinator { private static final String TAG = "JobNotificationCoordinator"; /** + * Local lock for independent objects like mUijNotifications and mUijNotificationChannels which + * don't depend on other JS objects such as JobServiceContext which require the global JS lock. + * + * Note: do <b>NOT</b> acquire the global lock while this one is held. + */ + private final Object mUijLock = new Object(); + + /** * Mapping of UserPackage -> {notificationId -> List<JobServiceContext>} to track which jobs * are associated with each app's notifications. */ @@ -49,6 +60,27 @@ class JobNotificationCoordinator { private final ArrayMap<JobServiceContext, NotificationDetails> mNotificationDetails = new ArrayMap<>(); + /** + * Mapping of userId -> {packageName, notificationIds} tracking which notifications + * associated with each app belong to user-initiated jobs. + * + * Note: this map can be accessed without holding the main JS lock, so that other services like + * NotificationManagerService can call into JS and verify associations. + */ + @GuardedBy("mUijLock") + private final SparseArrayMap<String, IntArray> mUijNotifications = new SparseArrayMap<>(); + + /** + * Mapping of userId -> {packageName, notificationChannels} tracking which notification channels + * associated with each app are hosting a user-initiated job notification. + * + * Note: this map can be accessed without holding the main JS lock, so that other services like + * NotificationManagerService can call into JS and verify associations. + */ + @GuardedBy("mUijLock") + private final SparseArrayMap<String, ArraySet<String>> mUijNotificationChannels = + new SparseArrayMap<>(); + private static final class NotificationDetails { @NonNull public final UserPackage userPackage; @@ -81,15 +113,24 @@ class JobNotificationCoordinator { int callingPid, int callingUid, int notificationId, @NonNull Notification notification, @JobService.JobEndNotificationPolicy int jobEndNotificationPolicy) { validateNotification(packageName, callingUid, notification, jobEndNotificationPolicy); + final JobStatus jobStatus = hostingContext.getRunningJobLocked(); final NotificationDetails oldDetails = mNotificationDetails.get(hostingContext); if (oldDetails != null && oldDetails.notificationId != notificationId) { // App is switching notification IDs. Remove association with the old one. - removeNotificationAssociation(hostingContext, JobParameters.STOP_REASON_UNDEFINED); + removeNotificationAssociation(hostingContext, JobParameters.STOP_REASON_UNDEFINED, + jobStatus); } final int userId = UserHandle.getUserId(callingUid); - final JobStatus jobStatus = hostingContext.getRunningJobLocked(); if (jobStatus != null && jobStatus.startedAsUserInitiatedJob) { notification.flags |= Notification.FLAG_USER_INITIATED_JOB; + synchronized (mUijLock) { + maybeCreateUijNotificationSetsLocked(userId, packageName); + final IntArray notificationIds = mUijNotifications.get(userId, packageName); + if (notificationIds.indexOf(notificationId) == -1) { + notificationIds.add(notificationId); + } + mUijNotificationChannels.get(userId, packageName).add(notification.getChannelId()); + } } final UserPackage userPackage = UserPackage.of(userId, packageName); final NotificationDetails details = new NotificationDetails( @@ -110,7 +151,7 @@ class JobNotificationCoordinator { } void removeNotificationAssociation(@NonNull JobServiceContext hostingContext, - @JobParameters.StopReason int stopReason) { + @JobParameters.StopReason int stopReason, JobStatus completedJob) { final NotificationDetails details = mNotificationDetails.remove(hostingContext); if (details == null) { return; @@ -121,10 +162,11 @@ class JobNotificationCoordinator { Slog.wtf(TAG, "Association data structures not in sync"); return; } - final String packageName = details.userPackage.packageName; final int userId = UserHandle.getUserId(details.appUid); + final String packageName = details.userPackage.packageName; + final int notificationId = details.notificationId; boolean stripUijFlag = true; - ArraySet<JobServiceContext> associatedContexts = associations.get(details.notificationId); + ArraySet<JobServiceContext> associatedContexts = associations.get(notificationId); if (associatedContexts == null || associatedContexts.isEmpty()) { // No more jobs using this notification. Apply the final job stop policy. // If the user attempted to stop the job/app, then always remove the notification @@ -133,23 +175,50 @@ class JobNotificationCoordinator { || stopReason == JobParameters.STOP_REASON_USER) { mNotificationManagerInternal.cancelNotification( packageName, packageName, details.appUid, details.appPid, /* tag */ null, - details.notificationId, userId); + notificationId, userId); stripUijFlag = false; } } else { // Strip the UIJ flag only if there are no other UIJs associated with the notification - stripUijFlag = !isNotificationAssociatedWithAnyUserInitiatedJobs( - details.notificationId, userId, packageName); + stripUijFlag = !isNotificationUsedForAnyUij(userId, packageName, notificationId); } if (stripUijFlag) { - // Strip the user-initiated job flag from the notification. mNotificationManagerInternal.removeUserInitiatedJobFlagFromNotification( - packageName, details.notificationId, userId); + packageName, notificationId, userId); + } + + // Clean up UIJ related objects if the just completed job was a UIJ + if (completedJob != null && completedJob.startedAsUserInitiatedJob) { + maybeDeleteNotificationIdAssociation(userId, packageName, notificationId); + maybeDeleteNotificationChannelAssociation( + userId, packageName, details.notificationChannel); } } boolean isNotificationAssociatedWithAnyUserInitiatedJobs(int notificationId, - int userId, String packageName) { + int userId, @NonNull String packageName) { + synchronized (mUijLock) { + final IntArray notifications = mUijNotifications.get(userId, packageName); + if (notifications != null) { + return notifications.indexOf(notificationId) != -1; + } + return false; + } + } + + boolean isNotificationChannelAssociatedWithAnyUserInitiatedJobs( + @NonNull String notificationChannel, int userId, @NonNull String packageName) { + synchronized (mUijLock) { + final ArraySet<String> channels = mUijNotificationChannels.get(userId, packageName); + if (channels != null) { + return channels.contains(notificationChannel); + } + return false; + } + } + + private boolean isNotificationUsedForAnyUij(int userId, String packageName, + int notificationId) { final UserPackage pkgDetails = UserPackage.of(userId, packageName); final SparseSetArray<JobServiceContext> associations = mCurrentAssociations.get(pkgDetails); if (associations == null) { @@ -170,8 +239,26 @@ class JobNotificationCoordinator { return false; } - boolean isNotificationChannelAssociatedWithAnyUserInitiatedJobs(String notificationChannel, - int userId, String packageName) { + private void maybeDeleteNotificationIdAssociation(int userId, String packageName, + int notificationId) { + if (isNotificationUsedForAnyUij(userId, packageName, notificationId)) { + return; + } + + // Safe to delete - no UIJs for this package are using this notification id + synchronized (mUijLock) { + final IntArray notifications = mUijNotifications.get(userId, packageName); + if (notifications != null) { + notifications.remove(notifications.indexOf(notificationId)); + if (notifications.size() == 0) { + mUijNotifications.delete(userId, packageName); + } + } + } + } + + private void maybeDeleteNotificationChannelAssociation(int userId, String packageName, + String notificationChannel) { for (int i = mNotificationDetails.size() - 1; i >= 0; i--) { final JobServiceContext jsc = mNotificationDetails.keyAt(i); final NotificationDetails details = mNotificationDetails.get(jsc); @@ -183,11 +270,31 @@ class JobNotificationCoordinator { && details.notificationChannel.equals(notificationChannel)) { final JobStatus jobStatus = jsc.getRunningJobLocked(); if (jobStatus != null && jobStatus.startedAsUserInitiatedJob) { - return true; + return; } } } - return false; + + // Safe to delete - no UIJs for this package are using this notification channel + synchronized (mUijLock) { + ArraySet<String> channels = mUijNotificationChannels.get(userId, packageName); + if (channels != null) { + channels.remove(notificationChannel); + if (channels.isEmpty()) { + mUijNotificationChannels.delete(userId, packageName); + } + } + } + } + + @GuardedBy("mUijLock") + private void maybeCreateUijNotificationSetsLocked(int userId, String packageName) { + if (mUijNotifications.get(userId, packageName) == null) { + mUijNotifications.add(userId, packageName, new IntArray()); + } + if (mUijNotificationChannels.get(userId, packageName) == null) { + mUijNotificationChannels.add(userId, packageName, new ArraySet<>()); + } } private void validateNotification(@NonNull String packageName, int callingUid, diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java index aef9dd058658..2588cd0dbb1c 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java @@ -3713,26 +3713,22 @@ public class JobSchedulerService extends com.android.server.SystemService @Override public boolean isNotificationAssociatedWithAnyUserInitiatedJobs(int notificationId, - int userId, String packageName) { + int userId, @NonNull String packageName) { if (packageName == null) { return false; } - synchronized (mLock) { - return mConcurrencyManager.isNotificationAssociatedWithAnyUserInitiatedJobs( - notificationId, userId, packageName); - } + return mConcurrencyManager.isNotificationAssociatedWithAnyUserInitiatedJobs( + notificationId, userId, packageName); } @Override public boolean isNotificationChannelAssociatedWithAnyUserInitiatedJobs( - String notificationChannel, int userId, String packageName) { + @NonNull String notificationChannel, int userId, @NonNull String packageName) { if (packageName == null || notificationChannel == null) { return false; } - synchronized (mLock) { - return mConcurrencyManager.isNotificationChannelAssociatedWithAnyUserInitiatedJobs( - notificationChannel, userId, packageName); - } + return mConcurrencyManager.isNotificationChannelAssociatedWithAnyUserInitiatedJobs( + notificationChannel, userId, packageName); } @Override diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobServiceContext.java b/apex/jobscheduler/service/java/com/android/server/job/JobServiceContext.java index 44700c86efef..fb36cdec490f 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobServiceContext.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobServiceContext.java @@ -1464,7 +1464,8 @@ public final class JobServiceContext implements ServiceConnection { JobSchedulerEconomicPolicy.ACTION_JOB_TIMEOUT, String.valueOf(mRunningJob.getJobId())); } - mNotificationCoordinator.removeNotificationAssociation(this, reschedulingStopReason); + mNotificationCoordinator.removeNotificationAssociation(this, + reschedulingStopReason, completedJob); if (mWakeLock != null) { mWakeLock.release(); } diff --git a/services/tests/mockingservicestests/src/com/android/server/job/JobNotificationCoordinatorTest.java b/services/tests/mockingservicestests/src/com/android/server/job/JobNotificationCoordinatorTest.java index df2f59a1cefc..e24354f26d8b 100644 --- a/services/tests/mockingservicestests/src/com/android/server/job/JobNotificationCoordinatorTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/job/JobNotificationCoordinatorTest.java @@ -149,7 +149,8 @@ public class JobNotificationCoordinatorTest { .enqueueNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid), eq(pid), any(), eq(notificationId), eq(notification), eq(UserHandle.getUserId(uid))); - coordinator.removeNotificationAssociation(jsc, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc, JobParameters.STOP_REASON_UNDEFINED, + jsc.getRunningJobLocked()); verify(mNotificationManagerInternal, never()) .cancelNotification(anyString(), anyString(), anyInt(), anyInt(), any(), anyInt(), anyInt()); @@ -170,7 +171,8 @@ public class JobNotificationCoordinatorTest { .enqueueNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid), eq(pid), any(), eq(notificationId), eq(notification), eq(UserHandle.getUserId(uid))); - coordinator.removeNotificationAssociation(jsc, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc, JobParameters.STOP_REASON_UNDEFINED, + jsc.getRunningJobLocked()); verify(mNotificationManagerInternal) .cancelNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid), eq(pid), any(), eq(notificationId), eq(UserHandle.getUserId(uid))); @@ -292,7 +294,8 @@ public class JobNotificationCoordinatorTest { eq(notificationId2), eq(notification2), eq(UserHandle.getUserId(uid))); // Remove the first job. Only the first notification should be removed. - coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_UNDEFINED, + jsc1.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal) .cancelNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid), eq(pid), any(), eq(notificationId1), eq(UserHandle.getUserId(uid))); @@ -300,7 +303,8 @@ public class JobNotificationCoordinatorTest { .cancelNotification(anyString(), anyString(), anyInt(), anyInt(), any(), eq(notificationId2), anyInt()); - coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_UNDEFINED, + jsc2.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal) .cancelNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid), eq(pid), any(), eq(notificationId2), eq(UserHandle.getUserId(uid))); @@ -335,12 +339,14 @@ public class JobNotificationCoordinatorTest { eq(notificationId), eq(notification2), eq(UserHandle.getUserId(uid))); // Remove the first job. The notification shouldn't be touched because of the 2nd job. - coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_UNDEFINED, + jsc1.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal, never()) .cancelNotification(anyString(), anyString(), anyInt(), anyInt(), any(), anyInt(), anyInt()); - coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_UNDEFINED, + jsc2.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal) .cancelNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid), eq(pid), any(), eq(notificationId), eq(UserHandle.getUserId(uid))); @@ -376,7 +382,8 @@ public class JobNotificationCoordinatorTest { eq(notificationId), eq(notification2), eq(UserHandle.getUserId(uid2))); // Remove the first job. Only the first notification should be removed. - coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_UNDEFINED, + jsc1.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal) .cancelNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid1), eq(pid), any(), eq(notificationId), eq(UserHandle.getUserId(uid1))); @@ -384,7 +391,8 @@ public class JobNotificationCoordinatorTest { .cancelNotification(anyString(), anyString(), eq(uid2), anyInt(), any(), anyInt(), anyInt()); - coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_UNDEFINED, + jsc2.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal) .cancelNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid2), eq(pid), any(), eq(notificationId), eq(UserHandle.getUserId(uid2))); @@ -421,7 +429,8 @@ public class JobNotificationCoordinatorTest { eq(notificationId), eq(notification2), eq(UserHandle.getUserId(uid))); // Remove the first job. Only the first notification should be removed. - coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_UNDEFINED, + jsc1.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal) .cancelNotification(eq(pkg1), eq(pkg1), eq(uid), eq(pid), any(), eq(notificationId), eq(UserHandle.getUserId(uid))); @@ -429,7 +438,8 @@ public class JobNotificationCoordinatorTest { .cancelNotification(anyString(), anyString(), eq(uid), anyInt(), any(), anyInt(), anyInt()); - coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_UNDEFINED); + coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_UNDEFINED, + jsc2.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal) .cancelNotification(eq(pkg2), eq(pkg2), eq(uid), eq(pid), any(), eq(notificationId), eq(UserHandle.getUserId(uid))); @@ -450,7 +460,8 @@ public class JobNotificationCoordinatorTest { .enqueueNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid), eq(pid), any(), eq(notificationId), eq(notification), eq(UserHandle.getUserId(uid))); - coordinator.removeNotificationAssociation(jsc, JobParameters.STOP_REASON_USER); + coordinator.removeNotificationAssociation(jsc, JobParameters.STOP_REASON_USER, + jsc.getRunningJobLocked()); verify(mNotificationManagerInternal) .cancelNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid), eq(pid), any(), eq(notificationId), eq(UserHandle.getUserId(uid))); @@ -485,12 +496,14 @@ public class JobNotificationCoordinatorTest { eq(notificationId), eq(notification2), eq(UserHandle.getUserId(uid))); // Remove the first job. The notification shouldn't be touched because of the 2nd job. - coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_USER); + coordinator.removeNotificationAssociation(jsc1, JobParameters.STOP_REASON_USER, + jsc1.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal, never()) .cancelNotification(anyString(), anyString(), anyInt(), anyInt(), any(), anyInt(), anyInt()); - coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_USER); + coordinator.removeNotificationAssociation(jsc2, JobParameters.STOP_REASON_USER, + jsc2.getRunningJobLocked()); inOrder.verify(mNotificationManagerInternal) .cancelNotification(eq(TEST_PACKAGE), eq(TEST_PACKAGE), eq(uid), eq(pid), any(), eq(notificationId), eq(UserHandle.getUserId(uid))); |