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

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

Change-Id: Ib7554be46dc4cdb6381c0924df504c289a689731

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,14 +653,16 @@ public class NotificationManagerService extends SystemService {
             if (!mEnabled.get(sbn.getNormalizedUserId(), false)) {
                 return;
             }
-            if (mBuffer.size() == mBufferSize) {
+            synchronized (mBufferLock) {
-                mBuffer.removeFirst();
+                if (mBuffer.size() == mBufferSize) {
-            }
+                    mBuffer.removeFirst();
+                }
 
-            // We don't want to store the heavy bits of the notification in the archive,
+                // We don't want to store the heavy bits of the notification in the archive,
-            // but other clients in the system process might be using the object, so we
+                // but other clients in the system process might be using the object, so we
-            // store a (lightened) copy.
+                // store a (lightened) copy.
-            mBuffer.addLast(new Pair<>(sbn.cloneLight(), reason));
+                mBuffer.addLast(new Pair<>(sbn.cloneLight(), reason));
+            }
         }
 
         public Iterator<Pair<StatusBarNotification, Integer>> descendingIterator() {
@@ -666,27 +670,31 @@ public class NotificationManagerService extends SystemService {
         }
 
         public StatusBarNotification[] getArray(int count, boolean includeSnoozed) {
-            if (count == 0) count = mBufferSize;
+            synchronized (mBufferLock) {
-            List<StatusBarNotification> a = new ArrayList();
+                if (count == 0) count = mBufferSize;
-            Iterator<Pair<StatusBarNotification, Integer>> iter = descendingIterator();
+                List<StatusBarNotification> a = new ArrayList();
-            int i=0;
+                Iterator<Pair<StatusBarNotification, Integer>> iter = descendingIterator();
-            while (iter.hasNext() && i < count) {
+                int i = 0;
-                Pair<StatusBarNotification, Integer> pair = iter.next();
+                while (iter.hasNext() && i < count) {
-                if (pair.second != REASON_SNOOZED || includeSnoozed) {
+                    Pair<StatusBarNotification, Integer> pair = iter.next();
-                    i++;
+                    if (pair.second != REASON_SNOOZED || includeSnoozed) {
-                    a.add(pair.first);
+                        i++;
+                        a.add(pair.first);
+                    }
                 }
+                return a.toArray(new StatusBarNotification[a.size()]);
             }
-            return  a.toArray(new StatusBarNotification[a.size()]);
         }
 
         public void updateHistoryEnabled(@UserIdInt int userId, boolean enabled) {
             mEnabled.put(userId, enabled);
 
             if (!enabled) {
-                for (int i = mBuffer.size() - 1; i >= 0; i--) {
+                synchronized (mBufferLock) {
-                    if (userId == mBuffer.get(i).first.getNormalizedUserId()) {
+                    for (int i = mBuffer.size() - 1; i >= 0; i--) {
-                        mBuffer.remove(i);
+                        if (userId == mBuffer.get(i).first.getNormalizedUserId()) {
+                            mBuffer.remove(i);
+                        }
                     }
                 }
             }
@@ -695,15 +703,18 @@ 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) {
-            while (bufferIter.hasNext()) {
+                Iterator<Pair<StatusBarNotification, Integer>> bufferIter = descendingIterator();
-                final Pair<StatusBarNotification, Integer> pair = bufferIter.next();
+                while (bufferIter.hasNext()) {
-                if (pair.first != null
+                    final Pair<StatusBarNotification, Integer> pair = bufferIter.next();
-                        && userId == pair.first.getNormalizedUserId()
+                    if (pair.first != null
-                        && pkg != null && pkg.equals(pair.first.getPackageName())
+                            && userId == pair.first.getNormalizedUserId()
-                        && pair.first.getNotification() != null
+                            && pkg != null && pkg.equals(pair.first.getPackageName())
-                        && Objects.equals(channelId, pair.first.getNotification().getChannelId())) {
+                            && pair.first.getNotification() != null
-                    bufferIter.remove();
+                            && Objects.equals(channelId,
+                            pair.first.getNotification().getChannelId())) {
+                        bufferIter.remove();
+                    }
                 }
             }
         }

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());
+        }
+    }
 }