From 0839c02bcb48d41442e772899c56d6e6e2c04031 Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Thu, 15 Jun 2017 15:24:01 -0400 Subject: [PATCH] Fix two IndexOutOfBoundsException crashes. Do not remove (sometimes multiple) items from lists you are iterating over. Test: runtest systemui-notification Change-Id: I130cc63ae2f5721e7b434006f4306e0b1eaef77d Fixes: 62622503 --- .../notification/NotificationManagerService.java | 58 ++++++++++++++-------- .../NotificationManagerServiceTest.java | 37 ++++++++++++++ 2 files changed, 75 insertions(+), 20 deletions(-) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 61d69f95c501..bdea24704ef5 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -307,11 +307,15 @@ public class NotificationManagerService extends SystemService { // used as a mutex for access to all active notifications & listeners final Object mNotificationLock = new Object(); + @GuardedBy("mNotificationLock") final ArrayList mNotificationList = new ArrayList(); + @GuardedBy("mNotificationLock") final ArrayMap mNotificationsByKey = new ArrayMap(); + @GuardedBy("mNotificationLock") final ArrayList mEnqueuedNotifications = new ArrayList<>(); + @GuardedBy("mNotificationLock") final ArrayMap> mAutobundledSummaries = new ArrayMap<>(); final ArrayList mToastQueue = new ArrayList(); final ArrayMap mSummaryByGroupKey = new ArrayMap<>(); @@ -2806,7 +2810,8 @@ public class NotificationManagerService extends SystemService { // Clear summary. final NotificationRecord removed = findNotificationByKeyLocked(summaries.remove(pkg)); if (removed != null) { - cancelNotificationLocked(removed, false, REASON_UNAUTOBUNDLED); + boolean wasPosted = removeFromNotificationListsLocked(removed); + cancelNotificationLocked(removed, false, REASON_UNAUTOBUNDLED, wasPosted); } } } @@ -3420,7 +3425,8 @@ public class NotificationManagerService extends SystemService { .setType(MetricsEvent.TYPE_CLOSE) .addTaggedData(MetricsEvent.NOTIFICATION_SNOOZED_CRITERIA, mSnoozeCriterionId == null ? 0 : 1)); - cancelNotificationLocked(r, false, REASON_SNOOZED); + boolean wasPosted = removeFromNotificationListsLocked(r); + cancelNotificationLocked(r, false, REASON_SNOOZED, wasPosted); updateLightsLocked(); if (mSnoozeCriterionId != null) { mNotificationAssistants.notifyAssistantSnoozedLocked(r.sbn, mSnoozeCriterionId); @@ -4206,15 +4212,18 @@ public class NotificationManagerService extends SystemService { manager.sendAccessibilityEvent(event); } + /** + * Removes all NotificationsRecords with the same key as the given notification record + * from both lists. Do not call this method while iterating over either list. + */ @GuardedBy("mNotificationLock") - private void cancelNotificationLocked(NotificationRecord r, boolean sendDelete, int reason) { - final String canceledKey = r.getKey(); - - // Remove from both lists, either list could have a separate Record for what is effectively - // the same notification. + private boolean removeFromNotificationListsLocked(NotificationRecord r) { + // Remove from both lists, either list could have a separate Record for what is + // effectively the same notification. boolean wasPosted = false; NotificationRecord recordInList = null; - if ((recordInList = findNotificationByListLocked(mNotificationList, r.getKey())) != null) { + if ((recordInList = findNotificationByListLocked(mNotificationList, r.getKey())) + != null) { mNotificationList.remove(recordInList); mNotificationsByKey.remove(recordInList.sbn.getKey()); wasPosted = true; @@ -4223,6 +4232,13 @@ public class NotificationManagerService extends SystemService { != null) { mEnqueuedNotifications.remove(recordInList); } + return wasPosted; + } + + @GuardedBy("mNotificationLock") + private void cancelNotificationLocked(NotificationRecord r, boolean sendDelete, int reason, + boolean wasPosted) { + final String canceledKey = r.getKey(); // Record caller. recordCallerLocked(r); @@ -4363,7 +4379,8 @@ public class NotificationManagerService extends SystemService { } // Cancel the notification. - cancelNotificationLocked(r, sendDelete, reason); + boolean wasPosted = removeFromNotificationListsLocked(r); + cancelNotificationLocked(r, sendDelete, reason, wasPosted); cancelGroupChildrenLocked(r, callingUid, callingPid, listenerName, sendDelete); updateLightsLocked(); @@ -4440,11 +4457,11 @@ public class NotificationManagerService extends SystemService { cancelAllNotificationsByListLocked(mNotificationList, callingUid, callingPid, pkg, true /*nullPkgIndicatesUserSwitch*/, channelId, flagChecker, false /*includeCurrentProfiles*/, userId, false /*sendDelete*/, reason, - listenerName); + listenerName, true /* wasPosted */); cancelAllNotificationsByListLocked(mEnqueuedNotifications, callingUid, callingPid, pkg, true /*nullPkgIndicatesUserSwitch*/, channelId, flagChecker, false /*includeCurrentProfiles*/, userId, - false /*sendDelete*/, reason, listenerName); + false /*sendDelete*/, reason, listenerName, false /* wasPosted */); mSnoozeHelper.cancel(userId, pkg); } } @@ -4460,7 +4477,7 @@ public class NotificationManagerService extends SystemService { private void cancelAllNotificationsByListLocked(ArrayList notificationList, int callingUid, int callingPid, String pkg, boolean nullPkgIndicatesUserSwitch, String channelId, FlagChecker flagChecker, boolean includeCurrentProfiles, int userId, - boolean sendDelete, int reason, String listenerName) { + boolean sendDelete, int reason, String listenerName, boolean wasPosted) { ArrayList canceledNotifications = null; for (int i = notificationList.size() - 1; i >= 0; --i) { NotificationRecord r = notificationList.get(i); @@ -4488,8 +4505,9 @@ public class NotificationManagerService extends SystemService { if (canceledNotifications == null) { canceledNotifications = new ArrayList<>(); } + notificationList.remove(i); canceledNotifications.add(r); - cancelNotificationLocked(r, sendDelete, reason); + cancelNotificationLocked(r, sendDelete, reason, wasPosted); } if (canceledNotifications != null) { final int M = canceledNotifications.size(); @@ -4548,11 +4566,11 @@ public class NotificationManagerService extends SystemService { cancelAllNotificationsByListLocked(mNotificationList, callingUid, callingPid, null, false /*nullPkgIndicatesUserSwitch*/, null, flagChecker, includeCurrentProfiles, userId, true /*sendDelete*/, reason, - listenerName); + listenerName, true); cancelAllNotificationsByListLocked(mEnqueuedNotifications, callingUid, callingPid, null, false /*nullPkgIndicatesUserSwitch*/, null, flagChecker, includeCurrentProfiles, userId, true /*sendDelete*/, - reason, listenerName); + reason, listenerName, false); mSnoozeHelper.cancel(userId, includeCurrentProfiles); } } @@ -4569,7 +4587,6 @@ public class NotificationManagerService extends SystemService { } String pkg = r.sbn.getPackageName(); - int userId = r.getUserId(); if (pkg == null) { if (DBG) Log.e(TAG, "No package for group summary: " + r.getKey()); @@ -4577,15 +4594,15 @@ public class NotificationManagerService extends SystemService { } cancelGroupChildrenByListLocked(mNotificationList, r, callingUid, callingPid, listenerName, - sendDelete); + sendDelete, true); cancelGroupChildrenByListLocked(mEnqueuedNotifications, r, callingUid, callingPid, - listenerName, sendDelete); + listenerName, sendDelete, false); } @GuardedBy("mNotificationLock") private void cancelGroupChildrenByListLocked(ArrayList notificationList, NotificationRecord parentNotification, int callingUid, int callingPid, - String listenerName, boolean sendDelete) { + String listenerName, boolean sendDelete, boolean wasPosted) { final String pkg = parentNotification.sbn.getPackageName(); final int userId = parentNotification.getUserId(); final int reason = REASON_GROUP_SUMMARY_CANCELED; @@ -4597,7 +4614,8 @@ public class NotificationManagerService extends SystemService { && (childR.getFlags() & Notification.FLAG_FOREGROUND_SERVICE) == 0) { EventLogTags.writeNotificationCancel(callingUid, callingPid, pkg, childSbn.getId(), childSbn.getTag(), userId, 0, 0, reason, listenerName); - cancelNotificationLocked(childR, sendDelete, reason); + notificationList.remove(i); + cancelNotificationLocked(childR, sendDelete, reason, wasPosted); } } } diff --git a/services/tests/notification/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/notification/src/com/android/server/notification/NotificationManagerServiceTest.java index bd6e379cb913..46c536c76d46 100644 --- a/services/tests/notification/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/notification/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -357,6 +357,43 @@ public class NotificationManagerServiceTest extends NotificationTestCase { } @Test + public void testCancelAllNotificationsMultipleEnqueuedDoesNotCrash() throws Exception { + final StatusBarNotification sbn = generateNotificationRecord(null).sbn; + for (int i = 0; i < 10; i++) { + mBinderService.enqueueNotificationWithTag(PKG, "opPkg", "tag", + sbn.getId(), sbn.getNotification(), sbn.getUserId()); + } + mBinderService.cancelAllNotifications(PKG, sbn.getUserId()); + waitForIdle(); + } + + @Test + public void testCancelGroupSummaryMultipleEnqueuedChildrenDoesNotCrash() throws Exception { + final NotificationRecord parent = generateNotificationRecord( + mTestNotificationChannel, 1, "group1", true); + final NotificationRecord parentAsChild = generateNotificationRecord( + mTestNotificationChannel, 1, "group1", false); + final NotificationRecord child = generateNotificationRecord( + mTestNotificationChannel, 2, "group1", false); + + // fully post parent notification + mBinderService.enqueueNotificationWithTag(PKG, "opPkg", "tag", + parent.sbn.getId(), parent.sbn.getNotification(), parent.sbn.getUserId()); + waitForIdle(); + + // enqueue the child several times + for (int i = 0; i < 10; i++) { + mBinderService.enqueueNotificationWithTag(PKG, "opPkg", "tag", + child.sbn.getId(), child.sbn.getNotification(), child.sbn.getUserId()); + } + // make the parent a child, which will cancel the child notification + mBinderService.enqueueNotificationWithTag(PKG, "opPkg", "tag", + parentAsChild.sbn.getId(), parentAsChild.sbn.getNotification(), + parentAsChild.sbn.getUserId()); + waitForIdle(); + } + + @Test public void testCancelAllNotifications_IgnoreForegroundService() throws Exception { final StatusBarNotification sbn = generateNotificationRecord(null).sbn; sbn.getNotification().flags |= Notification.FLAG_FOREGROUND_SERVICE; -- 2.11.0