From 01a7d32071240b171b19dec5d9eef07787f99dc4 Mon Sep 17 00:00:00 2001 From: Jean Chalard Date: Tue, 27 Jun 2023 08:54:23 +0000 Subject: [PATCH 1/7] Revert "Always disconnect agents immediately." This reverts commit 4bc9fa6b8cefca88f1065e95df06532979c202ee. Reason for revert: b/288450518 Some iterations over mNetworkAgentInfos result in networks being disconnected, which removes them from mNetworkAgentInfos during iteration. This crashes with NPE or OOB exceptions. Bug: 286649301 Bug: 288149251 Bug: 288450518 (cherry picked from https://android-review.googlesource.com/q/commit:3160bc0825490353d79c6586e21a7fc5893c678d) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:2a2b911a31e9996382eef9f61c7600dc30dd29ff) Merged-In: I6e0b5b614d9e88267db77cb807ae4bf09f88c0f6 Change-Id: I6e0b5b614d9e88267db77cb807ae4bf09f88c0f6 --- .../com/android/server/ConnectivityService.java | 16 ++++++---------- .../net/src/android/net/cts/NetworkAgentTest.kt | 13 ++++++------- .../android/server/ConnectivityServiceTest.java | 10 ++++------ 3 files changed, 16 insertions(+), 23 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 5e14be5852..28d1d73881 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -319,6 +319,7 @@ import java.io.IOException; import java.io.InterruptedIOException; import java.io.PrintWriter; import java.io.Writer; +import java.lang.IllegalArgumentException; import java.net.Inet4Address; import java.net.InetAddress; import java.net.InetSocketAddress; @@ -4011,7 +4012,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // the destroyed flag is only just above the "current satisfier wins" // tie-breaker. But technically anything that affects scoring should rematch. rematchAllNetworksAndRequests(); - mHandler.postDelayed(() -> disconnectAndDestroyNetwork(nai), timeoutMs); + mHandler.postDelayed(() -> nai.disconnect(), timeoutMs); break; } } @@ -4610,9 +4611,6 @@ public class ConnectivityService extends IConnectivityManager.Stub if (DBG) { log(nai.toShortString() + " disconnected, was satisfying " + nai.numNetworkRequests()); } - - nai.disconnect(); - // Clear all notifications of this network. mNotifier.clearNotification(nai.network.getNetId()); // A network agent has disconnected. @@ -5897,7 +5895,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork((Network) msg.obj); if (nai == null) break; nai.onPreventAutomaticReconnect(); - disconnectAndDestroyNetwork(nai); + nai.disconnect(); break; case EVENT_SET_VPN_NETWORK_PREFERENCE: handleSetVpnNetworkPreference((VpnNetworkPreferenceInfo) msg.obj); @@ -9044,7 +9042,7 @@ public class ConnectivityService extends IConnectivityManager.Stub break; } } - disconnectAndDestroyNetwork(nai); + nai.disconnect(); } private void handleLingerComplete(NetworkAgentInfo oldNetwork) { @@ -9586,10 +9584,7 @@ public class ConnectivityService extends IConnectivityManager.Stub updateLegacyTypeTrackerAndVpnLockdownForRematch(changes, nais); // Tear down all unneeded networks. - // Iterate in reverse order because teardownUnneededNetwork removes the nai from - // mNetworkAgentInfos. - for (int i = mNetworkAgentInfos.size() - 1; i >= 0; i--) { - final NetworkAgentInfo nai = mNetworkAgentInfos.valueAt(i); + for (NetworkAgentInfo nai : mNetworkAgentInfos) { if (unneeded(nai, UnneededFor.TEARDOWN)) { if (nai.getInactivityExpiry() > 0) { // This network has active linger timers and no requests, but is not @@ -9972,6 +9967,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // This has to happen after matching the requests, because callbacks are just requests. notifyNetworkCallbacks(networkAgent, ConnectivityManager.CALLBACK_PRECHECK); } else if (state == NetworkInfo.State.DISCONNECTED) { + networkAgent.disconnect(); if (networkAgent.isVPN()) { updateVpnUids(networkAgent, networkAgent.networkCapabilities, null); } diff --git a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt index 9f8a05db61..cf5fc50f04 100644 --- a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt +++ b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt @@ -1247,15 +1247,15 @@ class NetworkAgentTest { // Connect a third network. Because network1 is awaiting replacement, network3 is preferred // as soon as it validates (until then, it is outscored by network1). - // The fact that the first event seen by matchAllCallback is the connection of network3 + // The fact that the first events seen by matchAllCallback is the connection of network3 // implicitly ensures that no callbacks are sent since network1 was lost. val (agent3, network3) = connectNetwork() + matchAllCallback.expectAvailableThenValidatedCallbacks(network3) + testCallback.expectAvailableDoubleValidatedCallbacks(network3) + // As soon as the replacement arrives, network1 is disconnected. // Check that this happens before the replacement timeout (5 seconds) fires. - matchAllCallback.expectAvailableCallbacks(network3, validated = false) matchAllCallback.expect(network1, 2_000 /* timeoutMs */) - matchAllCallback.expectCaps(network3) { it.hasCapability(NET_CAPABILITY_VALIDATED) } - testCallback.expectAvailableDoubleValidatedCallbacks(network3) agent1.expectCallback() // Test lingering: @@ -1301,7 +1301,7 @@ class NetworkAgentTest { val callback = TestableNetworkCallback() requestNetwork(makeTestNetworkRequest(specifier = specifier6), callback) val agent6 = createNetworkAgent(specifier = specifier6) - agent6.register() + val network6 = agent6.register() if (SdkLevel.isAtLeastU()) { agent6.expectCallback() } else { @@ -1368,9 +1368,8 @@ class NetworkAgentTest { val (newWifiAgent, newWifiNetwork) = connectNetwork(TRANSPORT_WIFI) testCallback.expectAvailableCallbacks(newWifiNetwork, validated = true) - matchAllCallback.expectAvailableCallbacks(newWifiNetwork, validated = false) + matchAllCallback.expectAvailableThenValidatedCallbacks(newWifiNetwork) matchAllCallback.expect(wifiNetwork) - matchAllCallback.expectCaps(newWifiNetwork) { it.hasCapability(NET_CAPABILITY_VALIDATED) } wifiAgent.expectCallback() } diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index bb55aee35e..57c3acc9a2 100755 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -2946,24 +2946,22 @@ public class ConnectivityServiceTest { if (expectLingering) { generalCb.expectLosing(net1); } + generalCb.expectCaps(net2, c -> c.hasCapability(NET_CAPABILITY_VALIDATED)); + defaultCb.expectAvailableDoubleValidatedCallbacks(net2); // Make sure cell 1 is unwanted immediately if the radio can't time share, but only // after some delay if it can. if (expectLingering) { - generalCb.expectCaps(net2, c -> c.hasCapability(NET_CAPABILITY_VALIDATED)); - defaultCb.expectAvailableDoubleValidatedCallbacks(net2); net1.assertNotDisconnected(TEST_CALLBACK_TIMEOUT_MS); // always incurs the timeout generalCb.assertNoCallback(); // assertNotDisconnected waited for TEST_CALLBACK_TIMEOUT_MS, so waiting for the // linger period gives TEST_CALLBACK_TIMEOUT_MS time for the event to process. net1.expectDisconnected(UNREASONABLY_LONG_ALARM_WAIT_MS); - generalCb.expect(LOST, net1); } else { net1.expectDisconnected(TEST_CALLBACK_TIMEOUT_MS); - generalCb.expect(LOST, net1); - generalCb.expectCaps(net2, c -> c.hasCapability(NET_CAPABILITY_VALIDATED)); - defaultCb.expectAvailableDoubleValidatedCallbacks(net2); } + net1.disconnect(); + generalCb.expect(LOST, net1); // Remove primary from net 2 net2.setScore(new NetworkScore.Builder().build()); From 2e50cc9c29f97c03a5edc74c2a8cbd0c05ccbede Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 27 Jun 2023 19:16:00 +0900 Subject: [PATCH 2/7] Never create native network immediately. This patch flags off the functionality added by aosp/2162425 in the wake of b/286649301 where a race in ConnectivityService breaks WiFi connectivity until reboot. Bug: 286649301 Test: NetworkAgentTest ConnectivityServiceTest (cherry picked from https://android-review.googlesource.com/q/commit:a16607f421ecfbf37ef8fb2fa96d4d285df9e77b) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:1214b906463106d5a77f2da9a8bc49ddd868bd40) Merged-In: I96d755445f6e1f88bb71a7d32742e87dae185250 Change-Id: I96d755445f6e1f88bb71a7d32742e87dae185250 --- .../android/server/ConnectivityService.java | 8 +++++--- .../src/android/net/cts/NetworkAgentTest.kt | 20 ++++++++++++------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 28d1d73881..e6bc5015b4 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -4549,9 +4549,11 @@ public class ConnectivityService extends IConnectivityManager.Stub @VisibleForTesting protected static boolean shouldCreateNetworksImmediately() { - // Before U, physical networks are only created when the agent advances to CONNECTED. - // In U and above, all networks are immediately created when the agent is registered. - return SdkLevel.isAtLeastU(); + // The feature of creating the networks immediately was slated for U, but race conditions + // detected late required this was flagged off. + // TODO : enable this in a Mainline update or in V, and re-enable the test for this + // in NetworkAgentTest. + return false; } private static boolean shouldCreateNativeNetwork(@NonNull NetworkAgentInfo nai, diff --git a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt index cf5fc50f04..98ea2248bd 100644 --- a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt +++ b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt @@ -164,6 +164,12 @@ private fun Message(what: Int, arg1: Int, arg2: Int, obj: Any?) = Message.obtain it.obj = obj } +// On T and below, the native network is only created when the agent connects. +// Starting in U, the native network was to be created as soon as the agent is registered, +// but this has been flagged off for now pending resolution of race conditions. +// TODO : enable this in a Mainline update or in V. +private const val SHOULD_CREATE_NETWORKS_IMMEDIATELY = false + @RunWith(DevSdkIgnoreRunner::class) // NetworkAgent is not updatable in R-, so this test does not need to be compatible with older // versions. NetworkAgent was also based on AsyncChannel before S so cannot be tested the same way. @@ -1302,7 +1308,7 @@ class NetworkAgentTest { requestNetwork(makeTestNetworkRequest(specifier = specifier6), callback) val agent6 = createNetworkAgent(specifier = specifier6) val network6 = agent6.register() - if (SdkLevel.isAtLeastU()) { + if (SHOULD_CREATE_NETWORKS_IMMEDIATELY) { agent6.expectCallback() } else { // No callbacks are sent, so check LinkProperties to wait for the network to be created. @@ -1316,9 +1322,10 @@ class NetworkAgentTest { val timeoutMs = agent6.DEFAULT_TIMEOUT_MS.toInt() + 1_000 agent6.unregisterAfterReplacement(timeoutMs) agent6.expectCallback() - if (!SdkLevel.isAtLeastT() || SdkLevel.isAtLeastU()) { + if (!SdkLevel.isAtLeastT() || SHOULD_CREATE_NETWORKS_IMMEDIATELY) { // Before T, onNetworkDestroyed is called even if the network was never created. - // On U+, the network was created by register(). Destroying it sends onNetworkDestroyed. + // If immediate native network creation is supported, the network was created by + // register(). Destroying it sends onNetworkDestroyed. agent6.expectCallback() } // Poll for LinkProperties becoming null, because when onNetworkUnwanted is called, the @@ -1477,10 +1484,9 @@ class NetworkAgentTest { @Test fun testNativeNetworkCreation_PhysicalNetwork() { - // On T and below, the native network is only created when the agent connects. - // Starting in U, the native network is created as soon as the agent is registered. - doTestNativeNetworkCreation(expectCreatedImmediately = SdkLevel.isAtLeastU(), - intArrayOf(TRANSPORT_CELLULAR)) + doTestNativeNetworkCreation( + expectCreatedImmediately = SHOULD_CREATE_NETWORKS_IMMEDIATELY, + intArrayOf(TRANSPORT_CELLULAR)) } @Test From c9358ac55013abc3c159d835244b60b107d4aa81 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 29 Jun 2023 09:33:54 +0900 Subject: [PATCH 3/7] [DO NOT MERGE] Disable tests that depend on the prebuilt version in U This should be reverted as soon as it is merged into U and the build request is submitted. Bug: 286649301 Ignore-AOSP-First: udc-dev only change Test: NetworkAgentTest, TH (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:a0e8ab4933fc7fce41ce62942ca4e34bb1576f50) Merged-In: I2cbe990675b109e4ba744101dd7dbbb20229376f Change-Id: I2cbe990675b109e4ba744101dd7dbbb20229376f --- tests/cts/net/src/android/net/cts/NetworkAgentTest.kt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt index 98ea2248bd..5879065bb0 100644 --- a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt +++ b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt @@ -130,6 +130,7 @@ import kotlin.test.assertNull import kotlin.test.assertTrue import kotlin.test.fail import org.junit.After +import org.junit.Assume.assumeFalse import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -1214,6 +1215,7 @@ class NetworkAgentTest { @Test fun testUnregisterAfterReplacement() { + assumeFalse(Build.isDebuggable()) // Disabled presubmit pending prebuilt update on U // Keeps an eye on all test networks. val matchAllCallback = TestableNetworkCallback(timeoutMs = DEFAULT_TIMEOUT_MS) registerNetworkCallback(makeTestNetworkRequest(), matchAllCallback) @@ -1484,6 +1486,7 @@ class NetworkAgentTest { @Test fun testNativeNetworkCreation_PhysicalNetwork() { + assumeFalse(Build.isDebuggable()) // Disabled presubmit pending prebuilt update on U doTestNativeNetworkCreation( expectCreatedImmediately = SHOULD_CREATE_NETWORKS_IMMEDIATELY, intArrayOf(TRANSPORT_CELLULAR)) From cb4e91c5b503784b2a5dee97530e0695d0168e19 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 5 Jun 2023 18:47:42 +0900 Subject: [PATCH 4/7] Allow firing the LOST_INTERNET intent immediately in config When config_notifyNoInternetAsDialogWhenHighPriority is on, the LOST_INTERNET notification intent should be fired immediately rather than as a pending intent. Bug: 281970908 Test: Improve test for this in NetworkNotificationManagerTest (cherry picked from https://android-review.googlesource.com/q/commit:e13b833b21342d80ef940783acba4b3eecfce968) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:3bf3a59f359e2d6db3a335cad7085fe52a102e0b) Merged-In: I88565839a12a1ab4b096f763250944ebaf6c5349 Change-Id: I88565839a12a1ab4b096f763250944ebaf6c5349 --- .../res/values/config.xml | 15 ++- .../NetworkNotificationManager.java | 9 +- .../NetworkNotificationManagerTest.java | 106 +++++++++++++++--- 3 files changed, 108 insertions(+), 22 deletions(-) diff --git a/service/ServiceConnectivityResources/res/values/config.xml b/service/ServiceConnectivityResources/res/values/config.xml index 22d9b01b0f..f30abc6b0b 100644 --- a/service/ServiceConnectivityResources/res/values/config.xml +++ b/service/ServiceConnectivityResources/res/values/config.xml @@ -135,10 +135,17 @@ true - + false