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

Commit cafca44d authored by Suprabh Shukla's avatar Suprabh Shukla
Browse files

Remove UidStateCallbackInfo when uid is gone 

A bug was introduced by recent changes to filter out redundant process
state changes.
NPMS was caching UidStateCallbackInfo objects per uid and not clearing
it or resetting the procStateSeq when the uid went away. This resulted
in state-changes not being processed after app restarts.

For example, if an app exited from TOP, it would lose network access due
to some rules, but the last UidStateCallbackInfo would stay at TOP. If
the same app restarted, the change would be filtered out as
uninteresting even though the network rules require an update.

Test: atest FrameworksServicesTests:NetworkPolicyManagerServiceTest

Bug: 326370901
Bug: 326675380
Change-Id: I016435abcfd5300f33374e5d5c144b25f81728b4
parent 2ce9e8b0

services/core/java/com/android/server/net/NetworkPolicyManagerService.java

+13 −2
Original line number Diff line number Diff line
@@ -729,7 +729,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { 
     * Map of uid -> UidStateCallbackInfo objects holding the data received from

     * {@link IUidObserver#onUidStateChanged(int, int, long, int)} callbacks. In order to avoid

     * creating a new object for every callback received, we hold onto the object created for each

     * uid and reuse it.

     * uid and reuse it until the uid stays alive.

     *

     * Note that the lock used for accessing this object should not be used for anything else and we

     * should not be acquiring new locks or doing any heavy work while this lock is held since this
@@ -802,6 +802,13 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { 
                Clock.systemUTC());

    }



    @VisibleForTesting

    UidState getUidStateForTest(int uid) {

        synchronized (mUidRulesFirstLock) {

            return mUidState.get(uid);

        }

    }



    static class Dependencies {

        final Context mContext;

        final NetworkStatsManager mNetworkStatsManager;
@@ -1257,6 +1264,10 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { 
        }



        @Override public void onUidGone(int uid, boolean disabled) {

            synchronized (mUidStateCallbackInfos) {

                mUidStateCallbackInfos.remove(uid);

            }

            // TODO: b/327058756 - Remove any pending UID_MSG_STATE_CHANGED on the handler.

            mUidEventHandler.obtainMessage(UID_MSG_GONE, uid, 0).sendToTarget();

        }

    };
@@ -5918,7 +5929,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { 
    void handleUidGone(int uid) {

        Trace.traceBegin(Trace.TRACE_TAG_NETWORK, "onUidGone");

        try {

            boolean updated;

            final boolean updated;

            synchronized (mUidRulesFirstLock) {

                updated = removeUidStateUL(uid);

            }

services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java

+41 −0
Original line number Diff line number Diff line
@@ -149,6 +149,7 @@ import android.net.LinkProperties; 
import android.net.Network;

import android.net.NetworkCapabilities;

import android.net.NetworkPolicy;

import android.net.NetworkPolicyManager;

import android.net.NetworkStateSnapshot;

import android.net.NetworkTemplate;

import android.net.TelephonyNetworkSpecifier;
@@ -1620,6 +1621,12 @@ public class NetworkPolicyManagerServiceTest { 
        verify(mActivityManagerInternal).notifyNetworkPolicyRulesUpdated(UID_A, procStateSeq);

    }



    private void callAndWaitOnUidGone(int uid) throws Exception {

        // The disabled argument is used only for ephemeral apps and does not matter here.

        mUidObserver.onUidGone(uid, false /* disabled */);

        waitForUidEventHandlerIdle();

    }



    private void callAndWaitOnUidStateChanged(int uid, int procState, long procStateSeq)

            throws Exception {

        callAndWaitOnUidStateChanged(uid, procState, procStateSeq,
@@ -2250,6 +2257,15 @@ public class NetworkPolicyManagerServiceTest { 
        assertTrue(mService.isUidNetworkingBlocked(UID_A, false));

    }



    private boolean isUidState(int uid, int procState, int procStateSeq, int capability) {

        final NetworkPolicyManager.UidState uidState = mService.getUidStateForTest(uid);

        if (uidState == null) {

            return false;

        }

        return uidState.uid == uid && uidState.procStateSeq == procStateSeq

                && uidState.procState == procState && uidState.capability == capability;

    }



    @Ignore("Temporarily disabled until the feature is enabled")

    @Test

    @RequiresFlagsEnabled(Flags.FLAG_NETWORK_BLOCKED_FOR_TOP_SLEEPING_AND_ABOVE)
@@ -2370,6 +2386,31 @@ public class NetworkPolicyManagerServiceTest { 
        waitForUidEventHandlerIdle();

    }



    @Test

    public void testUidStateChangeAfterUidGone() throws Exception {

        final int testProcStateSeq = 51;

        final int testProcState = PROCESS_STATE_IMPORTANT_FOREGROUND;



        try (SyncBarrier b = new SyncBarrier(mService.mUidEventHandler)) {

            // First callback for uid.

            callOnUidStatechanged(UID_B, testProcState, testProcStateSeq, PROCESS_CAPABILITY_NONE);

            assertTrue(mService.mUidEventHandler.hasMessages(UID_MSG_STATE_CHANGED));

        }

        waitForUidEventHandlerIdle();

        assertTrue(isUidState(UID_B, testProcState, testProcStateSeq, PROCESS_CAPABILITY_NONE));



        callAndWaitOnUidGone(UID_B);

        try (SyncBarrier b = new SyncBarrier(mService.mUidEventHandler)) {

            // Even though the procState is the same, the uid had exited - so this should be

            // processed as a fresh callback.

            callOnUidStatechanged(UID_B, testProcState, testProcStateSeq + 1,

                    PROCESS_CAPABILITY_NONE);

            assertTrue(mService.mUidEventHandler.hasMessages(UID_MSG_STATE_CHANGED));

        }

        waitForUidEventHandlerIdle();

        assertTrue(isUidState(UID_B, testProcState, testProcStateSeq + 1, PROCESS_CAPABILITY_NONE));

    }



    @Test

    public void testLowPowerStandbyAllowlist() throws Exception {

        // Chain background is also enabled but these procstates are important enough to be exempt.