Merge "Add a lock around mBuffer in Archive." into sc-dev am: e9e836dffb am: 3f1fb8f233

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/14412686

Change-Id: Id14c564a5c4887e097fc1d1aa79180b24e8a6f00

services/core/java/com/android/server/notification/NotificationManagerService.java

@@ -629,6 +629,8 @@ public class NotificationManagerService extends SystemService {
     static class Archive {
         final SparseArray<Boolean> mEnabled;
         final int mBufferSize;
+        final Object mBufferLock = new Object();
+        @GuardedBy("mBufferLock")
         final LinkedList<Pair<StatusBarNotification, Integer>> mBuffer;
 
         public Archive(int size) {
@@ -651,6 +653,7 @@ public class NotificationManagerService extends SystemService {
             if (!mEnabled.get(sbn.getNormalizedUserId(), false)) {
                 return;
             }
+            synchronized (mBufferLock) {
                 if (mBuffer.size() == mBufferSize) {
                     mBuffer.removeFirst();
                 }
@@ -660,16 +663,18 @@ public class NotificationManagerService extends SystemService {
                 // store a (lightened) copy.
                 mBuffer.addLast(new Pair<>(sbn.cloneLight(), reason));
             }
+        }
 
         public Iterator<Pair<StatusBarNotification, Integer>> descendingIterator() {
             return mBuffer.descendingIterator();
         }
 
         public StatusBarNotification[] getArray(int count, boolean includeSnoozed) {
+            synchronized (mBufferLock) {
                 if (count == 0) count = mBufferSize;
                 List<StatusBarNotification> a = new ArrayList();
                 Iterator<Pair<StatusBarNotification, Integer>> iter = descendingIterator();
-            int i=0;
+                int i = 0;
                 while (iter.hasNext() && i < count) {
                     Pair<StatusBarNotification, Integer> pair = iter.next();
                     if (pair.second != REASON_SNOOZED || includeSnoozed) {
@@ -679,11 +684,13 @@ public class NotificationManagerService extends SystemService {
                 }
                 return a.toArray(new StatusBarNotification[a.size()]);
             }
+        }
 
         public void updateHistoryEnabled(@UserIdInt int userId, boolean enabled) {
             mEnabled.put(userId, enabled);
 
             if (!enabled) {
+                synchronized (mBufferLock) {
                     for (int i = mBuffer.size() - 1; i >= 0; i--) {
                         if (userId == mBuffer.get(i).first.getNormalizedUserId()) {
                             mBuffer.remove(i);
@@ -691,23 +698,27 @@ public class NotificationManagerService extends SystemService {
                     }
                 }
             }
+        }
 
         // Remove notifications with the specified user & channel ID.
         public void removeChannelNotifications(String pkg, @UserIdInt int userId,
                 String channelId) {
-            Iterator<Pair<StatusBarNotification, Integer>> bufferIter = mBuffer.iterator();
+            synchronized (mBufferLock) {
+                Iterator<Pair<StatusBarNotification, Integer>> bufferIter = descendingIterator();
                 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())) {
+                            && Objects.equals(channelId,
+                            pair.first.getNotification().getChannelId())) {
                         bufferIter.remove();
                     }
                 }
             }
         }
+    }
 
     void loadDefaultApprovedServices(int userId) {
         mListeners.loadDefaultsFromConfig();

services/tests/uiservicestests/src/com/android/server/notification/ArchiveTest.java

@@ -21,6 +21,8 @@ import static android.service.notification.NotificationListenerService.REASON_CA
 
 import static com.google.common.truth.Truth.assertThat;
 
+import static org.junit.Assert.fail;
+
 import android.app.Notification;
 import android.os.UserHandle;
 import android.service.notification.StatusBarNotification;
@@ -37,7 +39,11 @@ import org.mockito.MockitoAnnotations;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.ConcurrentModificationException;
 import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 @SmallTest
 @RunWith(AndroidJUnit4.class)
@@ -165,4 +171,54 @@ public class ArchiveTest extends UiServiceTestCase {
             assertThat(expected).contains(sbn.getKey());
         }
     }
+
+    @Test
+    public void testRemoveChannelNotifications_concurrently() throws InterruptedException {
+        List<String> expected = new ArrayList<>();
+        // 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; i++) {
+            StatusBarNotification sbn = getNotification("pkg", i, UserHandle.of(USER_CURRENT));
+            mArchive.record(sbn, REASON_CANCEL);
+            if (i >= SIZE - 2) {
+                // Remove everything < SIZE - 2
+                expected.add(sbn.getKey());
+            }
+        }
+
+        // Remove these in multiple threads to try to get them to happen at the same time
+        int numThreads = SIZE - 2;
+        AtomicBoolean error = new AtomicBoolean(false);
+        CountDownLatch startThreadsLatch = new CountDownLatch(1);
+        CountDownLatch threadsDone = new CountDownLatch(numThreads);
+        for (int i = 0; i < numThreads; i++) {
+            final int idx = i;
+            new Thread(() -> {
+                try {
+                    startThreadsLatch.await(10, TimeUnit.SECONDS);
+                } catch (InterruptedException e) {
+                    throw new RuntimeException(e);
+                }
+                try {
+                    mArchive.removeChannelNotifications("pkg", USER_CURRENT, "test" + idx);
+                } catch (ConcurrentModificationException e) {
+                    error.compareAndSet(false, true);
+                }
+            }).start();
+        }
+
+        startThreadsLatch.countDown();
+        threadsDone.await(10, TimeUnit.SECONDS);
+        if (error.get()) {
+            fail("Concurrent modification exception");
+        }
+
+        List<StatusBarNotification> actual = Arrays.asList(mArchive.getArray(SIZE, true));
+        assertThat(actual).hasSize(expected.size());
+        for (StatusBarNotification sbn : actual) {
+            assertThat(expected).contains(sbn.getKey());
+        }
+    }
 }