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

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

Avoid obsolete uid-state change evaluations in NPMS 

In some cases, where a uid undergoes quick consecutive state changes and
the handler message has not completed execution, we can return early if
we know a following handler message to evaluate the same rules is
pending.

We peek at UidStateCallbackInfos to infer this state as these are
populated directly in the UidObserver callbacks before posting to the
handler.

The code avoids removing messages from the Handler as
- it is a linear operation that may be slower.
- the check on UidStateCallback happens later and returns from redundant
handler message-processing without incurring much additional work.

Also taking out some code out of the mUidRulesFirstLock lock.

Test: atest CtsHostsideNetworkTests
Test: FrameworksServicesTests:NetworkPolicyManagerServiceTest

Bug: 327058756
Change-Id: I764f714d3ce09699a0ab82b77c8b6a577cd9f9ab
parent 53e84b3f

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

+26 −15
Original line number Diff line number Diff line
@@ -1252,7 +1252,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { 
                if (isUidStateChangeRelevant(callbackInfo, procState, procStateSeq, capability)) {

                    callbackInfo.update(uid, procState, procStateSeq, capability);

                    if (!callbackInfo.isPending) {

                        mUidEventHandler.obtainMessage(UID_MSG_STATE_CHANGED, callbackInfo)

                        mUidEventHandler.obtainMessage(UID_MSG_STATE_CHANGED, uid, 0)

                                .sendToTarget();

                        callbackInfo.isPending = true;

                    }
@@ -1264,7 +1264,6 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { 
            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();

        }

    };
@@ -5870,13 +5869,13 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { 
    private final Handler.Callback mUidEventHandlerCallback = new Handler.Callback() {

        @Override

        public boolean handleMessage(Message msg) {

            final int uid = msg.arg1;

            switch (msg.what) {

                case UID_MSG_STATE_CHANGED: {

                    handleUidChanged((UidStateCallbackInfo) msg.obj);

                    handleUidChanged(uid);

                    return true;

                }

                case UID_MSG_GONE: {

                    final int uid = msg.arg1;

                    handleUidGone(uid);

                    return true;

                }
@@ -5887,23 +5886,27 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { 
        }

    };



    void handleUidChanged(@NonNull UidStateCallbackInfo uidStateCallbackInfo) {

    void handleUidChanged(int uid) {

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

        try {

            boolean updated;

            final int uid;

            final int procState;

            final long procStateSeq;

            final int capability;

            synchronized (mUidRulesFirstLock) {

            synchronized (mUidStateCallbackInfos) {

                    uid = uidStateCallbackInfo.uid;

                final UidStateCallbackInfo uidStateCallbackInfo = mUidStateCallbackInfos.get(uid);

                if (uidStateCallbackInfo == null) {

                    // This can happen if UidObserver#onUidGone gets called before we reach

                    // here. In this case, there is no point in processing this change as this

                    // will immediately be followed by a call to handleUidGone anyway.

                    return;

                }

                procState = uidStateCallbackInfo.procState;

                procStateSeq = uidStateCallbackInfo.procStateSeq;

                capability = uidStateCallbackInfo.capability;

                uidStateCallbackInfo.isPending = false;

            }



            final boolean updated;

            synchronized (mUidRulesFirstLock) {

                // We received a uid state change callback, add it to the history so that it

                // will be useful for debugging.

                mLogger.uidStateChanged(uid, procState, procStateSeq, capability);
@@ -5926,6 +5929,14 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { 
    void handleUidGone(int uid) {

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

        try {

            synchronized (mUidStateCallbackInfos) {

                if (mUidStateCallbackInfos.contains(uid)) {

                    // This can happen if UidObserver#onUidStateChanged gets called before we

                    // reach here. In this case, there is no point in processing this change as this

                    // will immediately be followed by a call to handleUidChanged anyway.

                    return;

                }

            }

            final boolean updated;

            synchronized (mUidRulesFirstLock) {

                updated = removeUidStateUL(uid);

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

+39 −0
Original line number Diff line number Diff line
@@ -2403,6 +2403,45 @@ public class NetworkPolicyManagerServiceTest { 
        assertTrue(isUidState(UID_B, testProcState, testProcStateSeq + 1, PROCESS_CAPABILITY_NONE));

    }



    @Test

    public void testObsoleteHandleUidGone() throws Exception {

        callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_TOP, 51);

        assertFalse(mService.isUidNetworkingBlocked(UID_A, false));



        clearInvocations(mNetworkManager);



        // In the service, handleUidGone is only called from mUidEventHandler. Then a call to it may

        // be rendered obsolete by a newer uid change posted on the handler. The latest uid state

        // change is always reflected in the current UidStateChangeCallbackInfo for the uid, so to

        // simulate an obsolete call for test, we directly call handleUidGone and leave the state in

        // UidStateChangeCallbackInfo set by the previous call to onUidStateChanged(TOP). This call

        // should then do nothing.

        mService.handleUidGone(UID_A);



        verify(mNetworkManager, times(0)).setFirewallUidRule(anyInt(), anyInt(), anyInt());

        assertFalse(mService.isUidNetworkingBlocked(UID_A, false));

    }



    @Test

    @RequiresFlagsEnabled(Flags.FLAG_NETWORK_BLOCKED_FOR_TOP_SLEEPING_AND_ABOVE)

    public void testObsoleteHandleUidChanged() throws Exception {

        callAndWaitOnUidGone(UID_A);

        assertTrue(mService.isUidNetworkingBlocked(UID_A, false));



        clearInvocations(mNetworkManager);



        // In the service, handleUidChanged is only called from mUidEventHandler. Then a call to it

        // may be rendered obsolete by an immediate uid-gone posted on the handler. The latest uid

        // state change is always reflected in the current UidStateChangeCallbackInfo for the uid,

        // so to simulate an obsolete call for test, we directly call handleUidChanged and leave the

        // state in UidStateChangeCallbackInfo as null as it would get removed by the previous call

        // to onUidGone(). This call should then do nothing.

        mService.handleUidChanged(UID_A);



        verify(mNetworkManager, times(0)).setFirewallUidRule(anyInt(), anyInt(), anyInt());

        assertTrue(mService.isUidNetworkingBlocked(UID_A, false));

    }



    @Test

    public void testLowPowerStandbyAllowlist() throws Exception {

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