From 282f743a8c374ca08c16fc4835287ce49292c098 Mon Sep 17 00:00:00 2001 From: Treehugger Robot Date: Wed, 30 Jun 2021 21:59:16 +0000 Subject: [PATCH] Fix network callback with the same PendingIntent does not release Currently, ConnectivityService uses EVENT_REGISTER_NETWORK_LISTENER to dispatch registering network callback with pending intent, this is wrong since the code flow will not check if the pending intent is duplicated. Thus, the registration will be duplicated if the caller uses the same pending intent and register multiple times. This change fixes the logic by using EVENT_REGISTER_NETWORK_LISTENER_WITH_INTENT instead of EVENT_REGISTER_NETWORK_LISTENER when dispatching register network callback with pending intent. Test: atest android.net.cts.ConnectivityManagerTest#testRegisterNetworkRequest_identicalPendingIntents Test: atest android.net.cts.ConnectivityManagerTest#testRegisterNetworkCallback_identicalPendingIntents Test: atest ConnectivityServiceTest#testNetworkCallbackMaximum Test: 1. Use test app to file callback with same PendingIntent 2. Check dumpsys output Bug: 189868426 Original-Change: https://android-review.googlesource.com/1727470 Merged-In: I38bdea3a026a78a6dc34b5200d43a75b3cd1ac0c Change-Id: I38bdea3a026a78a6dc34b5200d43a75b3cd1ac0c --- .../android/server/ConnectivityService.java | 3 +- .../net/cts/ConnectivityManagerTest.java | 9 +-- .../server/ConnectivityServiceTest.java | 70 ++++++++++++------- 3 files changed, 53 insertions(+), 29 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 94f652d307..7b5850399e 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -6288,7 +6288,8 @@ public class ConnectivityService extends IConnectivityManager.Stub callingAttributionTag); if (VDBG) log("pendingListenForNetwork for " + nri); - mHandler.sendMessage(mHandler.obtainMessage(EVENT_REGISTER_NETWORK_LISTENER, nri)); + mHandler.sendMessage(mHandler.obtainMessage( + EVENT_REGISTER_NETWORK_LISTENER_WITH_INTENT, nri)); } /** Returns the next Network provider ID. */ diff --git a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java index 9ad952241a..4403a0e744 100644 --- a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java +++ b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java @@ -76,6 +76,7 @@ import static android.system.OsConstants.AF_UNSPEC; import static com.android.compatibility.common.util.SystemUtil.callWithShellPermissionIdentity; import static com.android.compatibility.common.util.SystemUtil.runShellCommand; import static com.android.compatibility.common.util.SystemUtil.runWithShellPermissionIdentity; +import static com.android.modules.utils.build.SdkLevel.isAtLeastS; import static com.android.networkstack.apishim.ConstantsShim.BLOCKED_REASON_LOCKDOWN_VPN; import static com.android.networkstack.apishim.ConstantsShim.BLOCKED_REASON_NONE; import static com.android.testutils.MiscAsserts.assertThrows; @@ -943,8 +944,8 @@ public class ConnectivityManagerTest { // noticeably flaky. Thread.sleep(NO_CALLBACK_TIMEOUT_MS); - // TODO: BUG (b/189868426): this should also apply to listens - if (!useListen) { + // For R- frameworks, listens will receive duplicated callbacks. See b/189868426. + if (isAtLeastS() || !useListen) { assertEquals("PendingIntent should only be received once", 1, receivedCount.get()); } } finally { @@ -959,8 +960,8 @@ public class ConnectivityManagerTest { boolean useListen) { assertArrayEquals(filed.networkCapabilities.getCapabilities(), broadcasted.networkCapabilities.getCapabilities()); - // TODO: BUG (b/189868426): this should also apply to listens - if (useListen) return; + // For R- frameworks, listens will receive duplicated callbacks. See b/189868426. + if (!isAtLeastS() && useListen) return; assertArrayEquals(filed.networkCapabilities.getTransportTypes(), broadcasted.networkCapabilities.getTransportTypes()); } diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index f6ea964572..dfca73ec5f 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -5863,37 +5863,59 @@ public class ConnectivityServiceTest { @Test public void testNetworkCallbackMaximum() throws Exception { final int MAX_REQUESTS = 100; - final int CALLBACKS = 89; - final int INTENTS = 11; + final int CALLBACKS = 87; + final int DIFF_INTENTS = 10; + final int SAME_INTENTS = 10; final int SYSTEM_ONLY_MAX_REQUESTS = 250; - assertEquals(MAX_REQUESTS, CALLBACKS + INTENTS); + // Assert 1 (Default request filed before testing) + CALLBACKS + DIFF_INTENTS + + // 1 (same intent) = MAX_REQUESTS - 1, since the capacity is MAX_REQUEST - 1. + assertEquals(MAX_REQUESTS - 1, 1 + CALLBACKS + DIFF_INTENTS + 1); NetworkRequest networkRequest = new NetworkRequest.Builder().build(); ArrayList registered = new ArrayList<>(); - int j = 0; - while (j++ < CALLBACKS / 2) { - NetworkCallback cb = new NetworkCallback(); - mCm.requestNetwork(networkRequest, cb); + for (int j = 0; j < CALLBACKS; j++) { + final NetworkCallback cb = new NetworkCallback(); + if (j < CALLBACKS / 2) { + mCm.requestNetwork(networkRequest, cb); + } else { + mCm.registerNetworkCallback(networkRequest, cb); + } registered.add(cb); } - while (j++ < CALLBACKS) { - NetworkCallback cb = new NetworkCallback(); - mCm.registerNetworkCallback(networkRequest, cb); - registered.add(cb); + + // Since ConnectivityService will de-duplicate the request with the same intent, + // register multiple times does not really increase multiple requests. + final PendingIntent same_pi = PendingIntent.getBroadcast(mContext, 0 /* requestCode */, + new Intent("same"), FLAG_IMMUTABLE); + for (int j = 0; j < SAME_INTENTS; j++) { + mCm.registerNetworkCallback(networkRequest, same_pi); + // Wait for the requests with the same intent to be de-duplicated. Because + // ConnectivityService side incrementCountOrThrow in binder, decrementCount in handler + // thread, waitForIdle is needed to ensure decrementCount being invoked for same intent + // requests before doing further tests. + waitForIdle(); } - j = 0; - while (j++ < INTENTS / 2) { - final PendingIntent pi = PendingIntent.getBroadcast(mContext, 0 /* requestCode */, - new Intent("a" + j), FLAG_IMMUTABLE); - mCm.requestNetwork(networkRequest, pi); - registered.add(pi); + for (int j = 0; j < SAME_INTENTS; j++) { + mCm.requestNetwork(networkRequest, same_pi); + // Wait for the requests with the same intent to be de-duplicated. + // Refer to the reason above. + waitForIdle(); } - while (j++ < INTENTS) { - final PendingIntent pi = PendingIntent.getBroadcast(mContext, 0 /* requestCode */, - new Intent("b" + j), FLAG_IMMUTABLE); - mCm.registerNetworkCallback(networkRequest, pi); - registered.add(pi); + registered.add(same_pi); + + for (int j = 0; j < DIFF_INTENTS; j++) { + if (j < DIFF_INTENTS / 2) { + final PendingIntent pi = PendingIntent.getBroadcast(mContext, 0 /* requestCode */, + new Intent("a" + j), FLAG_IMMUTABLE); + mCm.requestNetwork(networkRequest, pi); + registered.add(pi); + } else { + final PendingIntent pi = PendingIntent.getBroadcast(mContext, 0 /* requestCode */, + new Intent("b" + j), FLAG_IMMUTABLE); + mCm.registerNetworkCallback(networkRequest, pi); + registered.add(pi); + } } // Test that the limit is enforced when MAX_REQUESTS simultaneous requests are added. @@ -5943,10 +5965,10 @@ public class ConnectivityServiceTest { for (Object o : registered) { if (o instanceof NetworkCallback) { - mCm.unregisterNetworkCallback((NetworkCallback)o); + mCm.unregisterNetworkCallback((NetworkCallback) o); } if (o instanceof PendingIntent) { - mCm.unregisterNetworkCallback((PendingIntent)o); + mCm.unregisterNetworkCallback((PendingIntent) o); } } waitForIdle();