Don't send onLinkPropertiesChanged after onLost for 464xlat.
Currently, when a network that uses 464xlat is torn down,
NetworkCallbacks will receive onLinkPropertiesChanged after
onLost. This is confusing and incorrect.
The incorrect callback is sent because handleLinkProperties
checks that the netId of the agent still exists, not that the
NetworkAgent is still registered. This is normally correct,
because the NetworkAgent is removed from mNetworkAgentInfos and
the netId are removed from mNetworkForNetId by the same method,
disconnectAndDestroyNetwork.
In this specific case it's not correct, because the call to
handleUpdateLinkProperties is from disconnectAndDestroyNetwork
itself via nai.clatd.update and calls Nat464Xlat#stop.
No other callers of handleUpdateLinkProperties are affected
because:
- EVENT_NETWORK_PROPERTIES_CHANGED is called only by
maybeHandleNetworkAgentMessage, which first checks that the
NetworkAgent is registered.
- handlePrivateDnsSettingsChanged only looks at registered
NetworkAgents (it loops over mNetworkAgentInfos).
- handlePrivateDnsValidationUpdate, handleNat64PrefixEvent and
handleCapportApiDataUpdate call getNetworkAgentInfoForNetId,
which will correctly determine that the agent is no longer
registered, since they run on the handler thread and thus
cannot run at the same time as disconnectAndDestroyNetwork.
The existing code contains a check for the netId being current.
This is intended to ensure that an update from a NetworkAgent
cannot affect another agent with the same Network. This extra
check is not necessary, because code running on the handler
thread can never observe a NetworkAgent in mNetworkAgentInfos
unless mNetworkForNetId maps that NetworkAgent's Network to that
NetworkAgent. This is because mNetworkForNetId is updated by the
same methods as mNetworkAgentInfos, and those updates occur on
the handler thread. So all code on the handler thread will see
those two as consistent.
Bug: 176496580
Test: atest FrameworksNetTests CtsNetTestCases HostsideVpnTests
Change-Id: I944f4c6ad36206bdccd85a6ea7ef71324a29c685
diff --git a/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java b/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java
index cd27318..69471a1 100644
--- a/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java
+++ b/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java
@@ -398,10 +398,10 @@
// notifications (e.g. matching more than one of our callbacks).
//
// Also, it can happen that onLinkPropertiesChanged is called after
- // onLost removed the state from mNetworkMap. This appears to be due
- // to a bug in disconnectAndDestroyNetwork, which calls
- // nai.clatd.update() after the onLost callbacks.
- // TODO: fix the bug and make this method void.
+ // onLost removed the state from mNetworkMap. This is due to a bug
+ // in disconnectAndDestroyNetwork, which calls nai.clatd.update()
+ // after the onLost callbacks. This was fixed in S.
+ // TODO: make this method void when R is no longer supported.
return null;
}
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index 842ad62..49aec86 100644
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -6294,8 +6294,7 @@
// there may not be a strict 1:1 correlation between the two.
private final NetIdManager mNetIdManager;
- // NetworkAgentInfo keyed off its connecting messenger
- // TODO - eval if we can reduce the number of lists/hashmaps/sparsearrays
+ // Tracks all NetworkAgents that are currently registered.
// NOTE: Only should be accessed on ConnectivityServiceThread, except dump().
private final ArraySet<NetworkAgentInfo> mNetworkAgentInfos = new ArraySet<>();
@@ -7433,7 +7432,7 @@
public void handleUpdateLinkProperties(NetworkAgentInfo nai, LinkProperties newLp) {
ensureRunningOnConnectivityServiceThread();
- if (getNetworkAgentInfoForNetId(nai.network.getNetId()) != nai) {
+ if (!mNetworkAgentInfos.contains(nai)) {
// Ignore updates for disconnected networks
return;
}
diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
index ab7d5ba..edaf218 100644
--- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
@@ -8544,13 +8544,8 @@
// Disconnect the network. clat is stopped and the network is destroyed.
mCellNetworkAgent.disconnect();
networkCallback.expectCallback(CallbackEntry.LOST, mCellNetworkAgent);
- // TODO: delete this spurious onLinkPropertiesChanged callback.
- networkCallback.expectLinkPropertiesThat(mCellNetworkAgent,
- lp -> lp.getStackedLinks().isEmpty());
networkCallback.assertNoCallback();
verify(mMockNetd).clatdStop(MOBILE_IFNAME);
- verify(mMockNetd).networkRemoveInterface(cellNetId, CLAT_MOBILE_IFNAME);
- assertRoutesRemoved(cellNetId, stackedDefault);
verify(mMockNetd).idletimerRemoveInterface(eq(MOBILE_IFNAME), anyInt(),
eq(Integer.toString(TRANSPORT_CELLULAR)));
verify(mMockNetd).networkDestroy(cellNetId);