From 9f0e8ddad531daad2357207d6a20422198742dcd 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/internal/util/ObjectUtils.java | 8 ++++ .../android/server/ConnectivityService.java | 40 +++++++++++-------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/core/java/com/android/internal/util/ObjectUtils.java b/core/java/com/android/internal/util/ObjectUtils.java index 59e5a6402fb82..a6b345fadbbe4 100644 --- a/core/java/com/android/internal/util/ObjectUtils.java +++ b/core/java/com/android/internal/util/ObjectUtils.java @@ -36,4 +36,12 @@ public class ObjectUtils { return (b != null) ? -1 : 0; } } + + /** + * Returns its first argument if non-null, and the second otherwise. + */ + @Nullable + public static T getOrElse(@Nullable final T object, @Nullable final T otherwise) { + return null != object ? object : otherwise; + } } diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 4b9925fe597be..f7df8188ca091 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 f2cbe683faf1e6c83ba2cd14d984067975cf5046 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 f7df8188ca091..461c3ad215d54 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 59954c819e8c86359bd7d22c21b3eda8b3c650af 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 461c3ad215d54..9b0be062e0a1b 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 9956bada813f912df6b72198c1996fcc6af2419e 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 9b0be062e0a1b..36afbfbaad5a8 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 5b26b62c899648a549a065f6fd3c89435e9cfe29 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 36afbfbaad5a8..0e8fb978315aa 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 6d00610f7b4a665643e42d63aec5819484467d80 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 0e8fb978315aa..326ab2eb0ac1f 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 929dfc4d15111..707151059869e 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 13bab23472c86216bdb6273fc865b0ca5358d704 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 326ab2eb0ac1f..51148b270e252 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 ce454807e6c7a663d4d6b684e00e517f9b1b59ab 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 51148b270e252..0e17b23249aed 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 143124ba0da86deb57596efab0d2ad6224ce8538 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 50f1bbeed0c5b..1442b31aa8f1d 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 {