From acbca16cfaf4248320ab8c5e1e7189bf314eaa29 Mon Sep 17 00:00:00 2001
From: Yuri Lin <yurilin@google.com>
Date: Tue, 27 Apr 2021 11:13:49 -0400
Subject: [PATCH] Use an iterator in removeChannelNotifications rather than
 index.

The index-based implementation is buggy because if an element is removed from the list, the current index will be incorrect, thus both skipping over an element and potentially running off the end of the list.

This change also modifies ArchiveTest to test these edge cases.

Test: atest ArchiveTest (before & after change)
Bug: 186473453

Change-Id: I1611aa6d3630beef60f916d59a76c7976f1648d2
---
 .../notification/NotificationManagerService.java   |  7 ++++---
 .../android/server/notification/ArchiveTest.java   | 14 ++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java
index a71722c93d48a..215c3933fce3f 100755
--- a/services/core/java/com/android/server/notification/NotificationManagerService.java
+++ b/services/core/java/com/android/server/notification/NotificationManagerService.java
@@ -695,14 +695,15 @@ public class NotificationManagerService extends SystemService {
         // Remove notifications with the specified user & channel ID.
         public void removeChannelNotifications(String pkg, @UserIdInt int userId,
                 String channelId) {
-            for (int i = 0; i < mBuffer.size(); i++) {
-                final Pair<StatusBarNotification, Integer> pair = mBuffer.get(i);
+            Iterator<Pair<StatusBarNotification, Integer>> bufferIter = mBuffer.iterator();
+            while (bufferIter.hasNext()) {
+                final Pair<StatusBarNotification, Integer> pair = bufferIter.next();
                 if (pair.first != null
                         && userId == pair.first.getNormalizedUserId()
                         && pkg != null && pkg.equals(pair.first.getPackageName())
                         && pair.first.getNotification() != null
                         && Objects.equals(channelId, pair.first.getNotification().getChannelId())) {
-                    mBuffer.remove(i);
+                    bufferIter.remove();
                 }
             }
         }
diff --git a/services/tests/uiservicestests/src/com/android/server/notification/ArchiveTest.java b/services/tests/uiservicestests/src/com/android/server/notification/ArchiveTest.java
index a2ad89edcf5ea..a05fea2c8f70b 100644
--- a/services/tests/uiservicestests/src/com/android/server/notification/ArchiveTest.java
+++ b/services/tests/uiservicestests/src/com/android/server/notification/ArchiveTest.java
@@ -144,15 +144,21 @@ public class ArchiveTest extends UiServiceTestCase {
     @Test
     public void testRemoveChannelNotifications() {
         List<String> expected = new ArrayList<>();
-        for (int i = 0; i < SIZE; i++) {
+        // Add one extra notification to the beginning to test when 2 adjacent notifications will be
+        // removed in the same pass.
+        StatusBarNotification sbn0 = getNotification("pkg", 0, UserHandle.of(USER_CURRENT));
+        mArchive.record(sbn0, REASON_CANCEL);
+        for (int i = 0; i < SIZE - 1; i++) {
             StatusBarNotification sbn = getNotification("pkg", i, UserHandle.of(USER_CURRENT));
             mArchive.record(sbn, REASON_CANCEL);
-            if (i != 3) {
-                // Will delete notification for this user in channel "test3".
+            if (i != 0 && i != SIZE - 2) {
+                // Will delete notification for this user in channel "test0", and also the last
+                // element in the list.
                 expected.add(sbn.getKey());
             }
         }
-        mArchive.removeChannelNotifications("pkg", USER_CURRENT, "test3");
+        mArchive.removeChannelNotifications("pkg", USER_CURRENT, "test0");
+        mArchive.removeChannelNotifications("pkg", USER_CURRENT, "test" + (SIZE - 2));
         List<StatusBarNotification> actual = Arrays.asList(mArchive.getArray(SIZE, true));
         assertThat(actual).hasSize(expected.size());
         for (StatusBarNotification sbn : actual) {