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

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<String, NotificationEntry> mNotificationSet = new ArrayMap<>();
     private final Collection<NotificationEntry> mReadOnlyNotificationSet =
             Collections.unmodifiableCollection(mNotificationSet.values());
+    private final HashMap<String, FutureDismissal> mFutureDismissals = new HashMap<>();
 
     @Nullable private CollectionReadyForBuildListener mBuildListener;
     private final List<NotifCollectionListener> 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 = "<FutureDismissal@" + Integer.toHexString(hashCode())
+                    + " entry=" + logKey(mEntry)
+                    + " reason=" + cancellationReasonDebugString(cancellationReason)
+                    + " summary=" + logKey(mSummaryToDismiss)
+                    + ">";
+        }
+
+        @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 {}

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

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

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

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"

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(),

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

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

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

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

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

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);
     }
 
     /**

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