Prevent sending early termination of appop use

If a package starts a particular appop both as active and noted, once
one of them is finished (usually noted after 5s), a message will be sent
to callbacks indicating the end of the use. However, the app op may
still be active. This could result in the removal of indicators
prematurely from notifications.

This change prevents that from happening by checking if the app op is
still in use by that combination uid/package (either active or noted)
and not notifying listeners if that's the case.

Test: atest AppOpsControllerTest
Test: use app from bug report. Observe that notification does not lose
the microphone indicator
Bug: 144092031
Merged-In: I180e7c257e6171e7686ba7eda9bf02249358ed0

Change-Id: I94473c3ccf1318dac29f067dade91e540e20285e

2 files changed, 179 insertions, 10 deletions

diff --git a/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java b/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java
index afb8e7421412..bfdda207b5b4 100644
--- a/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java
+++ b/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java
@@ -193,20 +193,32 @@ public class AppOpsControllerImpl implements AppOpsController,
             mNotedItems.remove(item);
             if (DEBUG) Log.w(TAG, "Removed item: " + item.toString());
         }
-        notifySuscribers(code, uid, packageName, false);
+        boolean active;
+        // Check if the item is also active
+        synchronized (mActiveItems) {
+            active = getAppOpItem(mActiveItems, code, uid, packageName) != null;
+        }
+        if (!active) {
+            notifySuscribers(code, uid, packageName, false);
+        }
     }
 
-    private void addNoted(int code, int uid, String packageName) {
+    private boolean addNoted(int code, int uid, String packageName) {
         AppOpItem item;
+        boolean createdNew = false;
         synchronized (mNotedItems) {
             item = getAppOpItem(mNotedItems, code, uid, packageName);
             if (item == null) {
                 item = new AppOpItem(code, uid, packageName, System.currentTimeMillis());
                 mNotedItems.add(item);
                 if (DEBUG) Log.w(TAG, "Added item: " + item.toString());
+                createdNew = true;
             }
         }
+        // We should keep this so we make sure it cannot time out.
+        mBGHandler.removeCallbacksAndMessages(item);
         mBGHandler.scheduleRemoval(item, NOTED_OP_TIME_DELAY_MS);
+        return createdNew;
     }
 
     /**
@@ -253,23 +265,46 @@ public class AppOpsControllerImpl implements AppOpsController,
 
     @Override
     public void onOpActiveChanged(int code, int uid, String packageName, boolean active) {
-        if (updateActives(code, uid, packageName, active)) {
-            notifySuscribers(code, uid, packageName, active);
+        if (DEBUG) {
+            Log.w(TAG, String.format("onActiveChanged(%d,%d,%s,%s", code, uid, packageName,
+                    Boolean.toString(active)));
+        }
+        boolean activeChanged = updateActives(code, uid, packageName, active);
+        if (!activeChanged) return; // early return
+        // Check if the item is also noted, in that case, there's no update.
+        boolean alsoNoted;
+        synchronized (mNotedItems) {
+            alsoNoted = getAppOpItem(mNotedItems, code, uid, packageName) != null;
+        }
+        // If active is true, we only send the update if the op is not actively noted (already true)
+        // If active is false, we only send the update if the op is not actively noted (prevent
+        // early removal)
+        if (!alsoNoted) {
+            mBGHandler.post(() -> notifySuscribers(code, uid, packageName, active));
         }
     }
 
     @Override
     public void onOpNoted(int code, int uid, String packageName, int result) {
         if (DEBUG) {
-            Log.w(TAG, "Op: " + code + " with result " + AppOpsManager.MODE_NAMES[result]);
+            Log.w(TAG, "Noted op: " + code + " with result "
+                    + AppOpsManager.MODE_NAMES[result] + " for package " + packageName);
         }
         if (result != AppOpsManager.MODE_ALLOWED) return;
-        addNoted(code, uid, packageName);
-        notifySuscribers(code, uid, packageName, true);
+        boolean notedAdded = addNoted(code, uid, packageName);
+        if (!notedAdded) return; // early return
+        boolean alsoActive;
+        synchronized (mActiveItems) {
+            alsoActive = getAppOpItem(mActiveItems, code, uid, packageName) != null;
+        }
+        if (!alsoActive) {
+            mBGHandler.post(() -> notifySuscribers(code, uid, packageName, true));
+        }
     }
 
     private void notifySuscribers(int code, int uid, String packageName, boolean active) {
         if (mCallbacksByCode.containsKey(code)) {
+            if (DEBUG) Log.d(TAG, "Notifying of change in package " + packageName);
             for (Callback cb: mCallbacksByCode.get(code)) {
                 cb.onActiveStateChanged(code, uid, packageName, active);
             }
@@ -292,7 +327,7 @@ public class AppOpsControllerImpl implements AppOpsController,
 
     }
 
-    protected final class H extends Handler {
+    protected class H extends Handler {
         H(Looper looper) {
             super(looper);
         }
diff --git a/packages/SystemUI/tests/src/com/android/systemui/appops/AppOpsControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/appops/AppOpsControllerTest.java
index bd7f897dc1c0..df7221e2f663 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/appops/AppOpsControllerTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/appops/AppOpsControllerTest.java
@@ -29,6 +29,8 @@ import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
+import static java.lang.Thread.sleep;
+
 import android.app.AppOpsManager;
 import android.content.pm.PackageManager;
 import android.os.UserHandle;
@@ -37,7 +39,6 @@ import android.testing.TestableLooper;
 
 import androidx.test.filters.SmallTest;
 
-import com.android.systemui.Dependency;
 import com.android.systemui.SysuiTestCase;
 
 import org.junit.Before;
@@ -46,6 +47,8 @@ import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 
+import java.util.List;
+
 @SmallTest
 @RunWith(AndroidTestingRunner.class)
 @TestableLooper.RunWithLooper
@@ -64,14 +67,16 @@ public class AppOpsControllerTest extends SysuiTestCase {
     private AppOpsControllerImpl.H mMockHandler;
 
     private AppOpsControllerImpl mController;
+    private TestableLooper mTestableLooper;
 
     @Before
     public void setUp() {
         MockitoAnnotations.initMocks(this);
+        mTestableLooper = TestableLooper.get(this);
 
         getContext().addMockSystemService(AppOpsManager.class, mAppOpsManager);
 
-        mController = new AppOpsControllerImpl(mContext, Dependency.get(Dependency.BG_LOOPER));
+        mController = new AppOpsControllerImpl(mContext, mTestableLooper.getLooper());
     }
 
     @Test
@@ -95,6 +100,7 @@ public class AppOpsControllerTest extends SysuiTestCase {
                 AppOpsManager.OP_RECORD_AUDIO, TEST_UID, TEST_PACKAGE_NAME, true);
         mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
                 AppOpsManager.MODE_ALLOWED);
+        mTestableLooper.processAllMessages();
         verify(mCallback).onActiveStateChanged(AppOpsManager.OP_RECORD_AUDIO,
                 TEST_UID, TEST_PACKAGE_NAME, true);
     }
@@ -104,6 +110,7 @@ public class AppOpsControllerTest extends SysuiTestCase {
         mController.addCallback(new int[]{AppOpsManager.OP_FINE_LOCATION}, mCallback);
         mController.onOpActiveChanged(
                 AppOpsManager.OP_RECORD_AUDIO, TEST_UID, TEST_PACKAGE_NAME, true);
+        mTestableLooper.processAllMessages();
         verify(mCallback, never()).onActiveStateChanged(
                 anyInt(), anyInt(), anyString(), anyBoolean());
     }
@@ -114,6 +121,7 @@ public class AppOpsControllerTest extends SysuiTestCase {
         mController.removeCallback(new int[]{AppOpsManager.OP_RECORD_AUDIO}, mCallback);
         mController.onOpActiveChanged(
                 AppOpsManager.OP_RECORD_AUDIO, TEST_UID, TEST_PACKAGE_NAME, true);
+        mTestableLooper.processAllMessages();
         verify(mCallback, never()).onActiveStateChanged(
                 anyInt(), anyInt(), anyString(), anyBoolean());
     }
@@ -124,6 +132,7 @@ public class AppOpsControllerTest extends SysuiTestCase {
         mController.removeCallback(new int[]{AppOpsManager.OP_CAMERA}, mCallback);
         mController.onOpActiveChanged(
                 AppOpsManager.OP_RECORD_AUDIO, TEST_UID, TEST_PACKAGE_NAME, true);
+        mTestableLooper.processAllMessages();
         verify(mCallback).onActiveStateChanged(AppOpsManager.OP_RECORD_AUDIO,
                 TEST_UID, TEST_PACKAGE_NAME, true);
     }
@@ -186,4 +195,129 @@ public class AppOpsControllerTest extends SysuiTestCase {
         verify(mMockHandler).removeCallbacksAndMessages(null);
         assertTrue(mController.getActiveAppOps().isEmpty());
     }
+
+    @Test
+    public void noDoubleUpdateOnOpNoted() {
+        mController.setBGHandler(mMockHandler);
+
+        mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
+                AppOpsManager.MODE_ALLOWED);
+        mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
+                AppOpsManager.MODE_ALLOWED);
+
+        // Only one post to notify subscribers
+        verify(mMockHandler, times(1)).post(any());
+
+        List<AppOpItem> list = mController.getActiveAppOps();
+        assertEquals(1, list.size());
+    }
+
+    @Test
+    public void onDoubleOPNoted_scheduleTwiceForRemoval() {
+        mController.setBGHandler(mMockHandler);
+
+        mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
+                AppOpsManager.MODE_ALLOWED);
+        mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
+                AppOpsManager.MODE_ALLOWED);
+
+        // Only one post to notify subscribers
+        verify(mMockHandler, times(2)).scheduleRemoval(any(), anyLong());
+    }
+
+    @Test
+    public void testActiveOpNotRemovedAfterNoted() throws InterruptedException {
+        // Replaces the timeout delay with 5 ms
+        AppOpsControllerImpl.H testHandler = mController.new H(mTestableLooper.getLooper()) {
+            @Override
+            public void scheduleRemoval(AppOpItem item, long timeToRemoval) {
+                super.scheduleRemoval(item, 5L);
+            }
+        };
+
+        mController.addCallback(new int[]{AppOpsManager.OP_FINE_LOCATION}, mCallback);
+        mController.setBGHandler(testHandler);
+
+        mController.onOpActiveChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true);
+
+        mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
+                AppOpsManager.MODE_ALLOWED);
+
+        mTestableLooper.processAllMessages();
+        List<AppOpItem> list = mController.getActiveAppOps();
+        verify(mCallback).onActiveStateChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true);
+
+        // Duplicates are not removed between active and noted
+        assertEquals(2, list.size());
+
+        sleep(10L);
+
+        mTestableLooper.processAllMessages();
+
+        verify(mCallback, never()).onActiveStateChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, false);
+        list = mController.getActiveAppOps();
+        assertEquals(1, list.size());
+    }
+
+    @Test
+    public void testNotedNotRemovedAfterActive() {
+        mController.addCallback(new int[]{AppOpsManager.OP_FINE_LOCATION}, mCallback);
+
+        mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
+                AppOpsManager.MODE_ALLOWED);
+
+        mController.onOpActiveChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true);
+
+        mTestableLooper.processAllMessages();
+        List<AppOpItem> list = mController.getActiveAppOps();
+        verify(mCallback).onActiveStateChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true);
+
+        // Duplicates are not removed between active and noted
+        assertEquals(2, list.size());
+
+        mController.onOpActiveChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, false);
+
+        mTestableLooper.processAllMessages();
+
+        verify(mCallback, never()).onActiveStateChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, false);
+        list = mController.getActiveAppOps();
+        assertEquals(1, list.size());
+    }
+
+    @Test
+    public void testNotedAndActiveOnlyOneCall() {
+        mController.addCallback(new int[]{AppOpsManager.OP_FINE_LOCATION}, mCallback);
+
+        mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
+                AppOpsManager.MODE_ALLOWED);
+
+        mController.onOpActiveChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true);
+
+        mTestableLooper.processAllMessages();
+        verify(mCallback).onActiveStateChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true);
+    }
+
+    @Test
+    public void testActiveAndNotedOnlyOneCall() {
+        mController.addCallback(new int[]{AppOpsManager.OP_FINE_LOCATION}, mCallback);
+
+        mController.onOpActiveChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true);
+
+        mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
+                AppOpsManager.MODE_ALLOWED);
+
+        mTestableLooper.processAllMessages();
+        verify(mCallback).onActiveStateChanged(
+                AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true);
+    }
 }