From d0ac32bfee65ecaf2852e85130fa9eeb0e8836c5 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 21 Mar 2025 12:17:36 +0900 Subject: [PATCH 1/4] Let DSCP policy test accept early network agent creation Test: DscpPolicyTest with and without the early creation patches Change-Id: I87e9d710e54e861a5f6c2767da7db67329e1bb59 --- tests/cts/net/src/android/net/cts/DscpPolicyTest.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/cts/net/src/android/net/cts/DscpPolicyTest.kt b/tests/cts/net/src/android/net/cts/DscpPolicyTest.kt index df4dab5544..d531e7abee 100644 --- a/tests/cts/net/src/android/net/cts/DscpPolicyTest.kt +++ b/tests/cts/net/src/android/net/cts/DscpPolicyTest.kt @@ -774,7 +774,9 @@ class DscpPolicyTest { runAsShell(MANAGE_TEST_NETWORKS) { agent.register() } // Without the fix, this will crash the system with SIGSEGV. agent.sendAddDscpPolicy(DscpPolicy.Builder(1, 1).build()) - agent.expectCallback() + // Will receive OnNetworkCreated first if the agent is created early. To avoid reading + // the flag here, use eventuallyExpect. + agent.eventuallyExpect() } } From 2f52a0e86d45f4607bae71e43da2de2834e9ef3d Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 21 Mar 2025 10:14:28 +0900 Subject: [PATCH 2/4] Reapply "Make iterating over mNetworkAgentInfos safer." Now that the faulty test is fixed, This reverts commit 09bc78fda764d57e257df3c59857ca2414d0f6f8. Test: ConnectivityCoverageTests CtsNetTestCases FrameworksTelephonyTests Change-Id: If52cc211b7859c0add07c16f171a6ce342ccca97 --- .../android/server/ConnectivityService.java | 168 ++++++++++-------- 1 file changed, 95 insertions(+), 73 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index bfb51dadad..728c293874 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -429,6 +429,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; import java.util.function.Consumer; +import java.util.function.Predicate; /** * @hide @@ -5323,12 +5324,12 @@ public class ConnectivityService extends IConnectivityManager.Stub { private void handlePrivateDnsSettingsChanged() { final PrivateDnsConfig cfg = mDnsManager.getPrivateDnsConfig(); - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { handlePerNetworkPrivateDnsConfig(nai, cfg); if (networkRequiresPrivateDnsValidation(nai)) { handleUpdateLinkProperties(nai, new LinkProperties(nai.linkProperties)); } - } + }); } private void handlePerNetworkPrivateDnsConfig(NetworkAgentInfo nai, PrivateDnsConfig cfg) { @@ -5651,16 +5652,16 @@ public class ConnectivityService extends IConnectivityManager.Stub { private void maybeDisableForwardRulesForDisconnectingNai( @NonNull final NetworkAgentInfo disconnecting, final boolean sendCallbacks) { // Step 1 : maybe this network was the upstream for one or more local networks. - for (final NetworkAgentInfo local : mNetworkAgentInfos) { - if (!local.isLocalNetwork()) continue; + forEachNetworkAgentInfo(local -> { + if (!local.isLocalNetwork()) return; // return@forEach final NetworkRequest selector = local.localNetworkConfig.getUpstreamSelector(); - if (null == selector) continue; + if (null == selector) return; // return@forEach final NetworkRequestInfo nri = mNetworkRequests.get(selector); // null == nri can happen while disconnecting a network, because destroyNetwork() is // called after removing all associated NRIs from mNetworkRequests. - if (null == nri) continue; + if (null == nri) return; // return@forEach final NetworkAgentInfo satisfier = nri.getSatisfier(); - if (disconnecting != satisfier) continue; + if (disconnecting != satisfier) return; // return@forEach removeLocalNetworkUpstream(local, disconnecting); // Set the satisfier to null immediately so that the LOCAL_NETWORK_CHANGED callback // correctly contains null as an upstream. @@ -5668,7 +5669,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { nri.setSatisfier(null, null); notifyNetworkCallbacks(local, CALLBACK_LOCAL_NETWORK_INFO_CHANGED); } - } + }); // Step 2 : maybe this is a local network that had an upstream. if (!disconnecting.isLocalNetwork()) return; @@ -5841,12 +5842,12 @@ public class ConnectivityService extends IConnectivityManager.Stub { mNetworkRequests.put(req, nri); // TODO: Consider update signal strength for other types. if (req.isListen()) { - for (final NetworkAgentInfo network : mNetworkAgentInfos) { + forEachNetworkAgentInfo(network -> { if (req.networkCapabilities.hasSignalStrength() && network.satisfiesImmutableCapabilitiesOf(req)) { updateSignalStrengthThresholds(network, "REGISTER", req); } - } + }); } else if (req.isRequest() && mNetworkRequestStateStatsMetrics != null) { mNetworkRequestStateStatsMetrics.onNetworkRequestReceived(req); } @@ -6141,13 +6142,13 @@ public class ConnectivityService extends IConnectivityManager.Stub { private void removeListenRequestFromNetworks(@NonNull final NetworkRequest req) { // listens don't have a singular affected Network. Check all networks to see // if this listen request applies and remove it. - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { nai.removeRequest(req.requestId); if (req.networkCapabilities.hasSignalStrength() && nai.satisfiesImmutableCapabilitiesOf(req)) { updateSignalStrengthThresholds(nai, "RELEASE", req); } - } + }); } /** @@ -6210,6 +6211,43 @@ public class ConnectivityService extends IConnectivityManager.Stub { } } + /** + * Perform the specified operation on all networks. + * + * This method will run |op| exactly once for each network currently registered at the + * time it is called, even if |op| adds or removes networks. + * + * @param op the operation to perform. The operation is allowed to disconnect any number of + * networks. + */ + private void forEachNetworkAgentInfo(final Consumer op) { + // Create a copy instead of iterating over the set so |op| is allowed to disconnect any + // number of networks (which removes it from mNetworkAgentInfos). The copy is cheap + // because there are at most a handful of NetworkAgents connected at any given time. + final NetworkAgentInfo[] nais = new NetworkAgentInfo[mNetworkAgentInfos.size()]; + mNetworkAgentInfos.toArray(nais); + for (NetworkAgentInfo nai : nais) { + op.accept(nai); + } + } + + /** + * Check whether the specified condition is true for any network. + * + * This method will stop evaluating as soon as the condition returns true for any network. + * The order of iteration is not contractual. + * + * @param condition the condition to verify. This method must not modify the set of networks in + * any way. + * @return whether {@code condition} returned true for any network + */ + private boolean anyNetworkAgentInfo(final Predicate condition) { + for (int i = mNetworkAgentInfos.size() - 1; i >= 0; i--) { + if (condition.test(mNetworkAgentInfos.valueAt(i))) return true; + } + return false; + } + private RequestInfoPerUidCounter getRequestCounter(NetworkRequestInfo nri) { return hasAnyPermissionOf(mContext, nri.mPid, nri.mUid, NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) @@ -6551,14 +6589,14 @@ public class ConnectivityService extends IConnectivityManager.Stub { ensureRunningOnConnectivityServiceThread(); // Agent info scores and offer scores depend on whether cells yields to bad wifi. final boolean avoidBadWifi = avoidBadWifi(); - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { nai.updateScoreForNetworkAgentUpdate(); if (avoidBadWifi) { // If the device is now avoiding bad wifi, remove notifications that might have // been put up when the device didn't. mNotifier.clearNotification(nai.network.getNetId(), NotificationType.LOST_INTERNET); } - } + }); // UpdateOfferScore will update mNetworkOffers inline, so make a copy first. final ArrayList offersToUpdate = new ArrayList<>(mNetworkOffers); for (final NetworkOfferInfo noi : offersToUpdate) { @@ -6896,19 +6934,15 @@ public class ConnectivityService extends IConnectivityManager.Stub { final Network underpinnedNetwork = ki.getUnderpinnedNetwork(); final Network network = ki.getNetwork(); - boolean networkFound = false; - boolean underpinnedNetworkFound = false; - for (NetworkAgentInfo n : mNetworkAgentInfos) { - if (n.network.equals(network)) networkFound = true; - if (n.everConnected() && n.network.equals(underpinnedNetwork)) { - underpinnedNetworkFound = true; - } - } + final boolean networkFound = + anyNetworkAgentInfo(n -> n.network.equals(network)); // If the network no longer exists, then the keepalive should have been // cleaned up already. There is no point trying to resume keepalives. if (!networkFound) return; + final boolean underpinnedNetworkFound = anyNetworkAgentInfo( + n -> n.everConnected() && n.network.equals(underpinnedNetwork)); if (underpinnedNetworkFound) { mKeepaliveTracker.handleMonitorAutomaticKeepalive(ki, underpinnedNetwork.netId); @@ -7368,12 +7402,12 @@ public class ConnectivityService extends IConnectivityManager.Stub { return new UnderlyingNetworkInfo[0]; } List infoList = new ArrayList<>(); - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { UnderlyingNetworkInfo info = createVpnInfo(nai); if (info != null) { infoList.add(info); } - } + }); return infoList.toArray(new UnderlyingNetworkInfo[infoList.size()]); } @@ -7451,11 +7485,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { */ private void propagateUnderlyingNetworkCapabilities(Network updatedNetwork) { ensureRunningOnConnectivityServiceThread(); - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { if (updatedNetwork == null || hasUnderlyingNetwork(nai, updatedNetwork)) { updateCapabilitiesForNetwork(nai); } - } + }); } private boolean isUidBlockedByVpn(int uid, List blockedUidRanges) { @@ -7503,11 +7537,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { mPermissionMonitor.updateVpnLockdownUidRanges(requireVpn, ranges); } - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { final boolean curMetered = nai.networkCapabilities.isMetered(); maybeNotifyNetworkBlocked(nai, curMetered, curMetered, mVpnBlockedUidRanges, newVpnBlockedUidRanges); - } + }); mVpnBlockedUidRanges = newVpnBlockedUidRanges; } @@ -9071,6 +9105,9 @@ public class ConnectivityService extends IConnectivityManager.Stub { // Tracks all NetworkAgents that are currently registered. // NOTE: Only should be accessed on ConnectivityServiceThread, except dump(). + // Code iterating over this set is recommended to use forAllNetworkAgentInfos(), which allows + // code within the loop to disconnect networks during iteration without causing null pointer or + // OOB exceptions. private final ArraySet mNetworkAgentInfos = new ArraySet<>(); // UID ranges for users that are currently blocked by VPNs. @@ -10439,7 +10476,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { // A NetworkAgent's allowedUids may need to be updated if the app has lost // carrier config - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { if (nai.networkCapabilities.getAllowedUidsNoCopy().contains(uid) && getSubscriptionIdFromNetworkCaps(nai.networkCapabilities) == subId) { final NetworkCapabilities nc = new NetworkCapabilities(nai.networkCapabilities); @@ -10451,7 +10488,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { mCarrierPrivilegeAuthenticator); updateCapabilities(nai.getScore(), nai, nc); } - } + }); } /** @@ -11368,7 +11405,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { throw new IllegalStateException("No user is available"); } - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { ArraySet allowedUidRanges = new ArraySet<>(); for (final UserHandle user : users) { final ArraySet restrictedUidRanges = @@ -11380,7 +11417,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { final UidRangeParcel[] rangesParcel = toUidRangeStableParcels(allowedUidRanges); configs.add(new NativeUidRangeConfig( nai.network.netId, rangesParcel, 0 /* subPriority */)); - } + }); // The netd API replaces the previous configs with the current configs. // Thus, for network disconnection or preference removal, no need to @@ -11602,9 +11639,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { // Gather the list of all relevant agents. final ArrayList nais = new ArrayList<>(); - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { - nais.add(nai); - } + forEachNetworkAgentInfo(nai -> nais.add(nai)); for (final NetworkRequestInfo nri : networkRequests) { // Non-multilayer listen requests can be ignored. @@ -11710,14 +11745,14 @@ public class ConnectivityService extends IConnectivityManager.Stub { // Don't send CALLBACK_LOCAL_NETWORK_INFO_CHANGED yet though : they should be sent after // onAvailable so clients know what network the change is about. Store such changes in // an array that's only allocated if necessary (because it's almost never necessary). - ArrayList localInfoChangedAgents = null; - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { - if (!nai.isLocalNetwork()) continue; + final ArrayList localInfoChangedAgents = new ArrayList<>(); + forEachNetworkAgentInfo(nai -> { + if (!nai.isLocalNetwork()) return; // return@forEach final NetworkRequest nr = nai.localNetworkConfig.getUpstreamSelector(); - if (null == nr) continue; // No upstream for this local network + if (null == nr) return; // return@forEach, no upstream for this local network final NetworkRequestInfo nri = mNetworkRequests.get(nr); final NetworkReassignment.RequestReassignment change = changes.getReassignment(nri); - if (null == change) continue; // No change in upstreams for this network + if (null == change) return; // return@forEach, no change in upstreams for this network final String fromIface = nai.linkProperties.getInterfaceName(); if (!hasSameInterfaceName(change.mOldNetwork, change.mNewNetwork) || change.mOldNetwork.isDestroyed()) { @@ -11745,9 +11780,8 @@ public class ConnectivityService extends IConnectivityManager.Stub { loge("Can't update forwarding rules", e); } } - if (null == localInfoChangedAgents) localInfoChangedAgents = new ArrayList<>(); localInfoChangedAgents.add(nai); - } + }); // Notify requested networks are available after the default net is switched, but // before LegacyTypeTracker sends legacy broadcasts @@ -11798,17 +11832,14 @@ public class ConnectivityService extends IConnectivityManager.Stub { } // Send LOCAL_NETWORK_INFO_CHANGED callbacks now that onAvailable and onLost have been sent. - if (null != localInfoChangedAgents) { - for (final NetworkAgentInfo nai : localInfoChangedAgents) { - notifyNetworkCallbacks(nai, - CALLBACK_LOCAL_NETWORK_INFO_CHANGED); - } + for (final NetworkAgentInfo nai : localInfoChangedAgents) { + notifyNetworkCallbacks(nai, CALLBACK_LOCAL_NETWORK_INFO_CHANGED); } updateLegacyTypeTrackerAndVpnLockdownForRematch(changes, nais); // Tear down all unneeded networks. - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { if (unneeded(nai, UnneededFor.TEARDOWN)) { if (nai.getInactivityExpiry() > 0) { // This network has active linger timers and no requests, but is not @@ -11826,7 +11857,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { teardownUnneededNetwork(nai); } } - } + }); } /** @@ -12339,7 +12370,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { * @param blockedReasons The reasons for why an uid is blocked. */ private void maybeNotifyNetworkBlockedForNewState(int uid, @BlockedReason int blockedReasons) { - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { final boolean metered = nai.networkCapabilities.isMetered(); final boolean vpnBlocked = isUidBlockedByVpn(uid, mVpnBlockedUidRanges); @@ -12347,9 +12378,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { uid, mUidBlockedReasons.get(uid, BLOCKED_REASON_NONE), metered, vpnBlocked); final int newBlockedState = getBlockedState(uid, blockedReasons, metered, vpnBlocked); - if (oldBlockedState == newBlockedState) { - continue; - } + if (oldBlockedState == newBlockedState) return; // return@forEach for (int i = 0; i < nai.numNetworkRequests(); i++) { NetworkRequest nr = nai.requestAt(i); NetworkRequestInfo nri = mNetworkRequests.get(nr); @@ -12358,7 +12387,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { newBlockedState); } } - } + }); } @VisibleForTesting @@ -12447,11 +12476,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { activeNetIds.add(nri.getSatisfier().network().netId); } } - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { if (activeNetIds.contains(nai.network().netId) || nai.isVPN()) { defaultNetworks.add(nai.network); } - } + }); return defaultNetworks; } @@ -13342,15 +13371,10 @@ public class ConnectivityService extends IConnectivityManager.Stub { } private boolean ownsVpnRunningOverNetwork(int uid, Network network) { - for (NetworkAgentInfo virtual : mNetworkAgentInfos) { - if (virtual.propagateUnderlyingCapabilities() - && virtual.networkCapabilities.getOwnerUid() == uid - && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, network)) { - return true; - } - } - - return false; + return anyNetworkAgentInfo(virtual -> + virtual.propagateUnderlyingCapabilities() + && virtual.networkCapabilities.getOwnerUid() == uid + && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, network)); } @CheckResult @@ -13521,18 +13545,16 @@ public class ConnectivityService extends IConnectivityManager.Stub { @Override public void onInterfaceLinkStateChanged(@NonNull String iface, boolean up) { mHandler.post(() -> { - for (NetworkAgentInfo nai : mNetworkAgentInfos) { - nai.clatd.handleInterfaceLinkStateChanged(iface, up); - } + forEachNetworkAgentInfo(nai -> + nai.clatd.handleInterfaceLinkStateChanged(iface, up)); }); } @Override public void onInterfaceRemoved(@NonNull String iface) { mHandler.post(() -> { - for (NetworkAgentInfo nai : mNetworkAgentInfos) { - nai.clatd.handleInterfaceRemoved(iface); - } + forEachNetworkAgentInfo(nai -> + nai.clatd.handleInterfaceRemoved(iface)); }); } } @@ -14313,7 +14335,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { final long oldIngressRateLimit = mIngressRateLimit; mIngressRateLimit = ConnectivitySettingsManager.getIngressRateLimitInBytesPerSecond( mContext); - for (final NetworkAgentInfo networkAgent : mNetworkAgentInfos) { + forEachNetworkAgentInfo(networkAgent -> { if (canNetworkBeRateLimited(networkAgent)) { // If rate limit has previously been enabled, remove the old limit first. if (oldIngressRateLimit >= 0) { @@ -14324,7 +14346,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { mIngressRateLimit); } } - } + }); } private boolean canNetworkBeRateLimited(@NonNull final NetworkAgentInfo networkAgent) { From 9fdc988dc860a6bb37247827457c4151b715c3b8 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 21 Mar 2025 10:13:47 +0900 Subject: [PATCH 3/4] Reapply "Always disconnect agents immediately." Now that the faulty test is fixed, This reverts commit 19dcc447c47b732bc2a3f0802097031accaa8ecb. Test: ConnectivityCoverageTests CtsNetTestCases FrameworksTelephonyTests Change-Id: I30fa35cef1729e6ecd4140c52fabfa2da1a9bb4b --- .../android/server/ConnectivityService.java | 27 ++++++++++++++++--- .../src/android/net/cts/NetworkAgentTest.kt | 14 +++++----- .../server/ConnectivityServiceTest.java | 17 ++++++------ 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 728c293874..30a13c346e 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -4894,7 +4894,11 @@ 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(() -> nai.disconnect(), timeoutMs); + if (mQueueNetworkAgentEventsInSystemServer) { + mHandler.postDelayed(() -> disconnectAndDestroyNetwork(nai), timeoutMs); + } else { + mHandler.postDelayed(() -> nai.disconnect(), timeoutMs); + } break; } } @@ -5509,6 +5513,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { if (DBG) { log(nai.toShortString() + " disconnected, was satisfying " + nai.numNetworkRequests()); } + + if (mQueueNetworkAgentEventsInSystemServer) { + nai.disconnect(); + } + // Clear all notifications of this network. mNotifier.clearNotification(nai.network.getNetId()); // A network agent has disconnected. @@ -7012,7 +7021,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork((Network) msg.obj); if (nai == null) break; nai.onPreventAutomaticReconnect(); - nai.disconnect(); + if (mQueueNetworkAgentEventsInSystemServer) { + disconnectAndDestroyNetwork(nai); + } else { + nai.disconnect(); + } break; case EVENT_SET_VPN_NETWORK_PREFERENCE: handleSetVpnNetworkPreference((VpnNetworkPreferenceInfo) msg.obj); @@ -11245,7 +11258,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { break; } } - nai.disconnect(); + if (mQueueNetworkAgentEventsInSystemServer) { + disconnectAndDestroyNetwork(nai); + } else { + nai.disconnect(); + } } private void handleLingerComplete(NetworkAgentInfo oldNetwork) { @@ -12246,7 +12263,9 @@ public class ConnectivityService extends IConnectivityManager.Stub { // This has to happen after matching the requests, because callbacks are just requests. notifyNetworkCallbacks(networkAgent, CALLBACK_PRECHECK); } else if (state == NetworkInfo.State.DISCONNECTED) { - networkAgent.disconnect(); + if (!mQueueNetworkAgentEventsInSystemServer) { + 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 bd9bd2a5f9..92155c3287 100644 --- a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt +++ b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt @@ -1660,16 +1660,17 @@ 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 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(lp = lp) - matchAllCallback.expectAvailableThenValidatedCallbacks(network3) - testCallback.expectAvailableDoubleValidatedCallbacks(network3) - sendAndExpectUdpPacket(network3, reader, iface) // 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) } + sendAndExpectUdpPacket(network3, reader, iface) + testCallback.expectAvailableDoubleValidatedCallbacks(network3) agent1.expectCallback() // Test lingering: @@ -1717,7 +1718,7 @@ class NetworkAgentTest { val callback = TestableNetworkCallback() requestNetwork(makeTestNetworkRequest(specifier = specifier6), callback) val agent6 = createNetworkAgent(specifier = specifier6) - val network6 = agent6.register() + agent6.register() if (SHOULD_CREATE_NETWORKS_IMMEDIATELY) { agent6.expectCallback() } else { @@ -1787,8 +1788,9 @@ class NetworkAgentTest { val (newWifiAgent, newWifiNetwork) = connectNetwork(TRANSPORT_WIFI) testCallback.expectAvailableCallbacks(newWifiNetwork, validated = true) - matchAllCallback.expectAvailableThenValidatedCallbacks(newWifiNetwork) + matchAllCallback.expectAvailableCallbacks(newWifiNetwork, validated = false) matchAllCallback.expect(wifiNetwork) + matchAllCallback.expectCaps(newWifiNetwork) { it.hasCapability(NET_CAPABILITY_VALIDATED) } wifiAgent.expectCallback() testCallback.expect(newWifiNetwork) diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 3eefa0f227..35a7fbd9c8 100755 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -3092,23 +3092,24 @@ public class ConnectivityServiceTest { generalCb.expectAvailableCallbacksUnvalidated(net2); 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) { + // Make sure cell 1 is unwanted immediately if the radio can't time share, but only + // after some delay if it can. + 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); + net1.disconnect(); + 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 324365a9014280a32395363e00642d01cc67814b Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 21 Mar 2025 10:12:38 +0900 Subject: [PATCH 4/4] Reapply "Immediately create networks unless chickened out" Now that the faulty test is fixed, This reverts commit 0141d82c2ded3a2272ccb076ef3d0c75363ba8d4. Test: ConnectivityCoverageTests CtsNetTestCases FrameworksTelephonyTests Change-Id: Ic9fee5bb7efb8ca4c05d7c9e600236186d094080 --- .../android/server/ConnectivityService.java | 14 +++++++----- .../src/android/net/cts/NetworkAgentTest.kt | 22 ++++++++++++++----- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 30a13c346e..b9b590b9a0 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -113,6 +113,7 @@ import static android.net.NetworkCapabilities.RES_ID_MATCH_ALL_RESERVATIONS; import static android.net.NetworkCapabilities.RES_ID_UNSET; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; import static android.net.NetworkCapabilities.TRANSPORT_TEST; +import static android.net.NetworkCapabilities.TRANSPORT_THREAD; import static android.net.NetworkCapabilities.TRANSPORT_VPN; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; import static android.net.NetworkRequest.Type.LISTEN_FOR_BEST; @@ -5448,15 +5449,18 @@ public class ConnectivityService extends IConnectivityManager.Stub { } @VisibleForTesting - protected static boolean shouldCreateNetworksImmediately(@NonNull NetworkCapabilities caps) { + protected boolean shouldCreateNetworksImmediately(@NonNull NetworkCapabilities caps) { // 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 caps.hasCapability(NET_CAPABILITY_LOCAL_NETWORK); + // TODO : remove when it's determined that the code is stable + return mQueueNetworkAgentEventsInSystemServer + // Local network agents for Thread used to not create networks immediately, + // but other local agents (tethering, P2P) require this to function. + || (caps.hasCapability(NET_CAPABILITY_LOCAL_NETWORK) + && !caps.hasTransport(TRANSPORT_THREAD)); } - private static boolean shouldCreateNativeNetwork(@NonNull NetworkAgentInfo nai, + private boolean shouldCreateNativeNetwork(@NonNull NetworkAgentInfo nai, @NonNull NetworkInfo.State state) { if (nai.isCreated()) return false; if (state == NetworkInfo.State.CONNECTED) return true; diff --git a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt index 92155c3287..4c3bce0baf 100644 --- a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt +++ b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt @@ -179,6 +179,10 @@ private const val TAG = "NetworkAgentTest" // without affecting the run time of successful runs. Thus, set a very high timeout. private const val DEFAULT_TIMEOUT_MS = 5000L +private const val QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER = + "queue_network_agent_events_in_system_server" + + // When waiting for a NetworkCallback to determine there was no timeout, waiting is the // only possible thing (the relevant handler is the one in the real ConnectivityService, // and then there is the Binder call), so have a short timeout for this as it will be @@ -203,12 +207,6 @@ private val REMOTE_ADDRESS = InetAddresses.parseNumericAddress("2001:db8::123") private val PREFIX = IpPrefix("2001:db8::/64") private val NEXTHOP = InetAddresses.parseNumericAddress("fe80::abcd") -// 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 - @AppModeFull(reason = "Instant apps can't use NetworkAgent because it needs NETWORK_FACTORY'.") // NetworkAgent is updated as part of the connectivity module, and running NetworkAgent tests in MTS // for modules other than Connectivity does not provide much value. Only run them in connectivity @@ -234,6 +232,18 @@ class NetworkAgentTest { private var qosTestSocket: Closeable? = null // either Socket or DatagramSocket private val ifacesToCleanUp = mutableListOf() + // Unless the queuing in system server feature is chickened out, native networks are created + // immediately. Historically they would only created as they'd connect, which would force + // the code to apply link properties multiple times and suffer errors early on. Creating + // them early required that ordering between the client and the system server is guaranteed + // (at least to some extent), which has been done by moving the event queue from the client + // to the system server. When that feature is not chickened out, create networks immediately. + private val SHOULD_CREATE_NETWORKS_IMMEDIATELY + get() = mCM.isConnectivityServiceFeatureEnabledForTesting( + QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER + ) + + @Before fun setUp() { instrumentation.getUiAutomation().adoptShellPermissionIdentity()