diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 08a7d9e38d968..9fd79cefa19c6 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -629,6 +629,8 @@ public class NotificationManagerService extends SystemService { static class Archive { final SparseArray mEnabled; final int mBufferSize; + final Object mBufferLock = new Object(); + @GuardedBy("mBufferLock") final LinkedList> 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) { - mBuffer.removeFirst(); - } + synchronized (mBufferLock) { + if (mBuffer.size() == mBufferSize) { + mBuffer.removeFirst(); + } - // 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 - // store a (lightened) copy. - mBuffer.addLast(new Pair<>(sbn.cloneLight(), reason)); + // 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 + // store a (lightened) copy. + mBuffer.addLast(new Pair<>(sbn.cloneLight(), reason)); + } } public Iterator> descendingIterator() { @@ -666,27 +670,31 @@ public class NotificationManagerService extends SystemService { } public StatusBarNotification[] getArray(int count, boolean includeSnoozed) { - if (count == 0) count = mBufferSize; - List a = new ArrayList(); - Iterator> iter = descendingIterator(); - int i=0; - while (iter.hasNext() && i < count) { - Pair pair = iter.next(); - if (pair.second != REASON_SNOOZED || includeSnoozed) { - i++; - a.add(pair.first); + synchronized (mBufferLock) { + if (count == 0) count = mBufferSize; + List a = new ArrayList(); + Iterator> iter = descendingIterator(); + int i = 0; + while (iter.hasNext() && i < count) { + Pair pair = iter.next(); + if (pair.second != REASON_SNOOZED || includeSnoozed) { + 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--) { - if (userId == mBuffer.get(i).first.getNormalizedUserId()) { - mBuffer.remove(i); + synchronized (mBufferLock) { + for (int i = mBuffer.size() - 1; i >= 0; 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> bufferIter = mBuffer.iterator(); - while (bufferIter.hasNext()) { - final Pair 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())) { - bufferIter.remove(); + synchronized (mBufferLock) { + Iterator> bufferIter = descendingIterator(); + while (bufferIter.hasNext()) { + final Pair 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())) { + 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 a05fea2c8f70b..1126e1ece452a 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/ArchiveTest.java +++ b/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 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 actual = Arrays.asList(mArchive.getArray(SIZE, true)); + assertThat(actual).hasSize(expected.size()); + for (StatusBarNotification sbn : actual) { + assertThat(expected).contains(sbn.getKey()); + } + } }