From 3952fb150e69d5d2747e380dea22eb45c292a8a2 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 14:43:57 +0900 Subject: [PATCH 1/9] [NS A28] Move setting the default network out of the rematch loop. Bug: 113554781 Test: FrameworkNetTests NetworkStackTests Change-Id: I02d85f17bf0ea37ae173f306f5a47d7551773c3a --- .../android/server/ConnectivityService.java | 40 +++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 4b9925fe59..f7df8188ca 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -172,6 +172,7 @@ import com.android.internal.util.AsyncChannel; import com.android.internal.util.DumpUtils; import com.android.internal.util.IndentingPrintWriter; import com.android.internal.util.MessageUtils; +import com.android.internal.util.ObjectUtils; import com.android.internal.util.XmlUtils; import com.android.server.am.BatteryStatsService; import com.android.server.connectivity.AutodestructReference; @@ -6457,6 +6458,15 @@ public class ConnectivityService extends IConnectivityManager.Stub void addRematchedNetwork(@NonNull final NetworkBgStatePair network) { mRematchedNetworks.add(network); } + + // Will return null if this reassignment does not change the network assigned to + // the passed request, or if it changes this request to not have a satisfier any more. + @Nullable private NetworkAgentInfo getNewSatisfier(@NonNull final NetworkRequestInfo nri) { + for (final RequestReassignment event : getRequestReassignments()) { + if (nri == event.mRequest) return event.mNewNetwork; + } + return null; + } } private ArrayMap computeRequestReassignmentForNetwork( @@ -6523,8 +6533,6 @@ public class ConnectivityService extends IConnectivityManager.Stub @NonNull final NetworkAgentInfo newNetwork, final long now) { ensureRunningOnConnectivityServiceThread(); if (!newNetwork.everConnected) return; - boolean isNewDefault = false; - NetworkAgentInfo oldDefaultNetwork = null; changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(newNetwork, newNetwork.isBackgroundNetwork())); @@ -6566,8 +6574,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // netid->request mapping to each provider? sendUpdatedScoreToFactories(nri.request, newSatisfier); if (isDefaultRequest(nri)) { - isNewDefault = true; - oldDefaultNetwork = previousSatisfier; if (previousSatisfier != null) { mLingerMonitor.noteLingerDefaultNetwork(previousSatisfier, newSatisfier); } @@ -6604,17 +6610,6 @@ public class ConnectivityService extends IConnectivityManager.Stub callCallbackForRequest(nri, newNetwork, ConnectivityManager.CALLBACK_LOST, 0); } } - - if (isNewDefault) { - updateDataActivityTracking(newNetwork, oldDefaultNetwork); - // Notify system services that this network is up. - makeDefault(newNetwork); - // Log 0 -> X and Y -> X default network transitions, where X is the new default. - mDeps.getMetricsLogger().defaultNetworkMetrics().logDefaultNetworkEvent( - now, newNetwork, oldDefaultNetwork); - // Have a new default network, release the transition wakelock in - scheduleReleaseNetworkTransitionWakelock(); - } } /** @@ -6641,7 +6636,20 @@ public class ConnectivityService extends IConnectivityManager.Stub rematchNetworkAndRequests(changes, nai, now); } - final NetworkAgentInfo newDefaultNetwork = getDefaultNetwork(); + final NetworkRequestInfo defaultRequestInfo = mNetworkRequests.get(mDefaultRequest); + final NetworkAgentInfo newDefaultNetwork = ObjectUtils.getOrElse( + changes.getNewSatisfier(defaultRequestInfo), oldDefaultNetwork); + + if (oldDefaultNetwork != newDefaultNetwork) { + updateDataActivityTracking(newDefaultNetwork, oldDefaultNetwork); + // Notify system services that this network is up. + makeDefault(newDefaultNetwork); + // Log 0 -> X and Y -> X default network transitions, where X is the new default. + mDeps.getMetricsLogger().defaultNetworkMetrics().logDefaultNetworkEvent( + now, newDefaultNetwork, oldDefaultNetwork); + // Have a new default network, release the transition wakelock in + scheduleReleaseNetworkTransitionWakelock(); + } // Notify requested networks are available after the default net is switched, but // before LegacyTypeTracker sends legacy broadcasts From c1da930d29685f174f335f9238bf97821af298fb Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 15:55:14 +0900 Subject: [PATCH 2/9] [NS A29] Call LOST callbacks at the end of the rematch. Bug: 113554781 Test: FrameworksNetTests Change-Id: I72dd210a956545c75b3c702338af779e119d70e7 --- .../android/server/ConnectivityService.java | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index f7df8188ca..461c3ad215 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6549,6 +6549,8 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkRequestInfo nri = entry.getKey(); final NetworkAgentInfo previousSatisfier = nri.mSatisfier; final NetworkAgentInfo newSatisfier = entry.getValue(); + changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( + nri, previousSatisfier, newSatisfier)); if (newSatisfier != null) { if (VDBG) log("rematch for " + newSatisfier.name()); if (previousSatisfier != null) { @@ -6565,8 +6567,6 @@ public class ConnectivityService extends IConnectivityManager.Stub if (!newSatisfier.addRequest(nri.request)) { Slog.wtf(TAG, "BUG: " + newSatisfier.name() + " already has " + nri.request); } - changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( - nri, previousSatisfier, newSatisfier)); // Tell NetworkProviders about the new score, so they can stop // trying to connect if they know they cannot match it. // TODO - this could get expensive if we have a lot of requests for this @@ -6600,14 +6600,6 @@ public class ConnectivityService extends IConnectivityManager.Stub newNetwork.name() + " without updating mSatisfier or providers!"); } - // TODO: Technically, sending CALLBACK_LOST here is - // incorrect if there is a replacement network currently - // connected that can satisfy nri, which is a request - // (not a listen). However, the only capability that can both - // a) be requested and b) change is NET_CAPABILITY_TRUSTED, - // so this code is only incorrect for a network that loses - // the TRUSTED capability, which is a rare case. - callCallbackForRequest(nri, newNetwork, ConnectivityManager.CALLBACK_LOST, 0); } } } @@ -6657,6 +6649,16 @@ public class ConnectivityService extends IConnectivityManager.Stub changes.getRequestReassignments()) { if (null != event.mNewNetwork) { notifyNetworkAvailable(event.mNewNetwork, event.mRequest); + } else { + // TODO: Technically, sending CALLBACK_LOST here is + // incorrect if there is a replacement network currently + // connected that can satisfy nri, which is a request + // (not a listen). However, the only capability that can both + // a) be requested and b) change is NET_CAPABILITY_TRUSTED, + // so this code is only incorrect for a network that loses + // the TRUSTED capability, which is a rare case. + callCallbackForRequest(event.mRequest, event.mOldNetwork, + ConnectivityManager.CALLBACK_LOST, 0); } } From 387e7f5ee09f78e82f4cdde05fada49cc0805752 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 16:06:00 +0900 Subject: [PATCH 3/9] [NS A30] Note linger out of the rematch loop This doesn't have to be tested every time. Test: FrameworksNetTests Change-Id: Ic5702c8b4bd096860fe55c4d9e4c465703561911 --- .../core/java/com/android/server/ConnectivityService.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 461c3ad215..9b0be062e0 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6573,11 +6573,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // network. Think about if there is a way to reduce this. Push // netid->request mapping to each provider? sendUpdatedScoreToFactories(nri.request, newSatisfier); - if (isDefaultRequest(nri)) { - if (previousSatisfier != null) { - mLingerMonitor.noteLingerDefaultNetwork(previousSatisfier, newSatisfier); - } - } } else { // If "newNetwork" is listed as satisfying "nri" but no longer satisfies "nri", // mark it as no longer satisfying "nri". Because networks are processed by @@ -6633,6 +6628,9 @@ public class ConnectivityService extends IConnectivityManager.Stub changes.getNewSatisfier(defaultRequestInfo), oldDefaultNetwork); if (oldDefaultNetwork != newDefaultNetwork) { + if (oldDefaultNetwork != null) { + mLingerMonitor.noteLingerDefaultNetwork(oldDefaultNetwork, newDefaultNetwork); + } updateDataActivityTracking(newDefaultNetwork, oldDefaultNetwork); // Notify system services that this network is up. makeDefault(newDefaultNetwork); From a2d0a47e04dfe5711743386d1499c146829728b7 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 20:30:59 +0900 Subject: [PATCH 4/9] [NS A31] Simplification The condition this is testing for cannot actually be false. The only place where the code writes a null value into this map is at the end of computeRequestReassignmentForNetwork : reassignedRequests.put(nri, null). This proves the code the if() block, which proves that newNetwork.isSatisfyingRequest(nri.request.requestId) is true. By definition newNetwork.isSatisfyingRequest(nri) implies that nri.mSatifier == newNetwork, which proves that previousSatisfier == newNetwork whenever newSatisfier is null. Fixes: 146482072 Test: FrameworksNetTests Change-Id: Ifd6faedce7d49757b82a5f341076ab208b0ccfcb --- .../java/com/android/server/ConnectivityService.java | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 9b0be062e0..36afbfbaad 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6586,15 +6586,9 @@ public class ConnectivityService extends IConnectivityManager.Stub " request " + nri.request.requestId); } newNetwork.removeRequest(nri.request.requestId); - if (previousSatisfier == newNetwork) { - nri.mSatisfier = null; - if (isDefaultRequest(nri)) mDefaultNetworkNai = null; - sendUpdatedScoreToFactories(nri.request, null); - } else { - Slog.wtf(TAG, "BUG: Removing request " + nri.request.requestId + " from " + - newNetwork.name() + - " without updating mSatisfier or providers!"); - } + nri.mSatisfier = null; + if (isDefaultRequest(nri)) mDefaultNetworkNai = null; + sendUpdatedScoreToFactories(nri.request, null); } } } From 744fd0c773416552c33d3d54ee069d8bebc64272 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 20:37:01 +0900 Subject: [PATCH 5/9] [NS A32] More simplification Test: FrameworksNetTests Change-Id: Ib713c4ae0100c8c242bbba87b2881311c5761869 --- .../com/android/server/ConnectivityService.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 36afbfbaad..0e8fb97831 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6198,7 +6198,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - private void sendUpdatedScoreToFactories(NetworkRequest networkRequest, NetworkAgentInfo nai) { + private void sendUpdatedScoreToFactories(@NonNull NetworkRequest networkRequest, + @Nullable NetworkAgentInfo nai) { int score = 0; int serial = 0; if (nai != null) { @@ -6567,12 +6568,6 @@ public class ConnectivityService extends IConnectivityManager.Stub if (!newSatisfier.addRequest(nri.request)) { Slog.wtf(TAG, "BUG: " + newSatisfier.name() + " already has " + nri.request); } - // Tell NetworkProviders about the new score, so they can stop - // trying to connect if they know they cannot match it. - // TODO - this could get expensive if we have a lot of requests for this - // network. Think about if there is a way to reduce this. Push - // netid->request mapping to each provider? - sendUpdatedScoreToFactories(nri.request, newSatisfier); } else { // If "newNetwork" is listed as satisfying "nri" but no longer satisfies "nri", // mark it as no longer satisfying "nri". Because networks are processed by @@ -6588,8 +6583,12 @@ public class ConnectivityService extends IConnectivityManager.Stub newNetwork.removeRequest(nri.request.requestId); nri.mSatisfier = null; if (isDefaultRequest(nri)) mDefaultNetworkNai = null; - sendUpdatedScoreToFactories(nri.request, null); } + // Tell NetworkProviders about the new score, so they can stop + // trying to connect if they know they cannot match it. + // TODO - this could get expensive if there are a lot of outstanding requests for this + // network. Think of a way to reduce this. Push netid->request mapping to each factory? + sendUpdatedScoreToFactories(nri.request, newSatisfier); } } From 337a8035ef866378983044fc820bb9d004088e09 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 20:45:30 +0900 Subject: [PATCH 6/9] [NS A33] Unify changing the default network makeDefault() will be called in rematchAllNetworksAndRequests in all cases and its first job is to set mDefaultNetwork. The old code checks if the currently processing request is the default request and assigns mDefaultNetwork if it is, doing it earlier than in the new code iff the new default is not null. However there is no good reason to assign this member earlier in the non-null case than in the null case, it's simpler if the same code path is used in both cases. mDefaultNetwork is also not used between the old place where it was set and the new place where it is set, so changing the timing of the assignment has no observable side effects. Test: ConnectivityServiceTest Change-Id: Id47c19b73650ba66bff73b07edb8fd95c707e699 --- .../android/server/ConnectivityService.java | 37 ++++++++++++------- .../server/connectivity/LingerMonitor.java | 29 ++++++++++++--- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 0e8fb97831..326ab2eb0a 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -172,7 +172,6 @@ import com.android.internal.util.AsyncChannel; import com.android.internal.util.DumpUtils; import com.android.internal.util.IndentingPrintWriter; import com.android.internal.util.MessageUtils; -import com.android.internal.util.ObjectUtils; import com.android.internal.util.XmlUtils; import com.android.server.am.BatteryStatsService; import com.android.server.connectivity.AutodestructReference; @@ -6367,20 +6366,28 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - private void makeDefault(@NonNull final NetworkAgentInfo newNetwork) { + private void makeDefault(@Nullable final NetworkAgentInfo newNetwork) { if (DBG) log("Switching to new default network: " + newNetwork); + mDefaultNetworkNai = newNetwork; + try { - mNMS.setDefaultNetId(newNetwork.network.netId); + if (null != newNetwork) { + mNMS.setDefaultNetId(newNetwork.network.netId); + } else { + mNMS.clearDefaultNetId(); + } } catch (Exception e) { loge("Exception setting default network :" + e); } - mDefaultNetworkNai = newNetwork; notifyLockdownVpn(newNetwork); - handleApplyDefaultProxy(newNetwork.linkProperties.getHttpProxy()); - updateTcpBufferSizes(newNetwork.linkProperties.getTcpBufferSizes()); - mDnsManager.setDefaultDnsSystemProperties(newNetwork.linkProperties.getDnsServers()); + handleApplyDefaultProxy(null != newNetwork + ? newNetwork.linkProperties.getHttpProxy() : null); + updateTcpBufferSizes(null != newNetwork + ? newNetwork.linkProperties.getTcpBufferSizes() : null); + mDnsManager.setDefaultDnsSystemProperties(null != newNetwork + ? newNetwork.linkProperties.getDnsServers() : Collections.EMPTY_LIST); notifyIfacesChangedForNetworkStats(); // Fix up the NetworkCapabilities of any VPNs that don't specify underlying networks. updateAllVpnsCapabilities(); @@ -6461,10 +6468,11 @@ public class ConnectivityService extends IConnectivityManager.Stub } // Will return null if this reassignment does not change the network assigned to - // the passed request, or if it changes this request to not have a satisfier any more. - @Nullable private NetworkAgentInfo getNewSatisfier(@NonNull final NetworkRequestInfo nri) { + // the passed request. + @Nullable + private RequestReassignment getReassignment(@NonNull final NetworkRequestInfo nri) { for (final RequestReassignment event : getRequestReassignments()) { - if (nri == event.mRequest) return event.mNewNetwork; + if (nri == event.mRequest) return event; } return null; } @@ -6582,7 +6590,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } newNetwork.removeRequest(nri.request.requestId); nri.mSatisfier = null; - if (isDefaultRequest(nri)) mDefaultNetworkNai = null; } // Tell NetworkProviders about the new score, so they can stop // trying to connect if they know they cannot match it. @@ -6617,15 +6624,17 @@ public class ConnectivityService extends IConnectivityManager.Stub } final NetworkRequestInfo defaultRequestInfo = mNetworkRequests.get(mDefaultRequest); - final NetworkAgentInfo newDefaultNetwork = ObjectUtils.getOrElse( - changes.getNewSatisfier(defaultRequestInfo), oldDefaultNetwork); + final NetworkReassignment.RequestReassignment reassignment = + changes.getReassignment(defaultRequestInfo); + final NetworkAgentInfo newDefaultNetwork = + null != reassignment ? reassignment.mNewNetwork : oldDefaultNetwork; if (oldDefaultNetwork != newDefaultNetwork) { if (oldDefaultNetwork != null) { mLingerMonitor.noteLingerDefaultNetwork(oldDefaultNetwork, newDefaultNetwork); } updateDataActivityTracking(newDefaultNetwork, oldDefaultNetwork); - // Notify system services that this network is up. + // Notify system services of the new default. makeDefault(newDefaultNetwork); // Log 0 -> X and Y -> X default network transitions, where X is the new default. mDeps.getMetricsLogger().defaultNetworkMetrics().logDefaultNetworkEvent( diff --git a/services/core/java/com/android/server/connectivity/LingerMonitor.java b/services/core/java/com/android/server/connectivity/LingerMonitor.java index 929dfc4d15..7071510598 100644 --- a/services/core/java/com/android/server/connectivity/LingerMonitor.java +++ b/services/core/java/com/android/server/connectivity/LingerMonitor.java @@ -16,6 +16,10 @@ package com.android.server.connectivity; +import static android.net.ConnectivityManager.NETID_UNSET; + +import android.annotation.NonNull; +import android.annotation.Nullable; import android.app.PendingIntent; import android.content.ComponentName; import android.content.Context; @@ -27,18 +31,16 @@ import android.text.TextUtils; import android.text.format.DateUtils; import android.util.Log; import android.util.SparseArray; -import android.util.SparseIntArray; import android.util.SparseBooleanArray; -import java.util.Arrays; -import java.util.HashMap; +import android.util.SparseIntArray; import com.android.internal.R; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.MessageUtils; -import com.android.server.connectivity.NetworkNotificationManager; import com.android.server.connectivity.NetworkNotificationManager.NotificationType; -import static android.net.ConnectivityManager.NETID_UNSET; +import java.util.Arrays; +import java.util.HashMap; /** * Class that monitors default network linger events and possibly notifies the user of network @@ -206,8 +208,19 @@ public class LingerMonitor { mEverNotified.put(fromNai.network.netId, true); } + /** + * Put up or dismiss a notification or toast for of a change in the default network if needed. + * + * Putting up a notification when switching from no network to some network is not supported + * and as such this method can't be called with a null |fromNai|. It can be called with a + * null |toNai| if there isn't a default network any more. + * + * @param fromNai switching from this NAI + * @param toNai switching to this NAI + */ // The default network changed from fromNai to toNai due to a change in score. - public void noteLingerDefaultNetwork(NetworkAgentInfo fromNai, NetworkAgentInfo toNai) { + public void noteLingerDefaultNetwork(@NonNull final NetworkAgentInfo fromNai, + @Nullable final NetworkAgentInfo toNai) { if (VDBG) { Log.d(TAG, "noteLingerDefaultNetwork from=" + fromNai.name() + " everValidated=" + fromNai.everValidated + @@ -221,6 +234,10 @@ public class LingerMonitor { // Internet access). maybeStopNotifying(fromNai); + // If the network was simply lost (either because it disconnected or because it stopped + // being the default with no replacement), then don't show a notification. + if (null == toNai) return; + // If this network never validated, don't notify. Otherwise, we could do things like: // // 1. Unvalidated wifi connects. From b0e2af767e665e803fc5c07cc8f163ff86f3ee68 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 21:26:27 +0900 Subject: [PATCH 7/9] [NS A34] Still more simplification Test: ConnectivityServiceTest Change-Id: I85247411eb8fdfb3eae0e7c309ea9537e41cfb80 --- services/core/java/com/android/server/ConnectivityService.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 326ab2eb0a..51148b270e 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6572,7 +6572,6 @@ public class ConnectivityService extends IConnectivityManager.Stub if (VDBG || DDBG) log(" accepting network in place of null"); } newSatisfier.unlingerRequest(nri.request); - nri.mSatisfier = newSatisfier; if (!newSatisfier.addRequest(nri.request)) { Slog.wtf(TAG, "BUG: " + newSatisfier.name() + " already has " + nri.request); } @@ -6589,8 +6588,8 @@ public class ConnectivityService extends IConnectivityManager.Stub " request " + nri.request.requestId); } newNetwork.removeRequest(nri.request.requestId); - nri.mSatisfier = null; } + nri.mSatisfier = newSatisfier; // Tell NetworkProviders about the new score, so they can stop // trying to connect if they know they cannot match it. // TODO - this could get expensive if there are a lot of outstanding requests for this From af4bcc61b780d43f758b6c3b1efdfb8349e405cb Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 21:35:40 +0900 Subject: [PATCH 8/9] [NS A35] Send updated scores to factories at the end. Test: ConnectivityServiceTests Change-Id: I66c4e8f52e11bc7e199dd8c12d43b0b136a21f19 --- .../android/server/ConnectivityService.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 51148b270e..0e17b23249 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6199,11 +6199,14 @@ public class ConnectivityService extends IConnectivityManager.Stub private void sendUpdatedScoreToFactories(@NonNull NetworkRequest networkRequest, @Nullable NetworkAgentInfo nai) { - int score = 0; - int serial = 0; + final int score; + final int serial; if (nai != null) { score = nai.getCurrentScore(); serial = nai.factorySerialNumber; + } else { + score = 0; + serial = 0; } if (VDBG || DDBG){ log("sending new Min Network Score(" + score + "): " + networkRequest.toString()); @@ -6590,11 +6593,6 @@ public class ConnectivityService extends IConnectivityManager.Stub newNetwork.removeRequest(nri.request.requestId); } nri.mSatisfier = newSatisfier; - // Tell NetworkProviders about the new score, so they can stop - // trying to connect if they know they cannot match it. - // TODO - this could get expensive if there are a lot of outstanding requests for this - // network. Think of a way to reduce this. Push netid->request mapping to each factory? - sendUpdatedScoreToFactories(nri.request, newSatisfier); } } @@ -6646,6 +6644,12 @@ public class ConnectivityService extends IConnectivityManager.Stub // before LegacyTypeTracker sends legacy broadcasts for (final NetworkReassignment.RequestReassignment event : changes.getRequestReassignments()) { + // Tell NetworkProviders about the new score, so they can stop + // trying to connect if they know they cannot match it. + // TODO - this could get expensive if there are a lot of outstanding requests for this + // network. Think of a way to reduce this. Push netid->request mapping to each factory? + sendUpdatedScoreToFactories(event.mRequest.request, event.mNewNetwork); + if (null != event.mNewNetwork) { notifyNetworkAvailable(event.mNewNetwork, event.mRequest); } else { From 3b270131c0d708868c000524671a2f51850fbc44 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 22:13:37 +0900 Subject: [PATCH 9/9] [NS A36] Add a test for lost trusted capability This bug will be drive-by fixed by the next refactoring, so set up a test to see the difference. Bug: 113554781 Test: this Change-Id: Icb062ffbae904d1836a4a16fc5395687c3eda7b6 --- .../server/ConnectivityServiceTest.java | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 50f1bbeed0..1442b31aa8 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5741,6 +5741,40 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(defaultCallback); } + @Test + public final void testLoseTrusted() throws Exception { + final NetworkRequest trustedRequest = new NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_TRUSTED) + .build(); + final TestNetworkCallback trustedCallback = new TestNetworkCallback(); + mCm.requestNetwork(trustedRequest, trustedCallback); + + mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); + mCellNetworkAgent.connect(true); + trustedCallback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent); + verify(mNetworkManagementService).setDefaultNetId(eq(mCellNetworkAgent.getNetwork().netId)); + + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); + mWiFiNetworkAgent.connect(true); + trustedCallback.expectAvailableDoubleValidatedCallbacks(mWiFiNetworkAgent); + verify(mNetworkManagementService).setDefaultNetId(eq(mWiFiNetworkAgent.getNetwork().netId)); + + mWiFiNetworkAgent.removeCapability(NET_CAPABILITY_TRUSTED); + // There is currently a bug where losing the TRUSTED capability will send a LOST + // callback to requests before the available callback, in spite of the semantics + // of the requests dictating this should not happen. This is considered benign, but + // ideally should be fixed in the future. + trustedCallback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); + trustedCallback.expectAvailableCallbacksValidated(mCellNetworkAgent); + verify(mNetworkManagementService).setDefaultNetId(eq(mCellNetworkAgent.getNetwork().netId)); + + mCellNetworkAgent.removeCapability(NET_CAPABILITY_TRUSTED); + trustedCallback.expectCallback(CallbackEntry.LOST, mCellNetworkAgent); + verify(mNetworkManagementService).clearDefaultNetId(); + + mCm.unregisterNetworkCallback(trustedCallback); + } + @Ignore // 40%+ flakiness : figure out why and re-enable. @Test public final void testBatteryStatsNetworkType() throws Exception {