From 4b230198a46f82929bd59b5c97cda07a58e81d11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20Hern=C3=A1ndez?= Date: Tue, 8 Aug 2023 16:33:12 +0200 Subject: [PATCH] Lock state of NotificationComparator while sorting in RankingHelper When the default dialer or default SMS package is changed, the NotificationComparator and NotificationMessagingUtil update their cached copy of this value, which is used to evaluate isImportantOngoing and isImportantMessaging (two of the criteria involved in the comparison). However, if this is changed _while a sort() is ongoing_, then the results of element comparison could potentially be inconsistent whenever notifications posted by the new or previous package are involved. In the worst case this could result in a system crash ("IllegalArgumentException: Comparison method violates its general contract!"). This CL introduces a lock object in NotificationComparator that should be acquired and held for the full duration of the sort. Default package changes that arrive within this time will be processed afterwards. Fixes: 293249306 Test: atest NotificationComparatorTest -- testChangeDialerPackageWhileSorting() crashes without the synchronized block around records.sort(comparator). Change-Id: I67251437ae827c39b135f5d45cfe682aa852d09a --- .../util/NotificationMessagingUtil.java | 23 ++++-- .../dagger/AndroidInternalsModule.java | 6 +- .../notification/NotificationComparator.java | 21 +++-- .../server/notification/RankingHelper.java | 7 +- .../server/notification/ZenModeFiltering.java | 2 +- .../NotificationComparatorTest.java | 76 +++++++++++++++++++ 6 files changed, 116 insertions(+), 19 deletions(-) diff --git a/core/java/com/android/internal/util/NotificationMessagingUtil.java b/core/java/com/android/internal/util/NotificationMessagingUtil.java index d3cc0e7c767c2..10856b37ef8d7 100644 --- a/core/java/com/android/internal/util/NotificationMessagingUtil.java +++ b/core/java/com/android/internal/util/NotificationMessagingUtil.java @@ -16,6 +16,7 @@ package com.android.internal.util; +import android.annotation.Nullable; import android.app.Notification; import android.app.NotificationManager; import android.content.Context; @@ -39,10 +40,12 @@ public class NotificationMessagingUtil { private static final String DEFAULT_SMS_APP_SETTING = Settings.Secure.SMS_DEFAULT_APPLICATION; private final Context mContext; - private SparseArray mDefaultSmsApp = new SparseArray<>(); + private final SparseArray mDefaultSmsApp = new SparseArray<>(); + private final Object mStateLock; - public NotificationMessagingUtil(Context context) { + public NotificationMessagingUtil(Context context, @Nullable Object stateLock) { mContext = context; + mStateLock = stateLock != null ? stateLock : new Object(); mContext.getContentResolver().registerContentObserver( Settings.Secure.getUriFor(DEFAULT_SMS_APP_SETTING), false, mSmsContentObserver); } @@ -63,16 +66,20 @@ public class NotificationMessagingUtil { private boolean isDefaultMessagingApp(StatusBarNotification sbn) { final int userId = sbn.getUserId(); if (userId == UserHandle.USER_NULL || userId == UserHandle.USER_ALL) return false; - if (mDefaultSmsApp.get(userId) == null) { - cacheDefaultSmsApp(userId); + synchronized (mStateLock) { + if (mDefaultSmsApp.get(userId) == null) { + cacheDefaultSmsApp(userId); + } + return Objects.equals(mDefaultSmsApp.get(userId), sbn.getPackageName()); } - return Objects.equals(mDefaultSmsApp.get(userId), sbn.getPackageName()); } private void cacheDefaultSmsApp(int userId) { - mDefaultSmsApp.put(userId, Settings.Secure.getStringForUser( - mContext.getContentResolver(), - Settings.Secure.SMS_DEFAULT_APPLICATION, userId)); + String smsApp = Settings.Secure.getStringForUser(mContext.getContentResolver(), + Settings.Secure.SMS_DEFAULT_APPLICATION, userId); + synchronized (mStateLock) { + mDefaultSmsApp.put(userId, smsApp); + } } private final ContentObserver mSmsContentObserver = new ContentObserver( diff --git a/packages/SystemUI/src/com/android/systemui/dagger/AndroidInternalsModule.java b/packages/SystemUI/src/com/android/systemui/dagger/AndroidInternalsModule.java index 09928829b1957..585c390f83665 100644 --- a/packages/SystemUI/src/com/android/systemui/dagger/AndroidInternalsModule.java +++ b/packages/SystemUI/src/com/android/systemui/dagger/AndroidInternalsModule.java @@ -24,11 +24,11 @@ import com.android.internal.logging.UiEventLoggerImpl; import com.android.internal.util.NotificationMessagingUtil; import com.android.internal.widget.LockPatternUtils; -import javax.inject.Singleton; - import dagger.Module; import dagger.Provides; +import javax.inject.Singleton; + /** * Provides items imported from com.android.internal. */ @@ -51,7 +51,7 @@ public class AndroidInternalsModule { /** */ @Provides public NotificationMessagingUtil provideNotificationMessagingUtil(Context context) { - return new NotificationMessagingUtil(context); + return new NotificationMessagingUtil(context, null); } /** Provides an instance of {@link com.android.internal.logging.UiEventLogger} */ diff --git a/services/core/java/com/android/server/notification/NotificationComparator.java b/services/core/java/com/android/server/notification/NotificationComparator.java index 8992878860092..0e76dfb890fe6 100644 --- a/services/core/java/com/android/server/notification/NotificationComparator.java +++ b/services/core/java/com/android/server/notification/NotificationComparator.java @@ -25,6 +25,7 @@ import android.content.Intent; import android.content.IntentFilter; import android.telecom.TelecomManager; +import com.android.internal.os.BackgroundThread; import com.android.internal.util.NotificationMessagingUtil; import java.util.Comparator; @@ -33,18 +34,23 @@ import java.util.Objects; /** * Sorts notifications individually into attention-relevant order. */ -public class NotificationComparator - implements Comparator { +class NotificationComparator implements Comparator { private final Context mContext; private final NotificationMessagingUtil mMessagingUtil; private String mDefaultPhoneApp; + /** + * Lock that must be held during a sort() call that uses this {@link Comparator}, AND to make + * any changes to the state of this object that could affect the results of {@link #compare}. + */ + public final Object mStateLock = new Object(); + public NotificationComparator(Context context) { mContext = context; mContext.registerReceiver(mPhoneAppBroadcastReceiver, new IntentFilter(TelecomManager.ACTION_DEFAULT_DIALER_CHANGED)); - mMessagingUtil = new NotificationMessagingUtil(mContext); + mMessagingUtil = new NotificationMessagingUtil(mContext, mStateLock); } @Override @@ -212,8 +218,13 @@ public class NotificationComparator private final BroadcastReceiver mPhoneAppBroadcastReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { - mDefaultPhoneApp = - intent.getStringExtra(TelecomManager.EXTRA_CHANGE_DEFAULT_DIALER_PACKAGE_NAME); + BackgroundThread.getExecutor().execute(() -> { + synchronized (mStateLock) { + mDefaultPhoneApp = + intent.getStringExtra( + TelecomManager.EXTRA_CHANGE_DEFAULT_DIALER_PACKAGE_NAME); + } + }); } }; } diff --git a/services/core/java/com/android/server/notification/RankingHelper.java b/services/core/java/com/android/server/notification/RankingHelper.java index b4347e148ce01..773d10b1f0763 100644 --- a/services/core/java/com/android/server/notification/RankingHelper.java +++ b/services/core/java/com/android/server/notification/RankingHelper.java @@ -103,8 +103,11 @@ public class RankingHelper { notificationList.get(i).setGlobalSortKey(null); } - // rank each record individually - Collections.sort(notificationList, mPreliminaryComparator); + // Rank each record individually. + // Lock comparator state for consistent compare() results. + synchronized (mPreliminaryComparator.mStateLock) { + notificationList.sort(mPreliminaryComparator); + } synchronized (mProxyByGroupTmp) { // record individual ranking result and nominate proxies for each group diff --git a/services/core/java/com/android/server/notification/ZenModeFiltering.java b/services/core/java/com/android/server/notification/ZenModeFiltering.java index 5b7b0c12026b1..f56a67c00c15f 100644 --- a/services/core/java/com/android/server/notification/ZenModeFiltering.java +++ b/services/core/java/com/android/server/notification/ZenModeFiltering.java @@ -55,7 +55,7 @@ public class ZenModeFiltering { public ZenModeFiltering(Context context) { mContext = context; - mMessagingUtil = new NotificationMessagingUtil(mContext); + mMessagingUtil = new NotificationMessagingUtil(mContext, null); } public ZenModeFiltering(Context context, NotificationMessagingUtil messagingUtil) { diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationComparatorTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationComparatorTest.java index 22b112746b50d..7b16500928a0f 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationComparatorTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationComparatorTest.java @@ -15,15 +15,20 @@ */ package com.android.server.notification; +import static android.telecom.TelecomManager.EXTRA_CHANGE_DEFAULT_DIALER_PACKAGE_NAME; + import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import android.app.Notification; @@ -31,8 +36,10 @@ import android.app.NotificationChannel; import android.app.NotificationManager; import android.app.PendingIntent; import android.app.Person; +import android.content.BroadcastReceiver; import android.content.ContentResolver; import android.content.Context; +import android.content.Intent; import android.content.pm.ApplicationInfo; import android.content.pm.PackageManager; import android.content.res.Resources; @@ -54,12 +61,14 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.concurrent.CountDownLatch; @SmallTest @RunWith(AndroidJUnit4.class) @@ -335,6 +344,73 @@ public class NotificationComparatorTest extends UiServiceTestCase { assertTrue(comp.isImportantPeople(mRecordContact)); } + @Test + public void testChangeDialerPackageWhileSorting() throws InterruptedException { + final int halfList = 100; + int userId = UserHandle.myUserId(); + when(mTm.getDefaultDialerPackage()).thenReturn("B"); + + ArgumentCaptor broadcastReceiverCaptor = ArgumentCaptor.forClass( + BroadcastReceiver.class); + NotificationComparator comparator = new NotificationComparator(mMockContext); + verify(mMockContext).registerReceiver(broadcastReceiverCaptor.capture(), any()); + BroadcastReceiver dialerChangedBroadcastReceiver = broadcastReceiverCaptor.getValue(); + + ArrayList records = new ArrayList<>(); + for (int i = 0; i < halfList; i++) { + Notification notifCallFromPkgA = new Notification.Builder(mMockContext, TEST_CHANNEL_ID) + .setCategory(Notification.CATEGORY_CALL) + .setFlag(Notification.FLAG_FOREGROUND_SERVICE, true) + .build(); + records.add(new NotificationRecord(mMockContext, + new StatusBarNotification("A", "A", 2 * i, "callA", callUid, callUid, + notifCallFromPkgA, new UserHandle(userId), "", 0), + getDefaultChannel())); + + Notification notifCallFromPkgB = new Notification.Builder(mMockContext, TEST_CHANNEL_ID) + .setCategory(Notification.CATEGORY_CALL) + .setFlag(Notification.FLAG_FOREGROUND_SERVICE, true) + .build(); + records.add(new NotificationRecord(mMockContext, + new StatusBarNotification("B", "B", 2 * i + 1, "callB", callUid, callUid, + notifCallFromPkgB, new UserHandle(userId), "", 0), + getDefaultChannel())); + } + + CountDownLatch allDone = new CountDownLatch(2); + new Thread(() -> { + // The lock prevents the other thread from changing the dialer package mid-sort, so: + // 1) Results should be "all B before all A" (asserted below). + // 2) No "IllegalArgumentException: Comparison method violates its general contract!" + synchronized (comparator.mStateLock) { + records.sort(comparator); + allDone.countDown(); + } + }).start(); + + new Thread(() -> { + String nextDialer = "A"; + while (allDone.getCount() == 2) { + Intent dialerChangedIntent = new Intent(); + dialerChangedIntent.putExtra(EXTRA_CHANGE_DEFAULT_DIALER_PACKAGE_NAME, nextDialer); + dialerChangedBroadcastReceiver.onReceive(mMockContext, dialerChangedIntent); + nextDialer = nextDialer.equals("A") ? "B" : "A"; + } + allDone.countDown(); + }).start(); + + allDone.await(); + + for (int i = 0; i < halfList; i++) { + assertWithMessage("Wrong element in position #" + i) + .that(records.get(i).getSbn().getPackageName()).isEqualTo("B"); + } + for (int i = halfList; i < 2 * halfList; i++) { + assertWithMessage("Wrong element in position #" + i) + .that(records.get(i).getSbn().getPackageName()).isEqualTo("A"); + } + } + private NotificationChannel getDefaultChannel() { return new NotificationChannel(NotificationChannel.DEFAULT_CHANNEL_ID, "name", NotificationManager.IMPORTANCE_LOW);