Lifetime extension to not update on multicancel

Adds a new field to NotificationRecord that records when a lifetime
extended notification has been cancelled by an app. Then, when an app
sends a cancellation, we check that field before we update system UI.
This allows us to avoid sending multiple updates to system ui when an
app cancels repeatedly. These multiple cancelations were causing
rendering issues.

As a side effect, we can also remove the ONLY_ALERT_ONCE flag we added,
because we'll no longer cause multiple huns to appear because of sending
multiple updates.

Bug: 299448097
Flag: android.app.lifetime_extension_refactor
Test: flash, atest NotificationManagerServiceTest
Change-Id: Idc8e16d7aef321eaa76c64214febb79af4690463

3 files changed, 193 insertions, 12 deletions

diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java
index ffb2bb6a4528..54bc6fbfd930 100644
--- a/services/core/java/com/android/server/notification/NotificationManagerService.java
+++ b/services/core/java/com/android/server/notification/NotificationManagerService.java
@@ -1583,6 +1583,8 @@ public class NotificationManagerService extends SystemService {
                     // respond to direct replies with updates. So we need to update System UI
                     // immediately.
                     if (lifetimeExtensionRefactor()) {
+                        // We need to reset this to allow the notif to be updated again.
+                        r.setCanceledAfterLifetimeExtension(false);
                         maybeNotifySystemUiListenerLifetimeExtendedLocked(r,
                                 r.getSbn().getPackageName(), packageImportance);
                     }
@@ -1639,9 +1641,12 @@ public class NotificationManagerService extends SystemService {
                     // respond to direct replies with updates. So we need to update System UI
                     // immediately.
                     if (lifetimeExtensionRefactor()) {
+                        // We need to reset this to allow the notif to be updated again.
+                        r.setCanceledAfterLifetimeExtension(false);
                         maybeNotifySystemUiListenerLifetimeExtendedLocked(r,
                                 r.getSbn().getPackageName(), packageImportance);
                     }
+
                     r.recordSmartReplied();
                     LogMaker logMaker = r.getLogMaker()
                             .setCategory(MetricsEvent.SMART_REPLY_ACTION)
@@ -11741,17 +11746,37 @@ public class NotificationManagerService extends SystemService {
     private void maybeNotifySystemUiListenerLifetimeExtendedLocked(NotificationRecord record,
             String pkg, int packageImportance) {
         if (record != null && (record.getSbn().getNotification().flags
-                & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY) > 0) {
+                & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY) > 0
+                && !record.isCanceledAfterLifetimeExtension()) {
             boolean isAppForeground = pkg != null && packageImportance == IMPORTANCE_FOREGROUND;
 
-            // Lifetime extended notifications don't need to alert on state change.
+            // Save the original Record's post silently value, so we can restore it after we send
+            // the SystemUI specific silent update.
+            boolean savedPostSilentlyState = record.shouldPostSilently();
+            boolean savedOnlyAlertOnceState = (record.getNotification().flags
+                    & FLAG_ONLY_ALERT_ONCE) > 0;
+            // Lifetime extended notifications don't need to alert on new state change.
             record.setPostSilently(true);
             // We also set FLAG_ONLY_ALERT_ONCE to avoid the notification from HUN-ing again.
             record.getNotification().flags |= FLAG_ONLY_ALERT_ONCE;
 
+            PostNotificationTracker tracker = mPostNotificationTrackerFactory.newTracker(null);
+            tracker.addCleanupRunnable(() -> {
+                synchronized (mNotificationLock) {
+                    // Mark that the notification has been updated due to cancelation, so it won't
+                    // be updated again if the app cancels multiple times.
+                    record.setCanceledAfterLifetimeExtension(true);
+                    // Set the post silently status to the record's previous value.
+                    record.setPostSilently(savedPostSilentlyState);
+                    // Remove FLAG_ONLY_ALERT_ONCE if the notification did not previously have it.
+                    if (!savedOnlyAlertOnceState) {
+                        record.getNotification().flags &= ~FLAG_ONLY_ALERT_ONCE;
+                    }
+                }
+            });
+
             mHandler.post(new EnqueueNotificationRunnable(record.getUser().getIdentifier(),
-                    record, isAppForeground, /* isAppProvided= */ false,
-                    mPostNotificationTrackerFactory.newTracker(null)));
+                    record, isAppForeground, /* isAppProvided= */ false, tracker));
         }
     }
 
@@ -13351,17 +13376,23 @@ public class NotificationManagerService extends SystemService {
         @ElapsedRealtimeLong private final long mStartTime;
         @Nullable private final WakeLock mWakeLock;
         private boolean mOngoing;
+        private final List<Runnable> mCleanupRunnables;
 
         @VisibleForTesting
         PostNotificationTracker(@Nullable WakeLock wakeLock) {
             mStartTime = SystemClock.elapsedRealtime();
             mWakeLock = wakeLock;
             mOngoing = true;
+            mCleanupRunnables = new ArrayList<Runnable>();
             if (DBG) {
                 Slog.d(TAG, "PostNotification: Started");
             }
         }
 
+        void addCleanupRunnable(Runnable runnable) {
+            mCleanupRunnables.add(runnable);
+        }
+
         @ElapsedRealtimeLong
         long getStartTime() {
             return mStartTime;
@@ -13373,8 +13404,9 @@ public class NotificationManagerService extends SystemService {
         }
 
         /**
-         * Cancels the tracker (releasing the acquired WakeLock). Either {@link #finish} or
-         * {@link #cancel} (exclusively) should be called on this object before it's discarded.
+         * Cancels the tracker (releasing the acquired WakeLock) and runs any set cleanup runnables.
+         * Either {@link #finish} or {@link #cancel} (exclusively) should be called on this object
+         * before it's discarded.
          */
         void cancel() {
             if (!isOngoing()) {
@@ -13385,6 +13417,9 @@ public class NotificationManagerService extends SystemService {
             if (mWakeLock != null) {
                 Binder.withCleanCallingIdentity(() -> mWakeLock.release());
             }
+            for (Runnable r : mCleanupRunnables) {
+                r.run();
+            }
             if (DBG) {
                 long elapsedTime = SystemClock.elapsedRealtime() - mStartTime;
                 Slog.d(TAG, TextUtils.formatSimple("PostNotification: Abandoned after %d ms",
@@ -13393,9 +13428,10 @@ public class NotificationManagerService extends SystemService {
         }
 
         /**
-         * Finishes the tracker (releasing the acquired WakeLock) and returns the time elapsed since
-         * the operation started, in milliseconds. Either {@link #finish} or {@link #cancel}
-         * (exclusively) should be called on this object before it's discarded.
+         * Finishes the tracker (releasing the acquired WakeLock), runs any set cleanup runnables,
+         * and returns the time elapsed since the operation started, in milliseconds.
+         * Either {@link #finish} or {@link #cancel} (exclusively) should be called on this object
+         * before it's discarded.
          */
         @DurationMillisLong
         long finish() {
@@ -13408,6 +13444,9 @@ public class NotificationManagerService extends SystemService {
             if (mWakeLock != null) {
                 Binder.withCleanCallingIdentity(() -> mWakeLock.release());
             }
+            for (Runnable r : mCleanupRunnables) {
+                r.run();
+            }
             if (DBG) {
                 Slog.d(TAG,
                         TextUtils.formatSimple("PostNotification: Finished in %d ms", elapsedTime));
diff --git a/services/core/java/com/android/server/notification/NotificationRecord.java b/services/core/java/com/android/server/notification/NotificationRecord.java
index 1392003a13e7..e54124608a26 100644
--- a/services/core/java/com/android/server/notification/NotificationRecord.java
+++ b/services/core/java/com/android/server/notification/NotificationRecord.java
@@ -222,6 +222,9 @@ public final class NotificationRecord {
     private boolean mPendingLogUpdate = false;
     private int mProposedImportance = IMPORTANCE_UNSPECIFIED;
     private boolean mSensitiveContent = false;
+    // Whether an app has attempted to cancel this notification after it has been marked as
+    // lifetime extended.
+    private boolean mCanceledAfterLifetimeExtension = false;
 
     public NotificationRecord(Context context, StatusBarNotification sbn,
             NotificationChannel channel) {
@@ -535,6 +538,7 @@ public final class NotificationRecord {
                 + NotificationListenerService.Ranking.importanceToString(mProposedImportance));
         pw.println(prefix + "mIsAppImportanceLocked=" + mIsAppImportanceLocked);
         pw.println(prefix + "mSensitiveContent=" + mSensitiveContent);
+        pw.println(prefix + "mCanceledAfterLifetimeExtension=" + mCanceledAfterLifetimeExtension);
         pw.println(prefix + "mIntercept=" + mIntercept);
         pw.println(prefix + "mHidden==" + mHidden);
         pw.println(prefix + "mGlobalSortKey=" + mGlobalSortKey);
@@ -1620,6 +1624,14 @@ public final class NotificationRecord {
         mPkgAllowedAsConvo = allowedAsConvo;
     }
 
+    public boolean isCanceledAfterLifetimeExtension() {
+        return mCanceledAfterLifetimeExtension;
+    }
+
+    public void setCanceledAfterLifetimeExtension(boolean canceledAfterLifetimeExtension) {
+        mCanceledAfterLifetimeExtension = canceledAfterLifetimeExtension;
+    }
+
     /**
      * Whether this notification is a conversation notification.
      */
diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java
index 5a8de58c14ae..0a52238671cd 100644
--- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java
+++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java
@@ -3047,6 +3047,41 @@ public class NotificationManagerServiceTest extends UiServiceTestCase {
 
     @Test
     @EnableFlags(android.app.Flags.FLAG_LIFETIME_EXTENSION_REFACTOR)
+    public void testMultipleCancelOfLifetimeExtendedSendsOneUpdate() throws Exception {
+        final NotificationRecord notif = generateNotificationRecord(null);
+        notif.getSbn().getNotification().flags =
+                Notification.FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY;
+        mService.addNotification(notif);
+        final StatusBarNotification sbn = notif.getSbn();
+
+        assertThat(mBinderService.getActiveNotifications(sbn.getPackageName()).length).isEqualTo(1);
+        assertThat(mService.getNotificationRecordCount()).isEqualTo(1);
+
+        // Send two cancelations.
+        mBinderService.cancelNotificationWithTag(mPkg, mPkg, sbn.getTag(), sbn.getId(),
+                sbn.getUserId());
+        waitForIdle();
+        mBinderService.cancelNotificationWithTag(mPkg, mPkg, sbn.getTag(), sbn.getId(),
+                sbn.getUserId());
+        waitForIdle();
+
+        assertThat(mBinderService.getActiveNotifications(sbn.getPackageName()).length).isEqualTo(1);
+        assertThat(mService.getNotificationRecordCount()).isEqualTo(1);
+
+        // Checks that only one post update is sent.
+        verify(mWorkerHandler, times(1))
+                .post(any(NotificationManagerService.PostNotificationRunnable.class));
+        ArgumentCaptor<NotificationRecord> captor =
+                ArgumentCaptor.forClass(NotificationRecord.class);
+        verify(mListeners, times(1)).prepareNotifyPostedLocked(captor.capture(), any(),
+                anyBoolean());
+        assertThat(captor.getValue().getNotification().flags
+                & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY).isEqualTo(
+                FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY);
+    }
+
+    @Test
+    @EnableFlags(android.app.Flags.FLAG_LIFETIME_EXTENSION_REFACTOR)
     public void testCancelAllClearsLifetimeExtended() throws Exception {
         final NotificationRecord notif = generateNotificationRecord(
                 mTestNotificationChannel, 1, "group", true);
@@ -6419,12 +6454,31 @@ public class NotificationManagerServiceTest extends UiServiceTestCase {
     @EnableFlags(android.app.Flags.FLAG_LIFETIME_EXTENSION_REFACTOR)
     public void testStats_DirectReplyLifetimeExtendedPostsUpdate() throws Exception {
         final NotificationRecord r = generateNotificationRecord(mTestNotificationChannel);
+        // Marks the notification as having already been lifetime extended and canceled.
         r.getSbn().getNotification().flags |= FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY;
+        r.setCanceledAfterLifetimeExtension(true);
+        r.setPostSilently(true);
         mService.addNotification(r);
 
         mService.mNotificationDelegate.onNotificationDirectReplied(r.getKey());
         waitForIdle();
 
+        // At the moment prepareNotifyPostedLocked is called on the listeners,
+        // verify that FLAG_ONLY_ALERT_ONCE and shouldPostSilently are set, regardless of initial
+        // values.
+        doAnswer(
+                invocation -> {
+                    int flags = ((NotificationRecord) invocation.getArgument(0))
+                            .getSbn().getNotification().flags;
+                    assertThat(flags & FLAG_ONLY_ALERT_ONCE).isEqualTo(FLAG_ONLY_ALERT_ONCE);
+                    boolean shouldPostSilently = ((NotificationRecord) invocation.getArgument(0))
+                            .shouldPostSilently();
+                    assertThat(shouldPostSilently).isTrue();
+                    return null;
+                }
+        ).when(mListeners).prepareNotifyPostedLocked(any(), any(), anyBoolean());
+
+        // Checks that the record gets marked as a direct reply having occurred.
         assertThat(mService.getNotificationRecord(r.getKey()).getStats().hasDirectReplied())
                 .isTrue();
         // Checks that a post update is sent.
@@ -6437,9 +6491,65 @@ public class NotificationManagerServiceTest extends UiServiceTestCase {
         assertThat(captor.getValue().getNotification().flags
                 & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY).isEqualTo(
                 FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY);
+        // FLAG_ONLY_ALERT_ONCE was not present on the original notification, so it's not here.
         assertThat(captor.getValue().getNotification().flags
-                & FLAG_ONLY_ALERT_ONCE).isEqualTo(FLAG_ONLY_ALERT_ONCE);
+                & FLAG_ONLY_ALERT_ONCE).isEqualTo(0);
         assertThat(captor.getValue().shouldPostSilently()).isTrue();
+        assertThat(captor.getValue().isCanceledAfterLifetimeExtension()).isTrue();
+    }
+
+    @Test
+    @EnableFlags(android.app.Flags.FLAG_LIFETIME_EXTENSION_REFACTOR)
+    public void testStats_DirectReplyLifetimeExtendedPostsUpdate_RestorePostSilently()
+            throws Exception {
+        final NotificationRecord r = generateNotificationRecord(mTestNotificationChannel);
+        // Marks the notification as having already been lifetime extended and canceled.
+        r.getSbn().getNotification().flags |= FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY;
+        r.setPostSilently(false);
+        mService.addNotification(r);
+
+        mService.mNotificationDelegate.onNotificationDirectReplied(r.getKey());
+        waitForIdle();
+
+        // Checks that a post update is sent with shouldPostSilently set to true.
+        doAnswer(
+                invocation -> {
+                    boolean shouldPostSilently = ((NotificationRecord) invocation.getArgument(0))
+                            .shouldPostSilently();
+                    assertThat(shouldPostSilently).isTrue();
+                    return null;
+                }
+        ).when(mListeners).prepareNotifyPostedLocked(any(), any(), anyBoolean());
+
+        // Checks that shouldPostSilently is restored to its false state afterward.
+        assertThat(mService.getNotificationRecord(r.getKey()).shouldPostSilently()).isFalse();
+    }
+
+    @Test
+    @EnableFlags(android.app.Flags.FLAG_LIFETIME_EXTENSION_REFACTOR)
+    public void testStats_DirectReplyLifetimeExtendedPostsUpdate_RestoreOnlyAlertOnceFlag()
+            throws Exception {
+        final NotificationRecord r = generateNotificationRecord(mTestNotificationChannel);
+        // Marks the notification as having already been lifetime extended and canceled.
+        r.getSbn().getNotification().flags |= FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY;
+        mService.addNotification(r);
+
+        mService.mNotificationDelegate.onNotificationDirectReplied(r.getKey());
+        waitForIdle();
+
+        // Checks that a post update is sent with FLAG_ONLY_ALERT_ONCE set to true.
+        doAnswer(
+                invocation -> {
+                    int flags = ((NotificationRecord) invocation.getArgument(0))
+                            .getSbn().getNotification().flags;
+                    assertThat(flags & FLAG_ONLY_ALERT_ONCE).isEqualTo(FLAG_ONLY_ALERT_ONCE);
+                    return null;
+                }
+        ).when(mListeners).prepareNotifyPostedLocked(any(), any(), anyBoolean());
+
+        // Checks that the flag is removed afterward.
+        assertThat(mService.getNotificationRecord(r.getKey()).getSbn().getNotification().flags
+                & FLAG_ONLY_ALERT_ONCE).isEqualTo(0);
     }
 
     @Test
@@ -6476,6 +6586,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase {
                 anyBoolean());
         assertThat(captor.getValue().getNotification().flags
                 & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY).isEqualTo(0);
+        assertThat(captor.getValue().isCanceledAfterLifetimeExtension()).isFalse();
         assertThat(captor.getValue()
                 .getNotification().extras.getCharSequence(Notification.EXTRA_TITLE).toString())
                 .isEqualTo("new title");
@@ -9143,11 +9254,13 @@ public class NotificationManagerServiceTest extends UiServiceTestCase {
         final int replyIndex = 2;
         final String reply = "Hello";
         final boolean modifiedBeforeSending = true;
-        final boolean generatedByAssistant = true;
 
         NotificationRecord r = generateNotificationRecord(mTestNotificationChannel);
         r.getSbn().getNotification().flags |= FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY;
-        r.setSuggestionsGeneratedByAssistant(generatedByAssistant);
+        r.getSbn().getNotification().flags |= FLAG_ONLY_ALERT_ONCE;
+        r.setSuggestionsGeneratedByAssistant(true);
+        r.setCanceledAfterLifetimeExtension(true);
+        r.setPostSilently(true);
         mService.addNotification(r);
 
         mService.mNotificationDelegate.onNotificationSmartReplySent(
@@ -9155,6 +9268,21 @@ public class NotificationManagerServiceTest extends UiServiceTestCase {
                 modifiedBeforeSending);
         waitForIdle();
 
+        // At the moment prepareNotifyPostedLocked is called on the listeners,
+        // verify that FLAG_ONLY_ALERT_ONCE and shouldPostSilently are set, regardless of initial
+        // values.
+        doAnswer(
+                invocation -> {
+                    int flags = ((NotificationRecord) invocation.getArgument(0))
+                            .getSbn().getNotification().flags;
+                    assertThat(flags & FLAG_ONLY_ALERT_ONCE).isEqualTo(FLAG_ONLY_ALERT_ONCE);
+                    boolean shouldPostSilently = ((NotificationRecord) invocation.getArgument(0))
+                            .shouldPostSilently();
+                    assertThat(shouldPostSilently).isTrue();
+                    return null;
+                }
+        ).when(mListeners).prepareNotifyPostedLocked(any(), any(), anyBoolean());
+
         // Checks that a post update is sent.
         verify(mWorkerHandler, times(1))
                 .post(any(NotificationManagerService.PostNotificationRunnable.class));
@@ -9165,8 +9293,10 @@ public class NotificationManagerServiceTest extends UiServiceTestCase {
         assertThat(captor.getValue().getNotification().flags
                 & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY).isEqualTo(
                 FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY);
+        // Flag was present before, so it's set afterward
         assertThat(captor.getValue().getNotification().flags
                 & FLAG_ONLY_ALERT_ONCE).isEqualTo(FLAG_ONLY_ALERT_ONCE);
+        // Should post silently was set before, so it's set afterward.
         assertThat(captor.getValue().shouldPostSilently()).isTrue();
     }