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);