Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add a lock around mBuffer in Archive.

Browse Source 
Channel removal was resulting in some concurrent modification exceptions, so this change both adds a lock to synchronize any read/write access to mBuffer and also adds a test to make sure attempted concurrent access works correctly.

Test: atest ArchiveTest before and after change
Fixes: 186860793
Change-Id: Ic5f15a0fd3de27a44cc755c5d2f8008887c6b5b9
This commit is contained in:
Yuri Lin
2021-05-03 14:36:15 -04:00
parent 94592d369e
commit 4443582702
2 changed files with 96 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
services/core/java/com/android/server/notification/NotificationManagerService.java
View File

@@ -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) {
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<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;
List<StatusBarNotification> a = new ArrayList();
Iterator<Pair<StatusBarNotification, Integer>> iter = descendingIterator();
int i=0;
while (iter.hasNext() && i < count) {
Pair<StatusBarNotification, Integer> pair = iter.next();
if (pair.second != REASON_SNOOZED || includeSnoozed) {
i++;
a.add(pair.first);
synchronized (mBufferLock) {
if (count == 0) count = mBufferSize;
List<StatusBarNotification> a = new ArrayList();
Iterator<Pair<StatusBarNotification, Integer>> iter = descendingIterator();
int i = 0;
while (iter.hasNext() && i < count) {
Pair<StatusBarNotification, Integer> 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<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())) {
bufferIter.remove();
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())) {
bufferIter.remove();
}
}
}
}

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

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