From 88b2f9ecab113fdcb6515ee9490dcf68e962ad33 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 7c2f15e1a4cf8956ea41bb005bf53fd87d529fe4 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 bf021685ca215203163ddeec25bb973f10ccfe20 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 967190ecd0aec7113a05a7c334741bd9729e7c1e 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 00419abe9e9a7e53a2ee2f93dfd1dc8def599f85 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 8e382110b8932c169bbb2e28669d467a886d8d14 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 adf1aafb53e0c9cc03ee96f6db90bb03b6fcc883 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 04bc807898e4f68f2a42c85afe95a2519fe9fc09 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 b7ebc8a28f8894db3a1bc500af9b401a6542bb6e 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 {