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
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:cafca44d84955504857e337fced3bef2912802bd)
Merged-In: I016435abcfd5300f33374e5d5c144b25f81728b4
Change-Id: I016435abcfd5300f33374e5d5c144b25f81728b4

2 files changed, 54 insertions, 2 deletions

diff --git a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
index c11356b7308e..2fceb5b91bb2 100644
--- a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
+++ b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
@@ -728,7 +728,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
@@ -801,6 +801,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;
@@ -1256,6 +1263,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();
         }
     };
@@ -5899,7 +5910,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);
             }
diff --git a/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java
index a529382bfb26..b1e62f9c9a82 100644
--- a/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java
+++ b/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java
@@ -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)
@@ -2371,6 +2387,31 @@ public class NetworkPolicyManagerServiceTest {
     }
 
     @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.
         callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_TOP, 0);