Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 1a459638 authored by Fabian Kozynski's avatar Fabian Kozynski
Browse files

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
parent c64d1c46

packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java

+43 −8
Original line number Diff line number Diff line
@@ -193,20 +193,32 @@ public class AppOpsControllerImpl implements AppOpsController, 
            mNotedItems.remove(item);

            if (DEBUG) Log.w(TAG, "Removed item: " + item.toString());

        }

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

        }

packages/SystemUI/tests/src/com/android/systemui/appops/AppOpsControllerTest.java

+136 −2
Original line number Diff line number Diff line
@@ -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);

    }

}