From 4a206a416582e9242060176b4e3f8103e6b43473 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Wed, 11 May 2022 19:54:53 +0000 Subject: [PATCH] Fix NotificationEntry mismatch crashes when tapping notification. The root of these problems is holding onto a NotificationEntry instance and performing a dismiss or end to lifetime extension after a delay, during which apps or the system may cancel and re-post the given notification. The original fix to this by lifetime extending these notifications worked in the case where this closure around a NotificationEntry perfectly matched lifetime extension, but this was becoming a whack-a-mole of finding all the ways we might need to extend these notifications. Moreover, there was a bug in the implementation that used notification keys as the keys for the lifetime extension map, rather than NotificationEntry instances themselves, which caused b/227254780. This CL introduces the concept of reporting future dismissals to the NotifCollection (via OnUserInteractionCallback) which allows the NotifCollection to respond to system server's notification removal events by preventing the eventual dismissal call from crashing. This design is not the ideal end-game (see b/232260346) but rather preserves the existing call order into NotifCollection while also bailing out before any calls are made that would cause a crash. Test: atest ExpandableNotificationRowTest StatusBarNotificationActivityStarterTest NotifCollectionTest Fixes: 230540148 Fixes: 227254780 Bug: 232260346 Change-Id: Ia66ae5ade3fbfdd0436d54dcbeef720618622716 --- .../collection/NotifCollection.java | 157 +++++++++++++++++- .../coordinator/NotifCoordinators.kt | 2 +- .../OnUserInteractionCallbackImpl.java | 59 ++----- .../OnUserInteractionCallbackImplLegacy.java | 19 ++- .../notifcollection/NotifCollectionLogger.kt | 81 +++++++++ .../dagger/NotificationsModule.java | 3 +- .../row/ExpandableNotificationRow.java | 8 +- .../row/OnUserInteractionCallback.java | 33 ++-- .../StatusBarNotificationActivityStarter.java | 39 ++--- .../collection/NotifCollectionTest.java | 71 +++++++- .../row/ExpandableNotificationRowTest.java | 4 +- .../row/NotificationTestHelper.java | 7 + ...tusBarNotificationActivityStarterTest.java | 28 +++- 13 files changed, 381 insertions(+), 130 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java index bcd8e594ffbdf..6085096ee124e 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java @@ -18,9 +18,12 @@ package com.android.systemui.statusbar.notification.collection; import static android.service.notification.NotificationListenerService.REASON_APP_CANCEL; import static android.service.notification.NotificationListenerService.REASON_APP_CANCEL_ALL; +import static android.service.notification.NotificationListenerService.REASON_ASSISTANT_CANCEL; import static android.service.notification.NotificationListenerService.REASON_CANCEL; import static android.service.notification.NotificationListenerService.REASON_CANCEL_ALL; import static android.service.notification.NotificationListenerService.REASON_CHANNEL_BANNED; +import static android.service.notification.NotificationListenerService.REASON_CHANNEL_REMOVED; +import static android.service.notification.NotificationListenerService.REASON_CLEAR_DATA; import static android.service.notification.NotificationListenerService.REASON_CLICK; import static android.service.notification.NotificationListenerService.REASON_ERROR; import static android.service.notification.NotificationListenerService.REASON_GROUP_OPTIMIZATION; @@ -36,9 +39,11 @@ import static android.service.notification.NotificationListenerService.REASON_TI import static android.service.notification.NotificationListenerService.REASON_UNAUTOBUNDLED; import static android.service.notification.NotificationListenerService.REASON_USER_STOPPED; +import static com.android.systemui.statusbar.notification.NotificationUtils.logKey; import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.DISMISSED; import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.NOT_DISMISSED; import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.PARENT_DISMISSED; +import static com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLoggerKt.cancellationReasonDebugString; import static java.util.Objects.requireNonNull; @@ -99,6 +104,7 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -143,6 +149,7 @@ public class NotifCollection implements Dumpable { private final Map mNotificationSet = new ArrayMap<>(); private final Collection mReadOnlyNotificationSet = Collections.unmodifiableCollection(mNotificationSet.values()); + private final HashMap mFutureDismissals = new HashMap<>(); @Nullable private CollectionReadyForBuildListener mBuildListener; private final List mNotifCollectionListeners = new ArrayList<>(); @@ -511,6 +518,7 @@ public class NotifCollection implements Dumpable { cancelDismissInterception(entry); mEventQueue.add(new EntryRemovedEvent(entry, entry.mCancellationReason)); mEventQueue.add(new CleanUpEntryEvent(entry)); + handleFutureDismissal(entry); return true; } else { return false; @@ -519,31 +527,32 @@ public class NotifCollection implements Dumpable { /** * Get the group summary entry - * @param group + * @param groupKey * @return */ @Nullable - public NotificationEntry getGroupSummary(String group) { + public NotificationEntry getGroupSummary(String groupKey) { return mNotificationSet .values() .stream() - .filter(it -> Objects.equals(it.getSbn().getGroup(), group)) + .filter(it -> Objects.equals(it.getSbn().getGroupKey(), groupKey)) .filter(it -> it.getSbn().getNotification().isGroupSummary()) .findFirst().orElse(null); } /** - * Checks if the entry is the only child in the logical group - * @param entry - * @return + * Checks if the entry is the only child in the logical group; + * it need not have a summary to qualify + * + * @param entry the entry to check */ public boolean isOnlyChildInGroup(NotificationEntry entry) { - String group = entry.getSbn().getGroup(); + String groupKey = entry.getSbn().getGroupKey(); return mNotificationSet.get(entry.getKey()) == entry && mNotificationSet .values() .stream() - .filter(it -> Objects.equals(it.getSbn().getGroup(), group)) + .filter(it -> Objects.equals(it.getSbn().getGroupKey(), groupKey)) .filter(it -> !it.getSbn().getNotification().isGroupSummary()) .count() == 1; } @@ -916,10 +925,139 @@ public class NotifCollection implements Dumpable { dispatchEventsAndRebuildList(); } + /** + * A method to alert the collection that an async operation is happening, at the end of which a + * dismissal request will be made. This method has the additional guarantee that if a parent + * notification exists for a single child, then that notification will also be dismissed. + * + * The runnable returned must be run at the end of the async operation to enact the cancellation + * + * @param entry the notification we want to dismiss + * @param cancellationReason the reason for the cancellation + * @param statsCreator the callback for generating the stats for an entry + * @return the runnable to be run when the dismissal is ready to happen + */ + public Runnable registerFutureDismissal(NotificationEntry entry, int cancellationReason, + DismissedByUserStatsCreator statsCreator) { + FutureDismissal dismissal = mFutureDismissals.get(entry.getKey()); + if (dismissal != null) { + mLogger.logFutureDismissalReused(dismissal); + return dismissal; + } + dismissal = new FutureDismissal(entry, cancellationReason, statsCreator); + mFutureDismissals.put(entry.getKey(), dismissal); + mLogger.logFutureDismissalRegistered(dismissal); + return dismissal; + } + + private void handleFutureDismissal(NotificationEntry entry) { + final FutureDismissal futureDismissal = mFutureDismissals.remove(entry.getKey()); + if (futureDismissal != null) { + futureDismissal.onSystemServerCancel(entry.mCancellationReason); + } + } + + /** A single method interface that callers can pass in when registering future dismissals */ + public interface DismissedByUserStatsCreator { + DismissedByUserStats createDismissedByUserStats(NotificationEntry entry); + } + + /** A class which tracks the double dismissal events coming in from both the system server and + * the ui */ + public class FutureDismissal implements Runnable { + private final NotificationEntry mEntry; + private final DismissedByUserStatsCreator mStatsCreator; + @Nullable + private final NotificationEntry mSummaryToDismiss; + private final String mLabel; + + private boolean mDidRun; + private boolean mDidSystemServerCancel; + + private FutureDismissal(NotificationEntry entry, @CancellationReason int cancellationReason, + DismissedByUserStatsCreator statsCreator) { + mEntry = entry; + mStatsCreator = statsCreator; + mSummaryToDismiss = fetchSummaryToDismiss(entry); + mLabel = ""; + } + + @Nullable + private NotificationEntry fetchSummaryToDismiss(NotificationEntry entry) { + if (isOnlyChildInGroup(entry)) { + String group = entry.getSbn().getGroupKey(); + NotificationEntry summary = getGroupSummary(group); + if (summary != null && summary.isDismissable()) return summary; + } + return null; + } + + /** called when the entry has been removed from the collection */ + public void onSystemServerCancel(@CancellationReason int cancellationReason) { + Assert.isMainThread(); + if (mDidSystemServerCancel) { + mLogger.logFutureDismissalDoubleCancelledByServer(this); + return; + } + mLogger.logFutureDismissalGotSystemServerCancel(this, cancellationReason); + mDidSystemServerCancel = true; + // TODO: Internally dismiss the summary now instead of waiting for onUiCancel + } + + private void onUiCancel() { + mFutureDismissals.remove(mEntry.getKey()); + final NotificationEntry currentEntry = getEntry(mEntry.getKey()); + // generate stats for the entry before dismissing summary, which could affect state + final DismissedByUserStats stats = mStatsCreator.createDismissedByUserStats(mEntry); + // dismiss the summary (if it exists) + if (mSummaryToDismiss != null) { + final NotificationEntry currentSummary = getEntry(mSummaryToDismiss.getKey()); + if (currentSummary == mSummaryToDismiss) { + mLogger.logFutureDismissalDismissing(this, "summary"); + dismissNotification(mSummaryToDismiss, + mStatsCreator.createDismissedByUserStats(mSummaryToDismiss)); + } else { + mLogger.logFutureDismissalMismatchedEntry(this, "summary", currentSummary); + } + } + // dismiss this entry (if it is still around) + if (mDidSystemServerCancel) { + mLogger.logFutureDismissalAlreadyCancelledByServer(this); + } else if (currentEntry == mEntry) { + mLogger.logFutureDismissalDismissing(this, "entry"); + dismissNotification(mEntry, stats); + } else { + mLogger.logFutureDismissalMismatchedEntry(this, "entry", currentEntry); + } + } + + /** called when the dismissal should be completed */ + @Override + public void run() { + Assert.isMainThread(); + if (mDidRun) { + mLogger.logFutureDismissalDoubleRun(this); + return; + } + mDidRun = true; + onUiCancel(); + } + + /** provides a debug label for this instance */ + public String getLabel() { + return mLabel; + } + } + @IntDef(prefix = { "REASON_" }, value = { REASON_NOT_CANCELED, REASON_UNKNOWN, REASON_CLICK, + REASON_CANCEL, REASON_CANCEL_ALL, REASON_ERROR, REASON_PACKAGE_CHANGED, @@ -937,6 +1075,9 @@ public class NotifCollection implements Dumpable { REASON_CHANNEL_BANNED, REASON_SNOOZED, REASON_TIMEOUT, + REASON_CHANNEL_REMOVED, + REASON_CLEAR_DATA, + REASON_ASSISTANT_CANCEL, }) @Retention(RetentionPolicy.SOURCE) public @interface CancellationReason {} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt index b24d2922adfbb..acb26a930689b 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt @@ -93,7 +93,7 @@ class NotifCoordinatorsImpl @Inject constructor( mCoordinators.add(shadeEventCoordinator) mCoordinators.add(viewConfigCoordinator) mCoordinators.add(visualStabilityCoordinator) - mCoordinators.add(activityLaunchAnimCoordinator) +// mCoordinators.add(activityLaunchAnimCoordinator) // NOTE: will delete in followup CL if (notifPipelineFlags.isSmartspaceDedupingEnabled()) { mCoordinators.add(smartspaceDedupingCoordinator) } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java index 3bd91b5c84806..7dd3672a6e306 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java @@ -18,17 +18,17 @@ package com.android.systemui.statusbar.notification.collection.inflation; import static android.service.notification.NotificationStats.DISMISS_SENTIMENT_NEUTRAL; -import android.annotation.Nullable; import android.os.SystemClock; -import android.service.notification.NotificationListenerService; import android.service.notification.NotificationStats; +import androidx.annotation.NonNull; + import com.android.systemui.plugins.statusbar.StatusBarStateController; import com.android.systemui.statusbar.notification.collection.NotifCollection; +import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.coordinator.VisualStabilityCoordinator; import com.android.systemui.statusbar.notification.collection.notifcollection.DismissedByUserStats; -import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager; import com.android.systemui.statusbar.notification.collection.render.NotificationVisibilityProvider; import com.android.systemui.statusbar.notification.row.OnUserInteractionCallback; import com.android.systemui.statusbar.policy.HeadsUpManager; @@ -43,54 +43,33 @@ public class OnUserInteractionCallbackImpl implements OnUserInteractionCallback private final HeadsUpManager mHeadsUpManager; private final StatusBarStateController mStatusBarStateController; private final VisualStabilityCoordinator mVisualStabilityCoordinator; - private final GroupMembershipManager mGroupMembershipManager; public OnUserInteractionCallbackImpl( NotificationVisibilityProvider visibilityProvider, NotifCollection notifCollection, HeadsUpManager headsUpManager, StatusBarStateController statusBarStateController, - VisualStabilityCoordinator visualStabilityCoordinator, - GroupMembershipManager groupMembershipManager + VisualStabilityCoordinator visualStabilityCoordinator ) { mVisibilityProvider = visibilityProvider; mNotifCollection = notifCollection; mHeadsUpManager = headsUpManager; mStatusBarStateController = statusBarStateController; mVisualStabilityCoordinator = visualStabilityCoordinator; - mGroupMembershipManager = groupMembershipManager; } - /** - * Callback triggered when a user: - * 1. Manually dismisses a notification {@see ExpandableNotificationRow}. - * 2. Clicks on a notification with flag {@link android.app.Notification#FLAG_AUTO_CANCEL}. - * {@see StatusBarNotificationActivityStarter} - */ - @Override - public void onDismiss( - NotificationEntry entry, - @NotificationListenerService.NotificationCancelReason int cancellationReason, - @Nullable NotificationEntry groupSummaryToDismiss - ) { + @NonNull + private DismissedByUserStats getDismissedByUserStats(NotificationEntry entry) { int dismissalSurface = NotificationStats.DISMISSAL_SHADE; if (mHeadsUpManager.isAlerting(entry.getKey())) { dismissalSurface = NotificationStats.DISMISSAL_PEEK; } else if (mStatusBarStateController.isDozing()) { dismissalSurface = NotificationStats.DISMISSAL_AOD; } - - if (groupSummaryToDismiss != null) { - onDismiss(groupSummaryToDismiss, cancellationReason, null); - } - - mNotifCollection.dismissNotification( - entry, - new DismissedByUserStats( - dismissalSurface, - DISMISS_SENTIMENT_NEUTRAL, - mVisibilityProvider.obtain(entry, true)) - ); + return new DismissedByUserStats( + dismissalSurface, + DISMISS_SENTIMENT_NEUTRAL, + mVisibilityProvider.obtain(entry, true)); } @Override @@ -100,19 +79,11 @@ public class OnUserInteractionCallbackImpl implements OnUserInteractionCallback SystemClock.uptimeMillis()); } - /** - * @param entry that is being dismissed - * @return the group summary to dismiss along with this entry if this is the last entry in - * the group. Else, returns null. - */ + @NonNull @Override - @Nullable - public NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry) { - String group = entry.getSbn().getGroup(); - if (mNotifCollection.isOnlyChildInGroup(entry)) { - NotificationEntry summary = mNotifCollection.getGroupSummary(group); - if (summary != null && summary.isDismissable()) return summary; - } - return null; + public Runnable registerFutureDismissal(@NonNull NotificationEntry entry, + @CancellationReason int cancellationReason) { + return mNotifCollection.registerFutureDismissal( + entry, cancellationReason, this::getDismissedByUserStats); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java index 8daf8be0cc8ff..103b14b09e9c0 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java @@ -18,12 +18,15 @@ package com.android.systemui.statusbar.notification.collection.legacy; import static android.service.notification.NotificationStats.DISMISS_SENTIMENT_NEUTRAL; -import android.annotation.Nullable; import android.service.notification.NotificationListenerService; import android.service.notification.NotificationStats; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; + import com.android.systemui.plugins.statusbar.StatusBarStateController; import com.android.systemui.statusbar.notification.NotificationEntryManager; +import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.notifcollection.DismissedByUserStats; import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager; @@ -68,8 +71,7 @@ public class OnUserInteractionCallbackImplLegacy implements OnUserInteractionCal * along with this dismissal. If null, does not additionally * dismiss any notifications. */ - @Override - public void onDismiss( + private void onDismiss( NotificationEntry entry, @NotificationListenerService.NotificationCancelReason int cancellationReason, @Nullable NotificationEntry groupSummaryToDismiss @@ -106,14 +108,21 @@ public class OnUserInteractionCallbackImplLegacy implements OnUserInteractionCal * @return the group summary to dismiss along with this entry if this is the last entry in * the group. Else, returns null. */ - @Override @Nullable - public NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry) { + private NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry) { if (mGroupMembershipManager.isOnlyChildInGroup(entry)) { NotificationEntry groupSummary = mGroupMembershipManager.getLogicalGroupSummary(entry); return groupSummary.isDismissable() ? groupSummary : null; } return null; } + + @Override + @NonNull + public Runnable registerFutureDismissal(@NonNull NotificationEntry entry, + @CancellationReason int cancellationReason) { + NotificationEntry groupSummaryToDismiss = getGroupSummaryToDismiss(entry); + return () -> onDismiss(entry, cancellationReason, groupSummaryToDismiss); + } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt index 7302de57e9551..7e7936717b84e 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt @@ -28,7 +28,9 @@ import com.android.systemui.log.LogLevel.WTF import com.android.systemui.log.dagger.NotificationLog import com.android.systemui.statusbar.notification.collection.NotifCollection import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason +import com.android.systemui.statusbar.notification.collection.NotifCollection.FutureDismissal import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.logKey import javax.inject.Inject fun cancellationReasonDebugString(@CancellationReason reason: Int) = @@ -36,6 +38,7 @@ fun cancellationReasonDebugString(@CancellationReason reason: Int) = -1 -> "REASON_NOT_CANCELED" // NotifCollection.REASON_NOT_CANCELED NotifCollection.REASON_UNKNOWN -> "REASON_UNKNOWN" NotificationListenerService.REASON_CLICK -> "REASON_CLICK" + NotificationListenerService.REASON_CANCEL -> "REASON_CANCEL" NotificationListenerService.REASON_CANCEL_ALL -> "REASON_CANCEL_ALL" NotificationListenerService.REASON_ERROR -> "REASON_ERROR" NotificationListenerService.REASON_PACKAGE_CHANGED -> "REASON_PACKAGE_CHANGED" @@ -53,6 +56,9 @@ fun cancellationReasonDebugString(@CancellationReason reason: Int) = NotificationListenerService.REASON_CHANNEL_BANNED -> "REASON_CHANNEL_BANNED" NotificationListenerService.REASON_SNOOZED -> "REASON_SNOOZED" NotificationListenerService.REASON_TIMEOUT -> "REASON_TIMEOUT" + NotificationListenerService.REASON_CHANNEL_REMOVED -> "REASON_CHANNEL_REMOVED" + NotificationListenerService.REASON_CLEAR_DATA -> "REASON_CLEAR_DATA" + NotificationListenerService.REASON_ASSISTANT_CANCEL -> "REASON_ASSISTANT_CANCEL" else -> "unknown" } @@ -241,6 +247,81 @@ class NotifCollectionLogger @Inject constructor( "ERROR suppressed due to initialization forgiveness: $str1" }) } + + fun logFutureDismissalReused(dismissal: FutureDismissal) { + buffer.log(TAG, INFO, { + str1 = dismissal.label + }, { + "Reusing existing registration: $str1" + }) + } + + fun logFutureDismissalRegistered(dismissal: FutureDismissal) { + buffer.log(TAG, DEBUG, { + str1 = dismissal.label + }, { + "Registered: $str1" + }) + } + + fun logFutureDismissalDoubleCancelledByServer(dismissal: FutureDismissal) { + buffer.log(TAG, WARNING, { + str1 = dismissal.label + }, { + "System server double cancelled: $str1" + }) + } + + fun logFutureDismissalDoubleRun(dismissal: FutureDismissal) { + buffer.log(TAG, WARNING, { + str1 = dismissal.label + }, { + "Double run: $str1" + }) + } + + fun logFutureDismissalAlreadyCancelledByServer(dismissal: FutureDismissal) { + buffer.log(TAG, DEBUG, { + str1 = dismissal.label + }, { + "Ignoring: entry already cancelled by server: $str1" + }) + } + + fun logFutureDismissalGotSystemServerCancel( + dismissal: FutureDismissal, + @CancellationReason cancellationReason: Int + ) { + buffer.log(TAG, DEBUG, { + str1 = dismissal.label + int1 = cancellationReason + }, { + "SystemServer cancelled: $str1 reason=${cancellationReasonDebugString(int1)}" + }) + } + + fun logFutureDismissalDismissing(dismissal: FutureDismissal, type: String) { + buffer.log(TAG, DEBUG, { + str1 = dismissal.label + str2 = type + }, { + "Dismissing $str2 for: $str1" + }) + } + + fun logFutureDismissalMismatchedEntry( + dismissal: FutureDismissal, + type: String, + latestEntry: NotificationEntry? + ) { + buffer.log(TAG, WARNING, { + str1 = dismissal.label + str2 = type + str3 = latestEntry.logKey + }, { + "Mismatch: current $str2 is $str3 for: $str1" + }) + } } private const val TAG = "NotifCollection" diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java index d96590a825479..94848e83a3608 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java @@ -350,8 +350,7 @@ public interface NotificationsModule { notifCollection.get(), headsUpManager, statusBarStateController, - visualStabilityCoordinator.get(), - groupMembershipManagerLazy.get()) + visualStabilityCoordinator.get()) : new OnUserInteractionCallbackImplLegacy( entryManager, visibilityProvider.get(), diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java index ed69e0609096e..af9d751ae545e 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java @@ -17,7 +17,6 @@ package com.android.systemui.statusbar.notification.row; import static android.app.Notification.Action.SEMANTIC_ACTION_MARK_CONVERSATION_AS_PRIORITY; -import static android.os.UserHandle.USER_SYSTEM; import static android.service.notification.NotificationListenerService.REASON_CANCEL; import static com.android.systemui.statusbar.notification.row.NotificationContentView.VISIBLE_TYPE_HEADSUP; @@ -34,8 +33,6 @@ import android.app.Notification; import android.app.NotificationChannel; import android.app.role.RoleManager; import android.content.Context; -import android.content.pm.PackageInfo; -import android.content.pm.PackageManager; import android.content.res.Configuration; import android.content.res.Resources; import android.graphics.Canvas; @@ -51,7 +48,6 @@ import android.os.Bundle; import android.os.RemoteException; import android.os.ServiceManager; import android.os.Trace; -import android.provider.Settings; import android.service.notification.StatusBarNotification; import android.util.ArraySet; import android.util.AttributeSet; @@ -112,7 +108,6 @@ import com.android.systemui.statusbar.notification.stack.ExpandableViewState; import com.android.systemui.statusbar.notification.stack.NotificationChildrenContainer; import com.android.systemui.statusbar.notification.stack.NotificationStackScrollLayout; import com.android.systemui.statusbar.notification.stack.SwipeableView; -import com.android.systemui.statusbar.phone.CentralSurfaces; import com.android.systemui.statusbar.phone.KeyguardBypassController; import com.android.systemui.statusbar.policy.HeadsUpManager; import com.android.systemui.statusbar.policy.InflatedSmartReplyState; @@ -1455,8 +1450,7 @@ public class ExpandableNotificationRow extends ActivatableNotificationView dismiss(fromAccessibility); if (mEntry.isDismissable()) { if (mOnUserInteractionCallback != null) { - mOnUserInteractionCallback.onDismiss(mEntry, REASON_CANCEL, - mOnUserInteractionCallback.getGroupSummaryToDismiss(mEntry)); + mOnUserInteractionCallback.registerFutureDismissal(mEntry, REASON_CANCEL).run(); } } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java index 94c5507cfbf42..98d43539841df 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java @@ -16,8 +16,9 @@ package com.android.systemui.statusbar.notification.row; -import android.service.notification.NotificationListenerService; +import androidx.annotation.NonNull; +import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason; import com.android.systemui.statusbar.notification.collection.NotificationEntry; /** @@ -25,30 +26,24 @@ import com.android.systemui.statusbar.notification.collection.NotificationEntry; */ public interface OnUserInteractionCallback { - /** - * Handle a user interaction that triggers a notification dismissal. Called when a user clicks - * on an auto-cancelled notification or manually swipes to dismiss the notification. - * - * @param entry notification being dismissed - * @param cancellationReason reason for the cancellation - * @param groupSummaryToDismiss group summary to dismiss with `entry`. - */ - void onDismiss( - NotificationEntry entry, - @NotificationListenerService.NotificationCancelReason int cancellationReason, - NotificationEntry groupSummaryToDismiss); - /** * Triggered after a user has changed the importance of the notification via its * {@link NotificationGuts}. */ void onImportanceChanged(NotificationEntry entry); - /** - * @param entry being dismissed by the user - * @return group summary that should be dismissed along with `entry`. Can be null if no - * relevant group summary exists or the group summary should not be dismissed with `entry`. + * Called once it is known that a dismissal will take place for the given reason. + * This returns a Runnable which MUST be invoked when the dismissal is ready to be completed. + * + * Registering for future dismissal is typically done before notifying the NMS that a + * notification was clicked or dismissed, but the local dismissal may happen later. + * + * @param entry the entry being cancelled + * @param cancellationReason the reason for the cancellation + * @return the runnable to call when the dismissal can happen */ - NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry); + @NonNull + Runnable registerFutureDismissal(@NonNull NotificationEntry entry, + @CancellationReason int cancellationReason); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java index af4c81b4181d8..eee7b2a41de49 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java @@ -380,22 +380,15 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte final NotificationVisibility nv = mVisibilityProvider.obtain(entry, true); - // retrieve the group summary to remove with this entry before we tell NMS the - // notification was clicked to avoid a race condition - final boolean shouldAutoCancel = shouldAutoCancel(entry.getSbn()); - final NotificationEntry summaryToRemove = shouldAutoCancel - ? mOnUserInteractionCallback.getGroupSummaryToDismiss(entry) : null; - - // inform NMS that the notification was clicked - mClickNotifier.onNotificationClick(notificationKey, nv); - - if (!canBubble && (shouldAutoCancel + if (!canBubble && (shouldAutoCancel(entry.getSbn()) || mRemoteInputManager.isNotificationKeptForRemoteInputHistory(notificationKey))) { + final Runnable locallyDismissNotification = + mOnUserInteractionCallback.registerFutureDismissal(entry, REASON_CLICK); // Immediately remove notification from visually showing. // We have to post the removal to the UI thread for synchronization. mMainThreadHandler.post(() -> { final Runnable removeNotification = () -> { - mOnUserInteractionCallback.onDismiss(entry, REASON_CLICK, summaryToRemove); + locallyDismissNotification.run(); if (!animate) { // If we're animating, this would be invoked after the activity launch // animation completes. Since we're not animating, the launch already @@ -417,6 +410,9 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte () -> mLaunchEventsEmitter.notifyFinishLaunchNotifActivity(entry)); } + // inform NMS that the notification was clicked + mClickNotifier.onNotificationClick(notificationKey, nv); + mIsCollapsingToShowActivityOverLockscreen = false; } @@ -433,24 +429,14 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte // will focus follow operation only after drag-and-drop that notification. final NotificationVisibility nv = mVisibilityProvider.obtain(entry, true); - // retrieve the group summary to remove with this entry before we tell NMS the - // notification was clicked to avoid a race condition - final boolean shouldAutoCancel = shouldAutoCancel(entry.getSbn()); - final NotificationEntry summaryToRemove = shouldAutoCancel - ? mOnUserInteractionCallback.getGroupSummaryToDismiss(entry) : null; - String notificationKey = entry.getKey(); - // inform NMS that the notification was clicked - mClickNotifier.onNotificationClick(notificationKey, nv); - - if (shouldAutoCancel || mRemoteInputManager.isNotificationKeptForRemoteInputHistory( - notificationKey)) { + if (shouldAutoCancel(entry.getSbn()) + || mRemoteInputManager.isNotificationKeptForRemoteInputHistory(notificationKey)) { + final Runnable removeNotification = + mOnUserInteractionCallback.registerFutureDismissal(entry, REASON_CLICK); // Immediately remove notification from visually showing. // We have to post the removal to the UI thread for synchronization. mMainThreadHandler.post(() -> { - final Runnable removeNotification = () -> - mOnUserInteractionCallback.onDismiss( - entry, REASON_CLICK, summaryToRemove); if (mPresenter.isCollapsing()) { // To avoid lags we're only performing the remove // after the shade is collapsed @@ -461,6 +447,9 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte }); } + // inform NMS that the notification was clicked + mClickNotifier.onNotificationClick(notificationKey, nv); + mIsCollapsingToShowActivityOverLockscreen = false; } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java index 706800940fd12..958d54230f1c6 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java @@ -32,6 +32,8 @@ import static com.android.systemui.statusbar.notification.collection.Notificatio import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.NOT_DISMISSED; import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.PARENT_DISMISSED; +import static com.google.common.truth.Truth.assertThat; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; @@ -47,6 +49,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import static java.util.Collections.singletonList; @@ -180,13 +183,14 @@ public class NotifCollectionTest extends SysuiTestCase { @Test public void testGetGroupSummary() { - assertEquals(null, mCollection.getGroupSummary("group")); - NotifEvent summary = mNoMan.postNotif( - buildNotif(TEST_PACKAGE, 0) - .setGroup(mContext, "group") - .setGroupSummary(mContext, true)); + final NotificationEntryBuilder entryBuilder = buildNotif(TEST_PACKAGE, 0) + .setGroup(mContext, "group") + .setGroupSummary(mContext, true); + final String groupKey = entryBuilder.build().getSbn().getGroupKey(); + assertEquals(null, mCollection.getGroupSummary(groupKey)); + NotifEvent summary = mNoMan.postNotif(entryBuilder); - final NotificationEntry entry = mCollection.getGroupSummary("group"); + final NotificationEntry entry = mCollection.getGroupSummary(groupKey); assertEquals(summary.key, entry.getKey()); assertEquals(summary.sbn, entry.getSbn()); assertEquals(summary.ranking, entry.getRanking()); @@ -194,9 +198,9 @@ public class NotifCollectionTest extends SysuiTestCase { @Test public void testIsOnlyChildInGroup() { - NotifEvent notif1 = mNoMan.postNotif( - buildNotif(TEST_PACKAGE, 1) - .setGroup(mContext, "group")); + final NotificationEntryBuilder entryBuilder = buildNotif(TEST_PACKAGE, 1) + .setGroup(mContext, "group"); + NotifEvent notif1 = mNoMan.postNotif(entryBuilder); final NotificationEntry entry = mCollection.getEntry(notif1.key); assertTrue(mCollection.isOnlyChildInGroup(entry)); @@ -1487,6 +1491,55 @@ public class NotifCollectionTest extends SysuiTestCase { verify(mCollectionListener, never()).onEntryUpdated(any(), anyBoolean()); } + @Test + public void testRegisterFutureDismissal() throws RemoteException { + // GIVEN a pipeline with one notification + NotifEvent notifEvent = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + NotificationEntry entry = requireNonNull(mCollection.getEntry(notifEvent.key)); + clearInvocations(mCollectionListener); + + // WHEN registering a future dismissal, nothing happens right away + final Runnable onDismiss = mCollection.registerFutureDismissal(entry, REASON_CLICK, + NotifCollectionTest::defaultStats); + verifyNoMoreInteractions(mCollectionListener); + + // WHEN finally dismissing + onDismiss.run(); + verify(mStatusBarService).onNotificationClear(any(), anyInt(), eq(notifEvent.key), + anyInt(), anyInt(), any()); + verifyNoMoreInteractions(mStatusBarService); + verifyNoMoreInteractions(mCollectionListener); + } + + @Test + public void testRegisterFutureDismissalWithRetractionAndRepost() { + // GIVEN a pipeline with one notification + NotifEvent notifEvent = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + NotificationEntry entry = requireNonNull(mCollection.getEntry(notifEvent.key)); + clearInvocations(mCollectionListener); + + // WHEN registering a future dismissal, nothing happens right away + final Runnable onDismiss = mCollection.registerFutureDismissal(entry, REASON_CLICK, + NotifCollectionTest::defaultStats); + verifyNoMoreInteractions(mCollectionListener); + + // WHEN retracting the notification, and then reposting + mNoMan.retractNotif(notifEvent.sbn, REASON_CLICK); + mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + clearInvocations(mCollectionListener); + + // KNOWING that the entry in the collection is different now + assertThat(mCollection.getEntry(notifEvent.key)).isNotSameInstanceAs(entry); + + // WHEN finally dismissing + onDismiss.run(); + + // VERIFY that nothing happens; the notification should not be removed + verifyNoMoreInteractions(mCollectionListener); + assertThat(mCollection.getEntry(notifEvent.key)).isNotNull(); + verifyNoMoreInteractions(mStatusBarService); + } + @Test public void testCannotDismissOngoingNotificationChildren() { // GIVEN an ongoing notification diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java index b3c34c1e4ff4e..f36ffe6fb198b 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java @@ -335,7 +335,7 @@ public class ExpandableNotificationRowTest extends SysuiTestCase { .build(); row.performDismiss(false); verify(mNotificationTestHelper.mOnUserInteractionCallback) - .onDismiss(any(), anyInt(), any()); + .registerFutureDismissal(any(), anyInt()); } @Test @@ -347,6 +347,6 @@ public class ExpandableNotificationRowTest extends SysuiTestCase { .build(); row.performDismiss(false); verify(mNotificationTestHelper.mOnUserInteractionCallback, never()) - .onDismiss(any(), anyInt(), any()); + .registerFutureDismissal(any(), anyInt()); } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java index d5ed37a570d25..7a8b329bec64d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java @@ -23,8 +23,11 @@ import static android.app.NotificationManager.IMPORTANCE_HIGH; import static com.android.systemui.statusbar.NotificationEntryHelper.modifyRanking; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import android.annotation.Nullable; import android.app.ActivityManager; @@ -121,6 +124,7 @@ public class NotificationTestHelper { private StatusBarStateController mStatusBarStateController; private final PeopleNotificationIdentifier mPeopleNotificationIdentifier; public final OnUserInteractionCallback mOnUserInteractionCallback; + public final Runnable mFutureDismissalRunnable; public NotificationTestHelper( Context context, @@ -182,6 +186,9 @@ public class NotificationTestHelper { mBindPipelineEntryListener = collectionListenerCaptor.getValue(); mPeopleNotificationIdentifier = mock(PeopleNotificationIdentifier.class); mOnUserInteractionCallback = mock(OnUserInteractionCallback.class); + mFutureDismissalRunnable = mock(Runnable.class); + when(mOnUserInteractionCallback.registerFutureDismissal(any(), anyInt())) + .thenReturn(mFutureDismissalRunnable); } /** diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java index f1a6eba129525..273b9a17ce1d4 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java @@ -93,6 +93,7 @@ import org.mockito.MockitoAnnotations; import org.mockito.stubbing.Answer; import java.util.ArrayList; +import java.util.List; import java.util.Optional; @SmallTest @@ -141,6 +142,8 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { @Mock private OnUserInteractionCallback mOnUserInteractionCallback; @Mock + private Runnable mFutureDismissalRunnable; + @Mock private StatusBarNotificationActivityStarter mNotificationActivityStarter; @Mock private ActivityLaunchAnimator mActivityLaunchAnimator; @@ -187,8 +190,8 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { when(mEntryManager.getVisibleNotifications()).thenReturn(mActiveNotifications); when(mStatusBarStateController.getState()).thenReturn(StatusBarState.SHADE); when(mNotifPipelineFlags.isNewPipelineEnabled()).thenReturn(false); - when(mOnUserInteractionCallback.getGroupSummaryToDismiss(mNotificationRow.getEntry())) - .thenReturn(null); + when(mOnUserInteractionCallback.registerFutureDismissal(eq(mNotificationRow.getEntry()), + anyInt())).thenReturn(mFutureDismissalRunnable); when(mVisibilityProvider.obtain(anyString(), anyBoolean())) .thenAnswer(invocation -> NotificationVisibility.obtain( invocation.getArgument(0), 0, 1, false)); @@ -264,6 +267,10 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { @Test public void testOnNotificationClicked_keyGuardShowing() throws PendingIntent.CanceledException, RemoteException { + // To get the order right, collect posted runnables and run them later + List runnables = new ArrayList<>(); + doAnswer(answerVoid(r -> runnables.add((Runnable) r))) + .when(mHandler).post(any(Runnable.class)); // Given NotificationEntry entry = mNotificationRow.getEntry(); Notification notification = entry.getSbn().getNotification(); @@ -275,6 +282,8 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { // When mNotificationActivityStarter.onNotificationClicked(entry, mNotificationRow); + // Run the collected runnables in fifo order, the way post() really does. + while (!runnables.isEmpty()) runnables.remove(0).run(); // Then verify(mShadeController, atLeastOnce()).collapsePanel(); @@ -284,12 +293,14 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { verify(mAssistManager).hideAssist(); - InOrder orderVerifier = Mockito.inOrder(mClickNotifier, mOnUserInteractionCallback); + InOrder orderVerifier = Mockito.inOrder(mClickNotifier, mOnUserInteractionCallback, + mFutureDismissalRunnable); + // Notification calls dismiss callback to remove notification due to FLAG_AUTO_CANCEL + orderVerifier.verify(mOnUserInteractionCallback) + .registerFutureDismissal(eq(entry), eq(REASON_CLICK)); orderVerifier.verify(mClickNotifier).onNotificationClick( eq(entry.getKey()), any(NotificationVisibility.class)); - // Notification calls dismiss callback to remove notification due to FLAG_AUTO_CANCEL - orderVerifier.verify(mOnUserInteractionCallback).onDismiss(entry, - REASON_CLICK, null); + orderVerifier.verify(mFutureDismissalRunnable).run(); } @Test @@ -319,8 +330,9 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { verifyZeroInteractions(mContentIntent); // Notification should not be cancelled. - verify(mOnUserInteractionCallback, never()).onDismiss(eq(mNotificationRow.getEntry()), - anyInt(), eq(null)); + verify(mOnUserInteractionCallback, never()) + .registerFutureDismissal(eq(mNotificationRow.getEntry()), anyInt()); + verify(mFutureDismissalRunnable, never()).run(); } @Test