From 5c4bddb8cd41bd630dbe1675574419bb50a8c5d1 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 19 Nov 2019 22:08:18 +0900 Subject: [PATCH 1/5] [NS A20] Cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A lot of this code can't be triggered at all. • newNetwork.created in l.6488 is implied by newNetwork.everConnected in l.6357 • !newNetwork.isVPN() in l.6488 is implied by the fact that VPNs are always foreground, so oldPermission can't != newPermission in l.6488 • updateUids in l.6502 is useless because uids can't change during a rematch (because there is no code doing that). Metered state and roaming state similarly can't change during a rematch, so meteredChanged and roamingChanged are always false • updateAllVpnCapabilities in l.6537 is useless because VPN do not inherit the foreground state of their underlying networks, which would be the only reason to call that in l.6537 • Object.equals() in l.6480 is necessary false because at this line it is known that the foreground state has changed, which must have caused the NET_CAPABILITY_FOREGROUND to be different, so the objects can't be equal Test: FrameworksNetTests NetworkStackTest Change-Id: I2a52f7f4a085f3eea22a1dd170af8f04671250ff --- .../android/server/ConnectivityService.java | 52 ++----------------- 1 file changed, 5 insertions(+), 47 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 753c1171ae..d96362c0f2 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6470,17 +6470,12 @@ public class ConnectivityService extends IConnectivityManager.Stub // Second pass: process all listens. if (wasBackgroundNetwork != newNetwork.isBackgroundNetwork()) { - // 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, + final 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()) { + if (oldPermission != newPermission) { try { mNMS.setNetworkPermission(newNetwork.network.netId, newPermission); } catch (RemoteException e) { @@ -6488,50 +6483,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - 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(); - } - + // The requestable capabilities and the score can't have changed, therefore the change + // in background can't affect any requests. Only processing the listens is fine. + processListenRequests(newNetwork, true); } else { processListenRequests(newNetwork, false); } From 1123f5d0677da9fbf15ed7fe2af843f708de1f43 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 19 Nov 2019 22:20:55 +0900 Subject: [PATCH 2/5] [NS A21] More cleanup Test: FrameworksNetTests NetworkStackTests Change-Id: I4771f2e9151ff16a7045d9c3025ac686f244b22d --- .../com/android/server/ConnectivityService.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index d96362c0f2..692b050f0c 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6468,8 +6468,9 @@ public class ConnectivityService extends IConnectivityManager.Stub // before LegacyTypeTracker sends legacy broadcasts for (NetworkRequestInfo nri : addedRequests) notifyNetworkAvailable(newNetwork, nri); - // Second pass: process all listens. - if (wasBackgroundNetwork != newNetwork.isBackgroundNetwork()) { + // Maybe the network changed background states. Update its capabilities. + final boolean backgroundChanged = wasBackgroundNetwork != newNetwork.isBackgroundNetwork(); + if (backgroundChanged) { final NetworkCapabilities newNc = mixInCapabilities(newNetwork, newNetwork.networkCapabilities); @@ -6486,13 +6487,10 @@ public class ConnectivityService extends IConnectivityManager.Stub synchronized (newNetwork) { newNetwork.setNetworkCapabilities(newNc); } - - // The requestable capabilities and the score can't have changed, therefore the change - // in background can't affect any requests. Only processing the listens is fine. - processListenRequests(newNetwork, true); - } else { - processListenRequests(newNetwork, false); } + + // Finally, process listen requests. + processListenRequests(newNetwork, backgroundChanged); } /** From cd397a204208fefd32221ca78b1ffa2511fea7f2 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 22 Nov 2019 22:33:33 +0900 Subject: [PATCH 3/5] [NS A22] Small refactoring The point of this is to be able to move parts of processListenRequests independently. Test: FrameworksNetTests Change-Id: I6c889b15696123c1120221977b0f36fa3d91de56 --- .../android/server/ConnectivityService.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 692b050f0c..fc9c9cd364 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6271,8 +6271,18 @@ public class ConnectivityService extends IConnectivityManager.Stub updateAllVpnsCapabilities(); } - private void processListenRequests(NetworkAgentInfo nai, boolean capabilitiesChanged) { + private void processListenRequests(@NonNull NetworkAgentInfo nai, boolean capabilitiesChanged) { // For consistency with previous behaviour, send onLost callbacks before onAvailable. + processNewlyLostListenRequests(nai); + + if (capabilitiesChanged) { + notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED); + } + + processNewlySatisfiedListenRequests(nai); + } + + private void processNewlyLostListenRequests(@NonNull final NetworkAgentInfo nai) { for (NetworkRequestInfo nri : mNetworkRequests.values()) { NetworkRequest nr = nri.request; if (!nr.isListen()) continue; @@ -6281,11 +6291,9 @@ public class ConnectivityService extends IConnectivityManager.Stub callCallbackForRequest(nri, nai, ConnectivityManager.CALLBACK_LOST, 0); } } + } - if (capabilitiesChanged) { - notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED); - } - + private void processNewlySatisfiedListenRequests(@NonNull final NetworkAgentInfo nai) { for (NetworkRequestInfo nri : mNetworkRequests.values()) { NetworkRequest nr = nri.request; if (!nr.isListen()) continue; From 05edd05f53f19b35816c01032d7eb7b183735927 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 22 Nov 2019 22:39:56 +0900 Subject: [PATCH 4/5] [NS A23] Move a synchronized block in a central place As the calls to this apparently need to be synchronized, let's do it all in the same place instead of in all callers Test: FrameworksNetTests Change-Id: I0c097e7756fc155ba0243834b84626e86c68340e --- .../android/server/ConnectivityService.java | 26 +++++++++---------- .../server/connectivity/NetworkAgentInfo.java | 7 ++++- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index fc9c9cd364..325bc19686 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -5596,7 +5596,7 @@ public class ConnectivityService extends IConnectivityManager.Stub ns, mContext, mTrackerHandler, new NetworkMisc(networkMisc), this, mNetd, mDnsResolver, mNMS, factorySerialNumber); // Make sure the network capabilities reflect what the agent info says. - nai.setNetworkCapabilities(mixInCapabilities(nai, nc)); + nai.getAndSetNetworkCapabilities(mixInCapabilities(nai, nc)); final String extraInfo = networkInfo.getExtraInfo(); final String name = TextUtils.isEmpty(extraInfo) ? nai.networkCapabilities.getSSID() : extraInfo; @@ -5950,11 +5950,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - final NetworkCapabilities prevNc; - synchronized (nai) { - prevNc = nai.networkCapabilities; - nai.setNetworkCapabilities(newNc); - } + final NetworkCapabilities prevNc = nai.getAndSetNetworkCapabilities(newNc); updateUids(nai, prevNc, newNc); @@ -6476,6 +6472,11 @@ public class ConnectivityService extends IConnectivityManager.Stub // before LegacyTypeTracker sends legacy broadcasts for (NetworkRequestInfo nri : addedRequests) notifyNetworkAvailable(newNetwork, nri); + // Finally, process listen requests and update capabilities if the background state has + // changed for this network. For consistency with previous behavior, send onLost callbacks + // before onAvailable. + processNewlyLostListenRequests(newNetwork); + // Maybe the network changed background states. Update its capabilities. final boolean backgroundChanged = wasBackgroundNetwork != newNetwork.isBackgroundNetwork(); if (backgroundChanged) { @@ -6492,13 +6493,11 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - synchronized (newNetwork) { - newNetwork.setNetworkCapabilities(newNc); - } + newNetwork.getAndSetNetworkCapabilities(newNc); + notifyNetworkCallbacks(newNetwork, ConnectivityManager.CALLBACK_CAP_CHANGED); } - // Finally, process listen requests. - processListenRequests(newNetwork, backgroundChanged); + processNewlySatisfiedListenRequests(newNetwork); } /** @@ -6683,9 +6682,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // NetworkCapabilities need to be set before sending the private DNS config to // NetworkMonitor, otherwise NetworkMonitor cannot determine if validation is required. - synchronized (networkAgent) { - networkAgent.setNetworkCapabilities(networkAgent.networkCapabilities); - } + networkAgent.getAndSetNetworkCapabilities(networkAgent.networkCapabilities); + handlePerNetworkPrivateDnsConfig(networkAgent, mDnsManager.getPrivateDnsConfig()); updateLinkProperties(networkAgent, new LinkProperties(networkAgent.linkProperties), null); diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index bb7f86233a..5e085ca293 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -291,13 +291,18 @@ public class NetworkAgentInfo implements Comparable { * *

If {@link NetworkMonitor#notifyNetworkCapabilitiesChanged(NetworkCapabilities)} fails, * the exception is logged but not reported to callers. + * + * @return the old capabilities of this network. */ - public void setNetworkCapabilities(NetworkCapabilities nc) { + public synchronized NetworkCapabilities getAndSetNetworkCapabilities( + @NonNull final NetworkCapabilities nc) { + final NetworkCapabilities oldNc = networkCapabilities; networkCapabilities = nc; final NetworkMonitorManager nm = mNetworkMonitor; if (nm != null) { nm.notifyNetworkCapabilitiesChanged(nc); } + return oldNc; } public ConnectivityService connService() { From 05cbe97f28e670e350f5fafe88a7e59c77309b18 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 9 Dec 2019 11:50:38 +0900 Subject: [PATCH 5/5] [NS A23.1] Minor cleanup The argument is always true. Test: ConnectivityServiceTest Change-Id: Ibebdae14e63e6baf74db054038ee575ec462f6d5 --- .../java/com/android/server/ConnectivityService.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 325bc19686..f8f685d939 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -5959,7 +5959,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // the change we're processing can't affect any requests, it can only affect the listens // on this network. We might have been called by rematchNetworkAndRequests when a // network changed foreground state. - processListenRequests(nai, true); + processListenRequests(nai); } else { // If the requestable capabilities have changed or the score changed, we can't have been // called by rematchNetworkAndRequests, so it's safe to start a rematch. @@ -6267,14 +6267,10 @@ public class ConnectivityService extends IConnectivityManager.Stub updateAllVpnsCapabilities(); } - private void processListenRequests(@NonNull NetworkAgentInfo nai, boolean capabilitiesChanged) { + private void processListenRequests(@NonNull final NetworkAgentInfo nai) { // For consistency with previous behaviour, send onLost callbacks before onAvailable. processNewlyLostListenRequests(nai); - - if (capabilitiesChanged) { - notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED); - } - + notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED); processNewlySatisfiedListenRequests(nai); }