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

make IStatusBarService:onNotificationClear calls async

Browse Source 
Blocking binder calls to IStatusBarService:onNotificationClear during
important system transitions were causing user-visible jank in traces
collected from real user devices.

Test: atest NotificationEntryManagerTest
Test: atest NotifCollectionTest
Test: atest NotificationEntryManagerInflationTest
Test: atest LaunchConversationActivityTest

Bug: 230668198
Change-Id: I737a5d85049e5e016fd2f4ad37285001752f5f95
This commit is contained in:
András Kurucz
2022-07-19 13:23:44 +00:00
parent 0869abd848
commit a97c612f42
8 changed files with 160 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

62
packages/SystemUI/src/com/android/systemui/people/widget/LaunchConversationActivity.java
View File

@@ -21,6 +21,7 @@ import android.content.Context;
import android.content.Intent;
import android.content.pm.LauncherApps;
import android.os.Bundle;
import android.os.RemoteException;
import android.os.ServiceManager;
import android.os.UserHandle;
import android.os.UserManager;
@@ -34,6 +35,7 @@ import com.android.internal.logging.UiEventLogger;
import com.android.internal.logging.UiEventLoggerImpl;
import com.android.internal.statusbar.IStatusBarService;
import com.android.internal.statusbar.NotificationVisibility;
import com.android.systemui.dagger.qualifiers.Background;
import com.android.systemui.people.PeopleSpaceUtils;
import com.android.systemui.statusbar.CommandQueue;
import com.android.systemui.statusbar.notification.collection.NotificationEntry;
@@ -43,6 +45,7 @@ import com.android.systemui.wmshell.BubblesManager;
import com.android.wm.shell.bubbles.Bubble;
import java.util.Optional;
import java.util.concurrent.Executor;
import javax.inject.Inject;
@@ -58,6 +61,7 @@ public class LaunchConversationActivity extends Activity {
private boolean mIsForTesting;
private IStatusBarService mIStatusBarService;
private CommandQueue mCommandQueue;
private Executor mBgExecutor;
private Bubble mBubble;
private NotificationEntry mEntryToBubble;
@@ -67,7 +71,8 @@ public class LaunchConversationActivity extends Activity {
CommonNotifCollection commonNotifCollection,
Optional<BubblesManager> bubblesManagerOptional,
UserManager userManager,
CommandQueue commandQueue
CommandQueue commandQueue,
@Background Executor bgExecutor
) {
super();
mVisibilityProvider = visibilityProvider;
@@ -91,6 +96,7 @@ public class LaunchConversationActivity extends Activity {
mCommandQueue.removeCallback(this);
}
});
mBgExecutor = bgExecutor;
}
@Override
@@ -172,34 +178,36 @@ public class LaunchConversationActivity extends Activity {
return;
}
try {
if (mIStatusBarService == null || mCommonNotifCollection == null) {
if (DEBUG) {
Log.d(TAG, "Skipping clear notification: null services, key: " + notifKey);
}
return;
if (mIStatusBarService == null || mCommonNotifCollection == null) {
if (DEBUG) {
Log.d(TAG, "Skipping clear notification: null services, key: " + notifKey);
}
NotificationEntry entry = mCommonNotifCollection.getEntry(notifKey);
if (entry == null || entry.getRanking() == null) {
if (DEBUG) {
Log.d(TAG, "Skipping clear notification: NotificationEntry or its Ranking"
+ " is null, key: " + notifKey);
}
return;
}
NotificationVisibility notifVisibility = mVisibilityProvider.obtain(entry, true);
int rank = notifVisibility.rank;
if (DEBUG) Log.d(TAG, "Clearing notification, key: " + notifKey + ", rank: " + rank);
mIStatusBarService.onNotificationClear(
packageName, userHandle.getIdentifier(), notifKey,
NotificationStats.DISMISSAL_OTHER,
NotificationStats.DISMISS_SENTIMENT_POSITIVE, notifVisibility);
} catch (Exception e) {
Log.e(TAG, "Exception cancelling notification:" + e);
return;
}
NotificationEntry entry = mCommonNotifCollection.getEntry(notifKey);
if (entry == null || entry.getRanking() == null) {
if (DEBUG) {
Log.d(TAG, "Skipping clear notification: NotificationEntry or its Ranking"
+ " is null, key: " + notifKey);
}
return;
}
NotificationVisibility notifVisibility = mVisibilityProvider.obtain(entry, true);
int rank = notifVisibility.rank;
if (DEBUG) Log.d(TAG, "Clearing notification, key: " + notifKey + ", rank: " + rank);
mBgExecutor.execute(() -> {
try {
mIStatusBarService.onNotificationClear(
packageName, userHandle.getIdentifier(), notifKey,
NotificationStats.DISMISSAL_OTHER,
NotificationStats.DISMISS_SENTIMENT_POSITIVE, notifVisibility);
} catch (RemoteException e) {
Log.e(TAG, "Exception cancelling notification:" + e);
}
});
}
@VisibleForTesting

31
packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java
View File

@@ -41,6 +41,7 @@ import com.android.internal.annotations.VisibleForTesting;
import com.android.internal.statusbar.IStatusBarService;
import com.android.internal.statusbar.NotificationVisibility;
import com.android.systemui.Dumpable;
import com.android.systemui.dagger.qualifiers.Background;
import com.android.systemui.dump.DumpManager;
import com.android.systemui.statusbar.NotificationLifetimeExtender;
import com.android.systemui.statusbar.NotificationListener;
@@ -73,6 +74,7 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.Executor;
import dagger.Lazy;
@@ -113,6 +115,7 @@ public class NotificationEntryManager implements
private final IStatusBarService mStatusBarService;
private final NotifLiveDataStoreImpl mNotifLiveDataStore;
private final DumpManager mDumpManager;
private final Executor mBgExecutor;
private final Set<NotificationEntry> mAllNotifications = new ArraySet<>();
private final Set<NotificationEntry> mReadOnlyAllNotifications =
@@ -159,7 +162,8 @@ public class NotificationEntryManager implements
LeakDetector leakDetector,
IStatusBarService statusBarService,
NotifLiveDataStoreImpl notifLiveDataStore,
DumpManager dumpManager
DumpManager dumpManager,
@Background Executor bgExecutor
) {
mLogger = logger;
mGroupManager = groupManager;
@@ -170,6 +174,7 @@ public class NotificationEntryManager implements
mStatusBarService = statusBarService;
mNotifLiveDataStore = notifLiveDataStore;
mDumpManager = dumpManager;
mBgExecutor = bgExecutor;
}
/** Once called, the NEM will start processing notification events from system server. */
@@ -566,17 +571,19 @@ public class NotificationEntryManager implements
private void sendNotificationRemovalToServer(
StatusBarNotification notification,
DismissedByUserStats dismissedByUserStats) {
try {
mStatusBarService.onNotificationClear(
notification.getPackageName(),
notification.getUser().getIdentifier(),
notification.getKey(),
dismissedByUserStats.dismissalSurface,
dismissedByUserStats.dismissalSentiment,
dismissedByUserStats.notificationVisibility);
} catch (RemoteException ex) {
// system process is dead if we're here.
}
mBgExecutor.execute(() -> {
try {
mStatusBarService.onNotificationClear(
notification.getPackageName(),
notification.getUser().getIdentifier(),
notification.getKey(),
dismissedByUserStats.dismissalSurface,
dismissedByUserStats.dismissalSentiment,
dismissedByUserStats.notificationVisibility);
} catch (RemoteException ex) {
// system process is dead if we're here.
}
});
}
/**

31
packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java
View File

@@ -70,6 +70,7 @@ import com.android.internal.annotations.VisibleForTesting;
import com.android.internal.statusbar.IStatusBarService;
import com.android.systemui.Dumpable;
import com.android.systemui.dagger.SysUISingleton;
import com.android.systemui.dagger.qualifiers.Background;
import com.android.systemui.dagger.qualifiers.Main;
import com.android.systemui.dump.DumpManager;
import com.android.systemui.dump.LogBufferEulogizer;
@@ -112,6 +113,7 @@ import java.util.Map;
import java.util.Objects;
import java.util.Queue;
import java.util.Set;
import java.util.concurrent.Executor;
import java.util.concurrent.TimeUnit;
import javax.inject.Inject;
@@ -146,6 +148,7 @@ public class NotifCollection implements Dumpable {
private final NotifPipelineFlags mNotifPipelineFlags;
private final NotifCollectionLogger mLogger;
private final Handler mMainHandler;
private final Executor mBgExecutor;
private final LogBufferEulogizer mEulogizer;
private final DumpManager mDumpManager;
@@ -174,6 +177,7 @@ public class NotifCollection implements Dumpable {
NotifPipelineFlags notifPipelineFlags,
NotifCollectionLogger logger,
@Main Handler mainHandler,
@Background Executor bgExecutor,
LogBufferEulogizer logBufferEulogizer,
DumpManager dumpManager) {
mStatusBarService = statusBarService;
@@ -181,6 +185,7 @@ public class NotifCollection implements Dumpable {
mNotifPipelineFlags = notifPipelineFlags;
mLogger = logger;
mMainHandler = mainHandler;
mBgExecutor = bgExecutor;
mEulogizer = logBufferEulogizer;
mDumpManager = dumpManager;
}
@@ -294,18 +299,20 @@ public class NotifCollection implements Dumpable {
entriesToLocallyDismiss.add(entry);
if (!isCanceled(entry)) {
// send message to system server if this notification hasn't already been cancelled
try {
mStatusBarService.onNotificationClear(
entry.getSbn().getPackageName(),
entry.getSbn().getUser().getIdentifier(),
entry.getSbn().getKey(),
stats.dismissalSurface,
stats.dismissalSentiment,
stats.notificationVisibility);
} catch (RemoteException e) {
// system process is dead if we're here.
mLogger.logRemoteExceptionOnNotificationClear(entry, e);
}
mBgExecutor.execute(() -> {
try {
mStatusBarService.onNotificationClear(
entry.getSbn().getPackageName(),
entry.getSbn().getUser().getIdentifier(),
entry.getSbn().getKey(),
stats.dismissalSurface,
stats.dismissalSentiment,
stats.notificationVisibility);
} catch (RemoteException e) {
// system process is dead if we're here.
mLogger.logRemoteExceptionOnNotificationClear(entry, e);
}
});
}
}

6
packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java
View File

@@ -128,7 +128,8 @@ public interface NotificationsModule {
LeakDetector leakDetector,
IStatusBarService statusBarService,
NotifLiveDataStoreImpl notifLiveDataStore,
DumpManager dumpManager) {
DumpManager dumpManager,
@Background Executor bgExecutor) {
return new NotificationEntryManager(
logger,
groupManager,
@@ -138,7 +139,8 @@ public interface NotificationsModule {
leakDetector,
statusBarService,
notifLiveDataStore,
dumpManager);
dumpManager,
bgExecutor);
}
/** Provides an instance of {@link NotificationGutsManager} */

9
packages/SystemUI/tests/src/com/android/systemui/people/widget/LaunchConversationActivityTest.java
View File

@@ -48,6 +48,8 @@ import com.android.systemui.statusbar.CommandQueue;
import com.android.systemui.statusbar.notification.collection.NotificationEntry;
import com.android.systemui.statusbar.notification.collection.notifcollection.CommonNotifCollection;
import com.android.systemui.statusbar.notification.collection.render.NotificationVisibilityProvider;
import com.android.systemui.util.concurrency.FakeExecutor;
import com.android.systemui.util.time.FakeSystemClock;
import com.android.systemui.wmshell.BubblesManager;
import com.android.wm.shell.bubbles.Bubble;
@@ -106,6 +108,9 @@ public class LaunchConversationActivityTest extends SysuiTestCase {
private Intent mIntent;
private FakeSystemClock mFakeSystemClock = new FakeSystemClock();
private FakeExecutor mBgExecutor = new FakeExecutor(mFakeSystemClock);
@Before
public void setUp() throws Exception {
MockitoAnnotations.initMocks(this);
@@ -114,7 +119,8 @@ public class LaunchConversationActivityTest extends SysuiTestCase {
mNotifCollection,
Optional.of(mBubblesManager),
mUserManager,
mCommandQueue
mCommandQueue,
mBgExecutor
);
verify(mCommandQueue, times(1)).addCallback(mCallbacksCaptor.capture());
@@ -193,6 +199,7 @@ public class LaunchConversationActivityTest extends SysuiTestCase {
// Ensure callback removed
verify(mCommandQueue).removeCallback(any());
// Clear the notification for bubbles.
FakeExecutor.exhaustExecutors(mBgExecutor);
verify(mIStatusBarService, times(1)).onNotificationClear(any(),
anyInt(), any(), anyInt(), anyInt(), mNotificationVisibilityCaptor.capture());
// Do not select the bubble.

83
packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java
View File

@@ -19,6 +19,8 @@ package com.android.systemui.statusbar.notification;
import static android.app.NotificationManager.IMPORTANCE_DEFAULT;
import static android.service.notification.NotificationListenerService.NOTIFICATION_CHANNEL_OR_GROUP_UPDATED;
import static android.service.notification.NotificationListenerService.REASON_CANCEL;
import static android.service.notification.NotificationStats.DISMISSAL_SHADE;
import static android.service.notification.NotificationStats.DISMISS_SENTIMENT_NEUTRAL;
import static com.android.systemui.statusbar.notification.NotificationEntryManager.UNDEFINED_DISMISS_REASON;
@@ -41,6 +43,7 @@ import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
import android.app.ActivityManager;
@@ -50,6 +53,7 @@ import android.app.PendingIntent;
import android.content.Intent;
import android.graphics.drawable.Icon;
import android.os.Handler;
import android.os.RemoteException;
import android.os.UserHandle;
import android.service.notification.NotificationListenerService.Ranking;
import android.service.notification.NotificationListenerService.RankingMap;
@@ -91,7 +95,9 @@ import com.android.systemui.statusbar.notification.row.NotificationEntryManagerI
import com.android.systemui.statusbar.notification.row.RowInflaterTask;
import com.android.systemui.statusbar.policy.DeviceProvisionedController;
import com.android.systemui.statusbar.policy.HeadsUpManager;
import com.android.systemui.util.concurrency.FakeExecutor;
import com.android.systemui.util.leak.LeakDetector;
import com.android.systemui.util.time.FakeSystemClock;
import org.junit.Before;
import org.junit.Test;
@@ -138,9 +144,14 @@ public class NotificationEntryManagerTest extends SysuiTestCase {
@Mock private NotificationMediaManager mNotificationMediaManager;
@Mock private NotificationRowBinder mNotificationRowBinder;
@Mock private NotificationListener mNotificationListener;
@Mock private IStatusBarService mStatusBarService;
private FakeSystemClock mFakeSystemClock = new FakeSystemClock();
private FakeExecutor mBgExecutor = new FakeExecutor(mFakeSystemClock);
private int mId;
private NotificationEntry mEntry;
private DismissedByUserStats mStats;
private StatusBarNotification mSbn;
private NotificationEntryManager mEntryManager;
@@ -191,6 +202,7 @@ public class NotificationEntryManagerTest extends SysuiTestCase {
Handler.createAsync(TestableLooper.get(this).getLooper()));
mEntry = createNotification();
mStats = defaultStats(mEntry);
mSbn = mEntry.getSbn();
when(mNotifPipelineFlags.isNewPipelineEnabled()).thenReturn(false);
@@ -201,9 +213,10 @@ public class NotificationEntryManagerTest extends SysuiTestCase {
() -> mNotificationRowBinder,
() -> mRemoteInputManager,
mLeakDetector,
mock(IStatusBarService.class),
mStatusBarService,
NotifLiveDataStoreMocksKt.createNotifLiveDataStoreImplMock(),
mock(DumpManager.class)
mock(DumpManager.class),
mBgExecutor
);
mEntryManager.initialize(
mNotificationListener,
@@ -315,6 +328,31 @@ public class NotificationEntryManagerTest extends SysuiTestCase {
assertFalse(entriesContainKey(mEntryManager.getAllNotifs(), mSbn.getKey()));
}
@Test
public void testPerformRemoveNotification_sendRemovalToServer() throws RemoteException {
// GIVEN an entry manager with a notification
mEntryManager.addActiveNotificationForTest(mEntry);
// GIVEN interceptor that doesn't intercept
when(mRemoveInterceptor.onNotificationRemoveRequested(
eq(mEntry.getKey()), argThat(matchEntryOnKey()), anyInt()))
.thenReturn(false);
// WHEN the notification entry is removed
mEntryManager.performRemoveNotification(mSbn, mStats, REASON_CANCEL);
// THEN notification removal is sent to the server
FakeExecutor.exhaustExecutors(mBgExecutor);
verify(mStatusBarService).onNotificationClear(
mSbn.getPackageName(),
mSbn.getUser().getIdentifier(),
mSbn.getKey(),
mStats.dismissalSurface,
mStats.dismissalSentiment,
mStats.notificationVisibility);
verifyNoMoreInteractions(mStatusBarService);
}
@Test
public void testRemoveNotification_onEntryRemoveNotFiredIfEntryDoesntExist() {
@@ -573,23 +611,6 @@ public class NotificationEntryManagerTest extends SysuiTestCase {
any(NotificationVisibility.class), anyBoolean(), eq(UNDEFINED_DISMISS_REASON));
}
private NotificationEntry createNotification() {
Notification.Builder n = new Notification.Builder(mContext, "id")
.setSmallIcon(R.drawable.ic_person)
.setContentTitle("Title")
.setContentText("Text");
return new NotificationEntryBuilder()
.setPkg(TEST_PACKAGE_NAME)
.setOpPkg(TEST_PACKAGE_NAME)
.setUid(TEST_UID)
.setId(mId++)
.setNotification(n.build())
.setChannel(new NotificationChannel("id", "", IMPORTANCE_DEFAULT))
.setUser(new UserHandle(ActivityManager.getCurrentUser()))
.build();
}
/* Tests annexed from NotificationDataTest go here */
@Test
@@ -713,4 +734,28 @@ public class NotificationEntryManagerTest extends SysuiTestCase {
return mManagedNotifs.contains(notificationKey);
}
}
private NotificationEntry createNotification() {
Notification.Builder n = new Notification.Builder(mContext, "id")
.setSmallIcon(R.drawable.ic_person)
.setContentTitle("Title")
.setContentText("Text");
return new NotificationEntryBuilder()
.setPkg(TEST_PACKAGE_NAME)
.setOpPkg(TEST_PACKAGE_NAME)
.setUid(TEST_UID)
.setId(mId++)
.setNotification(n.build())
.setChannel(new NotificationChannel("id", "", IMPORTANCE_DEFAULT))
.setUser(new UserHandle(ActivityManager.getCurrentUser()))
.build();
}
private static DismissedByUserStats defaultStats(NotificationEntry entry) {
return new DismissedByUserStats(
DISMISSAL_SHADE,
DISMISS_SENTIMENT_NEUTRAL,
NotificationVisibility.obtain(entry.getKey(), 7, 2, true));
}
}

9
packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java
View File

@@ -92,6 +92,7 @@ import com.android.systemui.statusbar.notification.collection.notifcollection.No
import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLogger;
import com.android.systemui.statusbar.notification.collection.notifcollection.NotifDismissInterceptor;
import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender;
import com.android.systemui.util.concurrency.FakeExecutor;
import com.android.systemui.util.time.FakeSystemClock;
import org.junit.Before;
@@ -146,6 +147,7 @@ public class NotifCollectionTest extends SysuiTestCase {
private NoManSimulator mNoMan;
private FakeSystemClock mClock = new FakeSystemClock();
private FakeExecutor mBgExecutor = new FakeExecutor(mClock);
@Before
public void setUp() {
@@ -162,6 +164,7 @@ public class NotifCollectionTest extends SysuiTestCase {
mNotifPipelineFlags,
mLogger,
mMainHandler,
mBgExecutor,
mEulogizer,
mock(DumpManager.class));
mCollection.attach(mGroupCoalescer);
@@ -461,6 +464,8 @@ public class NotifCollectionTest extends SysuiTestCase {
DismissedByUserStats stats = defaultStats(entry2);
mCollection.dismissNotification(entry2, defaultStats(entry2));
FakeExecutor.exhaustExecutors(mBgExecutor);
// THEN we send the dismissal to system server
verify(mStatusBarService).onNotificationClear(
notif2.sbn.getPackageName(),
@@ -674,6 +679,8 @@ public class NotifCollectionTest extends SysuiTestCase {
mInterceptor1.onEndInterceptionCallback.onEndDismissInterception(mInterceptor1, entry,
stats);
FakeExecutor.exhaustExecutors(mBgExecutor);
// THEN we send the dismissal to system server
verify(mStatusBarService).onNotificationClear(
eq(notif.sbn.getPackageName()),
@@ -1211,6 +1218,7 @@ public class NotifCollectionTest extends SysuiTestCase {
new Pair<>(entry2, defaultStats(entry2))));
// THEN we send the dismissals to system server
FakeExecutor.exhaustExecutors(mBgExecutor);
verify(mStatusBarService).onNotificationClear(
notif1.sbn.getPackageName(),
notif1.sbn.getUser().getIdentifier(),
@@ -1577,6 +1585,7 @@ public class NotifCollectionTest extends SysuiTestCase {
// WHEN finally dismissing
onDismiss.run();
FakeExecutor.exhaustExecutors(mBgExecutor);
verify(mStatusBarService).onNotificationClear(any(), anyInt(), eq(notifEvent.key),
anyInt(), anyInt(), any());
verifyNoMoreInteractions(mStatusBarService);

3
packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationEntryManagerInflationTest.java
View File

@@ -194,7 +194,8 @@ public class NotificationEntryManagerInflationTest extends SysuiTestCase {
mLeakDetector,
mock(IStatusBarService.class),
NotifLiveDataStoreMocksKt.createNotifLiveDataStoreImplMock(),
mock(DumpManager.class)
mock(DumpManager.class),
mBgExecutor
);
mEntryManager.initialize(
mNotificationListener,