Merge "Don't throw exceptions in NotifCollection when initializing" into rvc-dev

6 files changed, 110 insertions, 8 deletions

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java
index 72d9d0ee8f8f..4d09071c6b5d 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java
@@ -104,6 +104,9 @@ public class NotificationListener extends NotificationListenerWithPlugins {
                     listener.onNotificationPosted(sbn, completeMap);
                 }
             }
+            for (NotificationHandler listener : mNotificationHandlers) {
+                listener.onNotificationsInitialized();
+            }
         });
         onSilentStatusBarIconsVisibilityChanged(
                 mNotificationManager.shouldHideSilentStatusBarIcons());
@@ -224,5 +227,10 @@ public class NotificationListener extends NotificationListenerWithPlugins {
         void onNotificationRemoved(StatusBarNotification sbn, RankingMap rankingMap);
         void onNotificationRemoved(StatusBarNotification sbn, RankingMap rankingMap, int reason);
         void onNotificationRankingUpdate(RankingMap rankingMap);
+
+        /**
+         * Called after the listener has connected to NoMan and posted any current notifications.
+         */
+        void onNotificationsInitialized();
     }
 }
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java
index adb51a5d959a..9abc66056452 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java
@@ -385,6 +385,10 @@ public class NotificationEntryManager implements
         public void onNotificationRankingUpdate(RankingMap rankingMap) {
             updateNotificationRanking(rankingMap);
         }
+
+        @Override
+        public void onNotificationsInitialized() {
+        }
     };
 
     /**
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 45987b6eb21b..c1acfbadef45 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
@@ -47,7 +47,6 @@ import android.annotation.Nullable;
 import android.annotation.UserIdInt;
 import android.app.Notification;
 import android.os.RemoteException;
-import android.os.SystemClock;
 import android.os.UserHandle;
 import android.service.notification.NotificationListenerService;
 import android.service.notification.NotificationListenerService.Ranking;
@@ -82,6 +81,7 @@ import com.android.systemui.statusbar.notification.collection.notifcollection.No
 import com.android.systemui.statusbar.notification.collection.notifcollection.RankingAppliedEvent;
 import com.android.systemui.statusbar.notification.collection.notifcollection.RankingUpdatedEvent;
 import com.android.systemui.util.Assert;
+import com.android.systemui.util.time.SystemClock;
 
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
@@ -95,6 +95,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Queue;
+import java.util.concurrent.TimeUnit;
 
 import javax.inject.Inject;
 import javax.inject.Singleton;
@@ -125,6 +126,7 @@ import javax.inject.Singleton;
 @Singleton
 public class NotifCollection implements Dumpable {
     private final IStatusBarService mStatusBarService;
+    private final SystemClock mClock;
     private final FeatureFlags mFeatureFlags;
     private final NotifCollectionLogger mLogger;
     private final LogBufferEulogizer mEulogizer;
@@ -142,20 +144,24 @@ public class NotifCollection implements Dumpable {
 
     private boolean mAttached = false;
     private boolean mAmDispatchingToOtherCode;
+    private long mInitializedTimestamp = 0;
 
     @Inject
     public NotifCollection(
             IStatusBarService statusBarService,
-            DumpManager dumpManager,
+            SystemClock clock,
             FeatureFlags featureFlags,
             NotifCollectionLogger logger,
-            LogBufferEulogizer logBufferEulogizer) {
+            LogBufferEulogizer logBufferEulogizer,
+            DumpManager dumpManager) {
         Assert.isMainThread();
         mStatusBarService = statusBarService;
+        mClock = clock;
+        mFeatureFlags = featureFlags;
         mLogger = logger;
         mEulogizer = logBufferEulogizer;
+
         dumpManager.registerDumpable(TAG, this);
-        mFeatureFlags = featureFlags;
     }
 
     /** Initializes the NotifCollection and registers it to receive notification events. */
@@ -376,9 +382,10 @@ public class NotifCollection implements Dumpable {
 
         final NotificationEntry entry = mNotificationSet.get(sbn.getKey());
         if (entry == null) {
-            throw mEulogizer.record(
+            crashIfNotInitializing(
                     new IllegalStateException("No notification to remove with key "
                             + sbn.getKey()));
+            return;
         }
 
         entry.mCancellationReason = reason;
@@ -394,6 +401,10 @@ public class NotifCollection implements Dumpable {
         dispatchEventsAndRebuildList();
     }
 
+    private void onNotificationsInitialized() {
+        mInitializedTimestamp = mClock.uptimeMillis();
+    }
+
     private void postNotification(
             StatusBarNotification sbn,
             Ranking ranking) {
@@ -401,7 +412,7 @@ public class NotifCollection implements Dumpable {
 
         if (entry == null) {
             // A new notification!
-            entry = new NotificationEntry(sbn, ranking, SystemClock.uptimeMillis());
+            entry = new NotificationEntry(sbn, ranking, mClock.uptimeMillis());
             mEventQueue.add(new InitEntryEvent(entry));
             mEventQueue.add(new BindEntryEvent(entry, sbn));
             mNotificationSet.put(sbn.getKey(), entry);
@@ -628,6 +639,23 @@ public class NotifCollection implements Dumpable {
         }
     }
 
+    // While the NotificationListener is connecting to NotificationManager, there is a short period
+    // during which it's possible for us to receive events about notifications we don't yet know
+    // about (or that otherwise don't make sense). Until that race condition is fixed, we create a
+    // "forgiveness window" of five seconds during which we won't crash if we receive nonsensical
+    // messages from system server.
+    private void crashIfNotInitializing(RuntimeException exception) {
+        final boolean isRecentlyInitialized = mInitializedTimestamp == 0
+                || mClock.uptimeMillis() - mInitializedTimestamp
+                        < INITIALIZATION_FORGIVENESS_WINDOW;
+
+        if (isRecentlyInitialized) {
+            mLogger.logIgnoredError(exception.getMessage());
+        } else {
+            throw mEulogizer.record(exception);
+        }
+    }
+
     private static Ranking requireRanking(RankingMap rankingMap, String key) {
         // TODO: Modify RankingMap so that we don't have to make a copy here
         Ranking ranking = new Ranking();
@@ -742,6 +770,11 @@ public class NotifCollection implements Dumpable {
         public void onNotificationRankingUpdate(RankingMap rankingMap) {
             NotifCollection.this.onNotificationRankingUpdate(rankingMap);
         }
+
+        @Override
+        public void onNotificationsInitialized() {
+            NotifCollection.this.onNotificationsInitialized();
+        }
     };
 
     private static final String TAG = "NotifCollection";
@@ -773,4 +806,6 @@ public class NotifCollection implements Dumpable {
 
     static final int REASON_NOT_CANCELED = -1;
     public static final int REASON_UNKNOWN = 0;
+
+    private static final long INITIALIZATION_FORGIVENESS_WINDOW = TimeUnit.SECONDS.toMillis(5);
 }
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coalescer/GroupCoalescer.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coalescer/GroupCoalescer.java
index 596235cfb4d0..1710daa16735 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coalescer/GroupCoalescer.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coalescer/GroupCoalescer.java
@@ -153,6 +153,11 @@ public class GroupCoalescer implements Dumpable {
             applyRanking(rankingMap);
             mHandler.onNotificationRankingUpdate(rankingMap);
         }
+
+        @Override
+        public void onNotificationsInitialized() {
+            mHandler.onNotificationsInitialized();
+        }
     };
 
     private void maybeEmitBatch(StatusBarNotification sbn) {
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 0eb2d64e8682..76751eaaecb1 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
@@ -20,6 +20,7 @@ import android.os.RemoteException
 import android.service.notification.NotificationListenerService.RankingMap
 import com.android.systemui.log.LogBuffer
 import com.android.systemui.log.LogLevel.DEBUG
+import com.android.systemui.log.LogLevel.ERROR
 import com.android.systemui.log.LogLevel.INFO
 import com.android.systemui.log.LogLevel.WARNING
 import com.android.systemui.log.LogLevel.WTF
@@ -167,6 +168,14 @@ class NotifCollectionLogger @Inject constructor(
             "LIFETIME EXTENSION ENDED for $str1 by '$str2'; $int1 remaining extensions"
         })
     }
+
+    fun logIgnoredError(message: String?) {
+        buffer.log(TAG, ERROR, {
+            str1 = message
+        }, {
+            "ERROR suppressed due to initialization forgiveness: $str1"
+        })
+    }
 }
 
 private const val TAG = "NotifCollection"
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 ca9cc299b36d..363fe95aae18 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
@@ -80,6 +80,7 @@ import com.android.systemui.statusbar.notification.collection.notifcollection.No
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLogger;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifDismissInterceptor;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender;
+import com.android.systemui.util.time.FakeSystemClock;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -131,6 +132,7 @@ public class NotifCollectionTest extends SysuiTestCase {
     private InOrder mListenerInOrder;
 
     private NoManSimulator mNoMan;
+    private FakeSystemClock mClock = new FakeSystemClock();
 
     @Before
     public void setUp() {
@@ -146,10 +148,11 @@ public class NotifCollectionTest extends SysuiTestCase {
 
         mCollection = new NotifCollection(
                 mStatusBarService,
-                mock(DumpManager.class),
+                mClock,
                 mFeatureFlags,
                 mLogger,
-                mEulogizer);
+                mEulogizer,
+                mock(DumpManager.class));
         mCollection.attach(mGroupCoalescer);
         mCollection.addCollectionListener(mCollectionListener);
         mCollection.setBuildListener(mBuildListener);
@@ -161,6 +164,8 @@ public class NotifCollectionTest extends SysuiTestCase {
 
         mNoMan = new NoManSimulator();
         mNoMan.addListener(mNotifHandler);
+
+        mNotifHandler.onNotificationsInitialized();
     }
 
     @Test
@@ -1268,6 +1273,42 @@ public class NotifCollectionTest extends SysuiTestCase {
         verify(mInterceptor3, never()).shouldInterceptDismissal(clearable);
     }
 
+    @Test(expected = IllegalStateException.class)
+    public void testClearNotificationThrowsIfMissing() {
+        // GIVEN that enough time has passed that we're beyond the forgiveness window
+        mClock.advanceTime(5001);
+
+        // WHEN we get a remove event for a notification we don't know about
+        final NotificationEntry container = new NotificationEntryBuilder()
+                .setPkg(TEST_PACKAGE)
+                .setId(47)
+                .build();
+        mNotifHandler.onNotificationRemoved(
+                container.getSbn(),
+                new RankingMap(new Ranking[]{ container.getRanking() }));
+
+        // THEN an exception is thrown
+    }
+
+    @Test
+    public void testClearNotificationDoesntThrowIfInForgivenessWindow() {
+        // GIVEN that some time has passed but we're still within the initialization forgiveness
+        // window
+        mClock.advanceTime(4999);
+
+        // WHEN we get a remove event for a notification we don't know about
+        final NotificationEntry container = new NotificationEntryBuilder()
+                .setPkg(TEST_PACKAGE)
+                .setId(47)
+                .build();
+        mNotifHandler.onNotificationRemoved(
+                container.getSbn(),
+                new RankingMap(new Ranking[]{ container.getRanking() }));
+
+        // THEN no exception is thrown, but no event is fired
+        verify(mCollectionListener, never()).onEntryRemoved(any(NotificationEntry.class), anyInt());
+    }
+
     private static NotificationEntryBuilder buildNotif(String pkg, int id, String tag) {
         return new NotificationEntryBuilder()
                 .setPkg(pkg)