Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit acbca16c authored by Yuri Lin's avatar Yuri Lin
Browse files

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
parent f10473c6

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

+4 −3
Original line number Diff line number Diff line
@@ -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();

                }

            }

        }

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

+10 −4
Original line number Diff line number Diff line
@@ -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) {