From 9127f03a5cc7723638dfdff91fb0b6fc30aa5460 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 19 Nov 2019 21:30:44 +0900 Subject: [PATCH 1/2] [NS A18] Reverse listens and request-availables This is a long standing bug that happens to now be trivial to fix, and also be beneficial for refactoring Test: FrameworksNetTests NetworkStackTests Change-Id: I38110f3a4a75936ea755788e7f9fee67863e14be --- .../core/java/com/android/server/ConnectivityService.java | 8 ++++---- .../java/com/android/server/ConnectivityServiceTest.java | 5 +---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index a3a61720ac..6428a7012a 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6453,6 +6453,10 @@ public class ConnectivityService extends IConnectivityManager.Stub newNetwork.name(), score, newNetwork.getCurrentScore())); } + // Notify requested networks are available after the default net is switched, but + // before LegacyTypeTracker sends legacy broadcasts + for (NetworkRequestInfo nri : addedRequests) notifyNetworkAvailable(newNetwork, nri); + // Second pass: process all listens. if (wasBackgroundNetwork != newNetwork.isBackgroundNetwork()) { // If the network went from background to foreground or vice versa, we need to update @@ -6463,10 +6467,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } else { processListenRequests(newNetwork, false); } - - // do this after the default net is switched, but - // before LegacyTypeTracker sends legacy broadcasts - for (NetworkRequestInfo nri : addedRequests) notifyNetworkAvailable(newNetwork, nri); } /** diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index c4e353bdca..8eac95e523 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -3134,14 +3134,11 @@ public class ConnectivityServiceTest { .addTransportType(TRANSPORT_CELLULAR).build(); final TestNetworkCallback cellCallback = new TestNetworkCallback(); mCm.requestNetwork(cellRequest, cellCallback); - // NOTE: This request causes the network's capabilities to change. This - // is currently delivered before the onAvailable() callbacks. - // TODO: Fix this. - cellCallback.expectCapabilitiesWith(NET_CAPABILITY_FOREGROUND, mCellNetworkAgent); cellCallback.expectAvailableCallbacksValidated(mCellNetworkAgent); fgCallback.expectAvailableCallbacksValidated(mCellNetworkAgent); // Expect a network capabilities update with FOREGROUND, because the most recent // request causes its state to change. + cellCallback.expectCapabilitiesWith(NET_CAPABILITY_FOREGROUND, mCellNetworkAgent); callback.expectCapabilitiesWith(NET_CAPABILITY_FOREGROUND, mCellNetworkAgent); assertTrue(isForegroundNetwork(mCellNetworkAgent)); assertTrue(isForegroundNetwork(mWiFiNetworkAgent)); From 959c7cd889bb70c23af1f7294293eb22551beff2 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 19 Nov 2019 21:59:11 +0900 Subject: [PATCH 2/2] [NS A19] Inline updateCapabilities in rematch. This is ugly, but it's a necessary step to improve the code. Followups will clean this up. Importantly this kind of inlining will let us break the very confusing apparent loop between updateCapabilities and rematch. Test: FrameworksNetTests Change-Id: Ie756b9aa8066984264717f0b1e1f31606432f1a4 --- .../android/server/ConnectivityService.java | 67 +++++++++++++++++-- 1 file changed, 62 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 6428a7012a..9e6fd9e64f 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6459,11 +6459,68 @@ public class ConnectivityService extends IConnectivityManager.Stub // Second pass: process all listens. if (wasBackgroundNetwork != newNetwork.isBackgroundNetwork()) { - // If the network went from background to foreground or vice versa, we need to update - // its foreground state. It is safe to do this after rematching the requests because - // NET_CAPABILITY_FOREGROUND does not affect requests, as is not a requestable - // capability and does not affect the network's score (see the Slog.wtf call above). - updateCapabilities(score, newNetwork, newNetwork.networkCapabilities); + // TODO : most of the following is useless because the only thing that changed + // here is whether the network is a background network. Clean this up. + + NetworkCapabilities newNc = mixInCapabilities(newNetwork, + newNetwork.networkCapabilities); + + if (Objects.equals(newNetwork.networkCapabilities, newNc)) return; + + final int oldPermission = getNetworkPermission(newNetwork.networkCapabilities); + final int newPermission = getNetworkPermission(newNc); + if (oldPermission != newPermission && newNetwork.created && !newNetwork.isVPN()) { + try { + mNMS.setNetworkPermission(newNetwork.network.netId, newPermission); + } catch (RemoteException e) { + loge("Exception in setNetworkPermission: " + e); + } + } + + final NetworkCapabilities prevNc; + synchronized (newNetwork) { + prevNc = newNetwork.networkCapabilities; + newNetwork.setNetworkCapabilities(newNc); + } + + updateUids(newNetwork, prevNc, newNc); + + if (newNetwork.getCurrentScore() == score + && newNc.equalRequestableCapabilities(prevNc)) { + // If the requestable capabilities haven't changed, and the score hasn't changed, + // then the change we're processing can't affect any requests, it can only affect + // the listens on this network. + processListenRequests(newNetwork, true); + } else { + rematchAllNetworksAndRequests(); + notifyNetworkCallbacks(newNetwork, ConnectivityManager.CALLBACK_CAP_CHANGED); + } + + if (prevNc != null) { + final boolean oldMetered = prevNc.isMetered(); + final boolean newMetered = newNc.isMetered(); + final boolean meteredChanged = oldMetered != newMetered; + + if (meteredChanged) { + maybeNotifyNetworkBlocked(newNetwork, oldMetered, newMetered, + mRestrictBackground, mRestrictBackground); + } + + final boolean roamingChanged = prevNc.hasCapability(NET_CAPABILITY_NOT_ROAMING) + != newNc.hasCapability(NET_CAPABILITY_NOT_ROAMING); + + // Report changes that are interesting for network statistics tracking. + if (meteredChanged || roamingChanged) { + notifyIfacesChangedForNetworkStats(); + } + } + + if (!newNc.hasTransport(TRANSPORT_VPN)) { + // Tell VPNs about updated capabilities, since they may need to + // bubble those changes through. + updateAllVpnsCapabilities(); + } + } else { processListenRequests(newNetwork, false); }