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

Lock state of NotificationComparator while sorting in RankingHelper

Browse Source 
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
This commit is contained in:
Matías Hernández
2023-08-08 16:33:12 +02:00
parent 701e4404a6
commit 4b230198a4
6 changed files with 116 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
core/java/com/android/internal/util/NotificationMessagingUtil.java
View File

@@ -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<String> mDefaultSmsApp = new SparseArray<>();
private final SparseArray<String> 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(

6
packages/SystemUI/src/com/android/systemui/dagger/AndroidInternalsModule.java
View File

@@ -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} */

21
services/core/java/com/android/server/notification/NotificationComparator.java
View File

@@ -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<NotificationRecord> {
class NotificationComparator implements Comparator<NotificationRecord> {
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);
}
});
}
};
}

7
services/core/java/com/android/server/notification/RankingHelper.java
View File

@@ -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

2
services/core/java/com/android/server/notification/ZenModeFiltering.java
View File

@@ -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) {

76
services/tests/uiservicestests/src/com/android/server/notification/NotificationComparatorTest.java
View File

@@ -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<BroadcastReceiver> broadcastReceiverCaptor = ArgumentCaptor.forClass(
BroadcastReceiver.class);
NotificationComparator comparator = new NotificationComparator(mMockContext);
verify(mMockContext).registerReceiver(broadcastReceiverCaptor.capture(), any());
BroadcastReceiver dialerChangedBroadcastReceiver = broadcastReceiverCaptor.getValue();
ArrayList<NotificationRecord> 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);