Always disconnect agents immediately.
Currently, various codepaths in ConnectivityService disconnect
networks using NetworkAgentInfo#disconnect. This posts a message
to the NetworkAgent to disconnect and also posts a message to
ConnectivityService to call disconnectAndDestroyNetwork, which
performs all cleanup in ConnectivityService. These two messages
race and the order is non-deterministic.
Instead, always disconnect using disconnectAndDestroyNetwork,
and have disconnectAndDestroyNetwork post the message to the
agent to disconnect.
This fixes a bug where if wifi uses unregisterAfterReplacement
twice in quick succession, when the third agent connects it
doesn't work because the interface is still being used by the
second network.
Also remove the import of IllegalArgumentException to keep the
linter happy (java.lang.* never needs to be imported).
Bug: 286649301
Test: covered by existing tests
(cherry picked from https://android-review.googlesource.com/q/commit:4bc9fa6b8cefca88f1065e95df06532979c202ee)
Merged-In: Ie01f5589d6839ac6db25f0ba98fc929fbb5b0a96
Change-Id: Ie01f5589d6839ac6db25f0ba98fc929fbb5b0a96
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index f85f336..a4b4ef8 100755
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -319,7 +319,6 @@
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;
@@ -4012,7 +4011,7 @@
// the destroyed flag is only just above the "current satisfier wins"
// tie-breaker. But technically anything that affects scoring should rematch.
rematchAllNetworksAndRequests();
- mHandler.postDelayed(() -> nai.disconnect(), timeoutMs);
+ mHandler.postDelayed(() -> disconnectAndDestroyNetwork(nai), timeoutMs);
break;
}
}
@@ -4609,6 +4608,9 @@
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.
@@ -5893,7 +5895,7 @@
final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork((Network) msg.obj);
if (nai == null) break;
nai.onPreventAutomaticReconnect();
- nai.disconnect();
+ disconnectAndDestroyNetwork(nai);
break;
case EVENT_SET_VPN_NETWORK_PREFERENCE:
handleSetVpnNetworkPreference((VpnNetworkPreferenceInfo) msg.obj);
@@ -9040,7 +9042,7 @@
break;
}
}
- nai.disconnect();
+ disconnectAndDestroyNetwork(nai);
}
private void handleLingerComplete(NetworkAgentInfo oldNetwork) {
@@ -9582,7 +9584,10 @@
updateLegacyTypeTrackerAndVpnLockdownForRematch(changes, nais);
// Tear down all unneeded networks.
- for (NetworkAgentInfo nai : mNetworkAgentInfos) {
+ // 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);
if (unneeded(nai, UnneededFor.TEARDOWN)) {
if (nai.getInactivityExpiry() > 0) {
// This network has active linger timers and no requests, but is not
@@ -9965,7 +9970,6 @@
// 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 cf5fc50..9f8a05d 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 @@
// 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 events seen by matchAllCallback is the connection of network3
+ // The fact that the first event 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<Lost>(network1, 2_000 /* timeoutMs */)
+ matchAllCallback.expectCaps(network3) { it.hasCapability(NET_CAPABILITY_VALIDATED) }
+ testCallback.expectAvailableDoubleValidatedCallbacks(network3)
agent1.expectCallback<OnNetworkUnwanted>()
// Test lingering:
@@ -1301,7 +1301,7 @@
val callback = TestableNetworkCallback()
requestNetwork(makeTestNetworkRequest(specifier = specifier6), callback)
val agent6 = createNetworkAgent(specifier = specifier6)
- val network6 = agent6.register()
+ agent6.register()
if (SdkLevel.isAtLeastU()) {
agent6.expectCallback<OnNetworkCreated>()
} else {
@@ -1368,8 +1368,9 @@
val (newWifiAgent, newWifiNetwork) = connectNetwork(TRANSPORT_WIFI)
testCallback.expectAvailableCallbacks(newWifiNetwork, validated = true)
- matchAllCallback.expectAvailableThenValidatedCallbacks(newWifiNetwork)
+ matchAllCallback.expectAvailableCallbacks(newWifiNetwork, validated = false)
matchAllCallback.expect<Lost>(wifiNetwork)
+ matchAllCallback.expectCaps(newWifiNetwork) { it.hasCapability(NET_CAPABILITY_VALIDATED) }
wifiAgent.expectCallback<OnNetworkUnwanted>()
}
diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
index 43c6225..595b786 100755
--- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
@@ -2943,22 +2943,24 @@
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());