Make ForegroundCoordinator single-threaded

The appops callback is frequently called from a background thread, which
was causing a crash when calling mNotifPipeline.getActiveNotifs().
Switched the class over to being exclusively single-threaded; it didn't
need to support multiple threads and this simplifies things.

Test: atest SystemUITests:ForegroundCoordinatorTest
Change-Id: I4f010c16fa3f2467a5aaa05ef72c0278396a30e0

2 files changed, 142 insertions, 39 deletions

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ForegroundCoordinator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ForegroundCoordinator.java
index da119c1502c6..854444fc8bb7 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ForegroundCoordinator.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ForegroundCoordinator.java
@@ -17,7 +17,6 @@
 package com.android.systemui.statusbar.notification.collection.coordinator;
 
 import android.app.Notification;
-import android.os.Handler;
 import android.os.UserHandle;
 import android.service.notification.StatusBarNotification;
 import android.util.ArraySet;
@@ -30,6 +29,8 @@ import com.android.systemui.statusbar.notification.collection.NotificationEntry;
 import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender;
+import com.android.systemui.util.Assert;
+import com.android.systemui.util.concurrency.DelayableExecutor;
 
 import java.util.HashMap;
 import java.util.Map;
@@ -47,6 +48,8 @@ import javax.inject.Singleton;
  *  frameworks/base/packages/SystemUI/src/com/android/systemui/ForegroundServiceController
  *  frameworks/base/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener
  *  frameworks/base/packages/SystemUI/src/com/android/systemui/ForegroundServiceLifetimeExtender
+ *
+ *  TODO: AppOps stuff should be spun off into its own coordinator
  */
 @Singleton
 public class ForegroundCoordinator implements Coordinator {
@@ -54,7 +57,7 @@ public class ForegroundCoordinator implements Coordinator {
 
     private final ForegroundServiceController mForegroundServiceController;
     private final AppOpsController mAppOpsController;
-    private final Handler mMainHandler;
+    private final DelayableExecutor mMainExecutor;
 
     private NotifPipeline mNotifPipeline;
 
@@ -62,10 +65,10 @@ public class ForegroundCoordinator implements Coordinator {
     public ForegroundCoordinator(
             ForegroundServiceController foregroundServiceController,
             AppOpsController appOpsController,
-            @Main Handler mainHandler) {
+            @Main DelayableExecutor mainExecutor) {
         mForegroundServiceController = foregroundServiceController;
         mAppOpsController = appOpsController;
-        mMainHandler = mainHandler;
+        mMainExecutor = mainExecutor;
     }
 
     @Override
@@ -78,11 +81,11 @@ public class ForegroundCoordinator implements Coordinator {
         // listen for new notifications to add appOps
         mNotifPipeline.addCollectionListener(mNotifCollectionListener);
 
-        // when appOps change, update any relevant notifications to update appOps for
-        mAppOpsController.addCallback(ForegroundServiceController.APP_OPS, this::onAppOpsChanged);
-
         // filter out foreground service notifications that aren't necessary anymore
         mNotifPipeline.addPreGroupFilter(mNotifFilter);
+
+        // when appOps change, update any relevant notifications to update appOps for
+        mAppOpsController.addCallback(ForegroundServiceController.APP_OPS, this::onAppOpsChanged);
     }
 
     /**
@@ -93,7 +96,8 @@ public class ForegroundCoordinator implements Coordinator {
         public boolean shouldFilterOut(NotificationEntry entry, long now) {
             StatusBarNotification sbn = entry.getSbn();
             if (mForegroundServiceController.isDisclosureNotification(sbn)
-                    && !mForegroundServiceController.isDisclosureNeededForUser(sbn.getUserId())) {
+                    && !mForegroundServiceController.isDisclosureNeededForUser(
+                            sbn.getUser().getIdentifier())) {
                 return true;
             }
 
@@ -102,7 +106,7 @@ public class ForegroundCoordinator implements Coordinator {
                         Notification.EXTRA_FOREGROUND_APPS);
                 if (apps != null && apps.length >= 1) {
                     if (!mForegroundServiceController.isSystemAlertWarningNeeded(
-                            sbn.getUserId(), apps[0])) {
+                            sbn.getUser().getIdentifier(), apps[0])) {
                         return true;
                     }
                 }
@@ -119,7 +123,7 @@ public class ForegroundCoordinator implements Coordinator {
             new NotifLifetimeExtender() {
         private static final int MIN_FGS_TIME_MS = 5000;
         private OnEndLifetimeExtensionCallback mEndCallback;
-        private Map<String, Runnable> mEndRunnables = new HashMap<>();
+        private Map<NotificationEntry, Runnable> mEndRunnables = new HashMap<>();
 
         @Override
         public String getName() {
@@ -142,16 +146,18 @@ public class ForegroundCoordinator implements Coordinator {
             final boolean extendLife = currTime - entry.getSbn().getPostTime() < MIN_FGS_TIME_MS;
 
             if (extendLife) {
-                if (!mEndRunnables.containsKey(entry.getKey())) {
-                    final Runnable runnable = new Runnable() {
-                        @Override
-                        public void run() {
-                            mEndCallback.onEndLifetimeExtension(mForegroundLifetimeExtender, entry);
-                        }
+                if (!mEndRunnables.containsKey(entry)) {
+                    final Runnable endExtensionRunnable = () -> {
+                        mEndRunnables.remove(entry);
+                        mEndCallback.onEndLifetimeExtension(
+                                mForegroundLifetimeExtender,
+                                entry);
                     };
-                    mEndRunnables.put(entry.getKey(), runnable);
-                    mMainHandler.postDelayed(runnable,
+
+                    final Runnable cancelRunnable = mMainExecutor.executeDelayed(
+                            endExtensionRunnable,
                             MIN_FGS_TIME_MS - (currTime - entry.getSbn().getPostTime()));
+                    mEndRunnables.put(entry, cancelRunnable);
                 }
             }
 
@@ -160,9 +166,9 @@ public class ForegroundCoordinator implements Coordinator {
 
         @Override
         public void cancelLifetimeExtension(NotificationEntry entry) {
-            if (mEndRunnables.containsKey(entry.getKey())) {
-                Runnable endRunnable = mEndRunnables.remove(entry.getKey());
-                mMainHandler.removeCallbacks(endRunnable);
+            Runnable cancelRunnable = mEndRunnables.remove(entry);
+            if (cancelRunnable != null) {
+                cancelRunnable.run();
             }
         }
     };
@@ -184,25 +190,32 @@ public class ForegroundCoordinator implements Coordinator {
         private void tagForeground(NotificationEntry entry) {
             final StatusBarNotification sbn = entry.getSbn();
             // note: requires that the ForegroundServiceController is updating their appOps first
-            ArraySet<Integer> activeOps = mForegroundServiceController.getAppOps(sbn.getUserId(),
-                    sbn.getPackageName());
+            ArraySet<Integer> activeOps =
+                    mForegroundServiceController.getAppOps(
+                            sbn.getUser().getIdentifier(),
+                            sbn.getPackageName());
             if (activeOps != null) {
-                synchronized (entry.mActiveAppOps) {
-                    entry.mActiveAppOps.clear();
-                    entry.mActiveAppOps.addAll(activeOps);
-                }
+                entry.mActiveAppOps.clear();
+                entry.mActiveAppOps.addAll(activeOps);
             }
         }
     };
 
+    private void onAppOpsChanged(int code, int uid, String packageName, boolean active) {
+        mMainExecutor.execute(() -> handleAppOpsChanged(code, uid, packageName, active));
+    }
+
     /**
      * Update the appOp for the posted notification associated with the current foreground service
+     *
      * @param code code for appOp to add/remove
      * @param uid of user the notification is sent to
      * @param packageName package that created the notification
      * @param active whether the appOpCode is active or not
      */
-    private void onAppOpsChanged(int code, int uid, String packageName, boolean active) {
+    private void handleAppOpsChanged(int code, int uid, String packageName, boolean active) {
+        Assert.isMainThread();
+
         int userId = UserHandle.getUserId(uid);
 
         // Update appOp if there's an associated posted notification:
@@ -214,15 +227,13 @@ public class ForegroundCoordinator implements Coordinator {
                     && uid == entry.getSbn().getUid()
                     && packageName.equals(entry.getSbn().getPackageName())) {
                 boolean changed;
-                synchronized (entry.mActiveAppOps) {
-                    if (active) {
-                        changed = entry.mActiveAppOps.add(code);
-                    } else {
-                        changed = entry.mActiveAppOps.remove(code);
-                    }
+                if (active) {
+                    changed = entry.mActiveAppOps.add(code);
+                } else {
+                    changed = entry.mActiveAppOps.remove(code);
                 }
                 if (changed) {
-                    mMainHandler.post(mNotifFilter::invalidateList);
+                    mNotifFilter.invalidateList();
                 }
             }
         }
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ForegroundCoordinatorTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ForegroundCoordinatorTest.java
index 6cc8dd908760..eb1af7c82324 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ForegroundCoordinatorTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ForegroundCoordinatorTest.java
@@ -16,20 +16,23 @@
 
 package com.android.systemui.statusbar.notification.collection.coordinator;
 
+import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertFalse;
 import static junit.framework.Assert.assertTrue;
 
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import android.app.Notification;
 import android.os.Bundle;
-import android.os.Handler;
 import android.os.UserHandle;
 import android.service.notification.NotificationListenerService;
 import android.service.notification.StatusBarNotification;
 import android.testing.AndroidTestingRunner;
+import android.testing.TestableLooper;
+import android.util.ArraySet;
 
 import androidx.test.filters.SmallTest;
 
@@ -41,36 +44,53 @@ import com.android.systemui.statusbar.notification.collection.NotificationEntry;
 import com.android.systemui.statusbar.notification.collection.NotificationEntryBuilder;
 import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender;
+import com.android.systemui.util.Assert;
+import com.android.systemui.util.concurrency.FakeExecutor;
+import com.android.systemui.util.time.FakeSystemClock;
 
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 
+import java.util.List;
+
 @SmallTest
 @RunWith(AndroidTestingRunner.class)
+@TestableLooper.RunWithLooper
 public class ForegroundCoordinatorTest extends SysuiTestCase {
     private static final String TEST_PKG = "test_pkg";
     private static final int NOTIF_USER_ID = 0;
 
-    @Mock private Handler mMainHandler;
     @Mock private ForegroundServiceController mForegroundServiceController;
     @Mock private AppOpsController mAppOpsController;
     @Mock private NotifPipeline mNotifPipeline;
 
+    @Captor private ArgumentCaptor<AppOpsController.Callback> mAppOpsCaptor;
+
     private NotificationEntry mEntry;
     private Notification mNotification;
     private ForegroundCoordinator mForegroundCoordinator;
     private NotifFilter mForegroundFilter;
+    private AppOpsController.Callback mAppOpsCallback;
     private NotifLifetimeExtender mForegroundNotifLifetimeExtender;
 
+    private FakeSystemClock mClock = new FakeSystemClock();
+    private FakeExecutor mExecutor = new FakeExecutor(mClock);
+
     @Before
     public void setup() {
         MockitoAnnotations.initMocks(this);
-        mForegroundCoordinator = new ForegroundCoordinator(
-                mForegroundServiceController, mAppOpsController, mMainHandler);
+        Assert.sMainLooper = TestableLooper.get(this).getLooper();
+
+        mForegroundCoordinator =
+                new ForegroundCoordinator(
+                        mForegroundServiceController,
+                        mAppOpsController,
+                        mExecutor);
 
         mNotification = new Notification();
         mEntry = new NotificationEntryBuilder()
@@ -86,9 +106,11 @@ public class ForegroundCoordinatorTest extends SysuiTestCase {
         verify(mNotifPipeline, times(1)).addPreGroupFilter(filterCaptor.capture());
         verify(mNotifPipeline, times(1)).addNotificationLifetimeExtender(
                 lifetimeExtenderCaptor.capture());
+        verify(mAppOpsController).addCallback(any(int[].class), mAppOpsCaptor.capture());
 
         mForegroundFilter = filterCaptor.getValue();
         mForegroundNotifLifetimeExtender = lifetimeExtenderCaptor.getValue();
+        mAppOpsCallback = mAppOpsCaptor.getValue();
     }
 
     @Test
@@ -183,4 +205,74 @@ public class ForegroundCoordinatorTest extends SysuiTestCase {
         assertFalse(mForegroundNotifLifetimeExtender
                 .shouldExtendLifetime(mEntry, NotificationListenerService.REASON_CLICK));
     }
+
+    @Test
+    public void testAppOpsAreApplied() {
+        // GIVEN Three current notifications, two with the same key but from different users
+        NotificationEntry entry1 = new NotificationEntryBuilder()
+                .setUser(new UserHandle(NOTIF_USER_ID))
+                .setPkg(TEST_PKG)
+                .setId(1)
+                .build();
+        NotificationEntry entry2 = new NotificationEntryBuilder()
+                .setUser(new UserHandle(NOTIF_USER_ID))
+                .setPkg(TEST_PKG)
+                .setId(2)
+                .build();
+        NotificationEntry entry2Other = new NotificationEntryBuilder()
+                .setUser(new UserHandle(NOTIF_USER_ID + 1))
+                .setPkg(TEST_PKG)
+                .setId(2)
+                .build();
+        when(mNotifPipeline.getActiveNotifs()).thenReturn(List.of(entry1, entry2, entry2Other));
+
+        // GIVEN that entry2 is currently associated with a foreground service
+        when(mForegroundServiceController.getStandardLayoutKey(0, TEST_PKG))
+                .thenReturn(entry2.getKey());
+
+        // WHEN a new app ops code comes in
+        mAppOpsCallback.onActiveStateChanged(47, NOTIF_USER_ID, TEST_PKG, true);
+        mExecutor.runAllReady();
+
+        // THEN entry2's app ops are updated, but no one else's are
+        assertEquals(
+                new ArraySet<>(),
+                entry1.mActiveAppOps);
+        assertEquals(
+                new ArraySet<>(List.of(47)),
+                entry2.mActiveAppOps);
+        assertEquals(
+                new ArraySet<>(),
+                entry2Other.mActiveAppOps);
+    }
+
+    @Test
+    public void testAppOpsAreRemoved() {
+        // GIVEN One notification which is associated with app ops
+        NotificationEntry entry = new NotificationEntryBuilder()
+                .setUser(new UserHandle(NOTIF_USER_ID))
+                .setPkg(TEST_PKG)
+                .setId(2)
+                .build();
+        when(mNotifPipeline.getActiveNotifs()).thenReturn(List.of(entry));
+        when(mForegroundServiceController.getStandardLayoutKey(0, TEST_PKG))
+                .thenReturn(entry.getKey());
+
+        // GIVEN that the notification's app ops are already [47, 33]
+        mAppOpsCallback.onActiveStateChanged(47, NOTIF_USER_ID, TEST_PKG, true);
+        mAppOpsCallback.onActiveStateChanged(33, NOTIF_USER_ID, TEST_PKG, true);
+        mExecutor.runAllReady();
+        assertEquals(
+                new ArraySet<>(List.of(47, 33)),
+                entry.mActiveAppOps);
+
+        // WHEN one of the app ops is removed
+        mAppOpsCallback.onActiveStateChanged(47, NOTIF_USER_ID, TEST_PKG, false);
+        mExecutor.runAllReady();
+
+        // THEN the entry's active app ops are updated as well
+        assertEquals(
+                new ArraySet<>(List.of(33)),
+                entry.mActiveAppOps);
+    }
 }