From b5becbca0a87eb810cbd8cb843d1be2a172f6106 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 5 Mar 2021 19:18:14 +0900 Subject: [PATCH] Fix a bug where listen callbacks would not be called MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NetworkAgentInfos cache the list of requests they satisfy, and that list is used to send callbacks. Therefore, when the TRACK_DEFAULTs are copied, this list needs to be updated. The best way to do this is to figure out what was the old active request and find which requests corresponds to it in the new list, and then upon registering adding the active request to the relevant satisfier if present. A few other ways can be considered like replacing the request as it gets added, but this would temporarily increase the number of callbacks allocated to the app and risks crashing it for no good reason ; furthermore the call to remove would have to be eschewed somehow for those requests that are replaced. This is much simpler. Test: new test for this. This also passes the future tests for per-profile default network preference. Change-Id: I001351e5c478c2c77cbf2844abca77b205291778 --- .../android/server/ConnectivityService.java | 35 ++++++++++++++----- .../server/ConnectivityServiceTest.java | 32 ++++++++++++++--- 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 2c9837d38d..b95a2defa5 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3651,6 +3651,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } } } + // If this NRI has a satisfier already, it is replacing an older request that + // has been removed. Track it. + final NetworkRequest activeRequest = nri.getActiveRequest(); + if (null != activeRequest) { + // If there is an active request, then for sure there is a satisfier. + nri.getSatisfier().addRequest(activeRequest); + } } rematchAllNetworksAndRequests(); @@ -5271,14 +5278,26 @@ public class ConnectivityService extends IConnectivityManager.Stub ensureAllNetworkRequestsHaveType(r); mRequests = initializeRequests(r); mNetworkRequestForCallback = nri.getNetworkRequestForCallback(); - // Note here that the satisfier may have corresponded to an old request, that - // this code doesn't try to take over. While it is a small discrepancy in the - // structure of these requests, it will be fixed by the next rematch and it's - // not as bad as having an NRI not storing its real satisfier. - // Fixing this discrepancy would require figuring out in the copying code what - // is the new request satisfied by this, which is a bit complex and not very - // useful as no code is using it until rematch fixes it. - mSatisfier = nri.mSatisfier; + final NetworkAgentInfo satisfier = nri.getSatisfier(); + if (null != satisfier) { + // If the old NRI was satisfied by an NAI, then it may have had an active request. + // The active request is necessary to figure out what callbacks to send, in + // particular then a network updates its capabilities. + // As this code creates a new NRI with a new set of requests, figure out which of + // the list of requests should be the active request. It is always the first + // request of the list that can be satisfied by the satisfier since the order of + // requests is a priority order. + // Note even in the presence of a satisfier there may not be an active request, + // when the satisfier is the no-service network. + NetworkRequest activeRequest = null; + for (final NetworkRequest candidate : r) { + if (candidate.canBeSatisfiedBy(satisfier.networkCapabilities)) { + activeRequest = candidate; + break; + } + } + setSatisfier(satisfier, activeRequest); + } mMessenger = nri.mMessenger; mBinder = nri.mBinder; mPid = nri.mPid; diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 2546580e18..6c603a4460 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -72,6 +72,7 @@ import static android.net.NetworkCapabilities.NET_CAPABILITY_OEM_PRIVATE; import static android.net.NetworkCapabilities.NET_CAPABILITY_PARTIAL_CONNECTIVITY; import static android.net.NetworkCapabilities.NET_CAPABILITY_RCS; import static android.net.NetworkCapabilities.NET_CAPABILITY_SUPL; +import static android.net.NetworkCapabilities.NET_CAPABILITY_TEMPORARILY_NOT_METERED; import static android.net.NetworkCapabilities.NET_CAPABILITY_TRUSTED; import static android.net.NetworkCapabilities.NET_CAPABILITY_VALIDATED; import static android.net.NetworkCapabilities.NET_CAPABILITY_WIFI_P2P; @@ -5574,7 +5575,7 @@ public class ConnectivityServiceTest { reset(mStatsManager); // Temp metered change shouldn't update ifaces - mCellNetworkAgent.addCapability(NetworkCapabilities.NET_CAPABILITY_TEMPORARILY_NOT_METERED); + mCellNetworkAgent.addCapability(NET_CAPABILITY_TEMPORARILY_NOT_METERED); waitForIdle(); verify(mStatsManager, never()).notifyNetworkStatus(eq(Arrays.asList(onlyCell)), any(List.class), eq(MOBILE_IFNAME), any(List.class)); @@ -10647,7 +10648,7 @@ public class ConnectivityServiceTest { null, null); - // default NCs will be unregistered in tearDown + // default callbacks will be unregistered in tearDown } /** @@ -10704,7 +10705,7 @@ public class ConnectivityServiceTest { null, mService.mNoServiceNetwork.network()); - // default NCs will be unregistered in tearDown + // default callbacks will be unregistered in tearDown } /** @@ -10763,7 +10764,7 @@ public class ConnectivityServiceTest { null, mService.mNoServiceNetwork.network()); - // default NCs will be unregistered in tearDown + // default callbacks will be unregistered in tearDown } /** @@ -10822,7 +10823,28 @@ public class ConnectivityServiceTest { null, mService.mNoServiceNetwork.network()); - // default NCs will be unregistered in tearDown + // default callbacks will be unregistered in tearDown + } + + @Test + public void testCapabilityWithOemNetworkPreference() throws Exception { + @OemNetworkPreferences.OemNetworkPreference final int networkPref = + OemNetworkPreferences.OEM_NETWORK_PREFERENCE_OEM_PRIVATE_ONLY; + setupMultipleDefaultNetworksForOemNetworkPreferenceNotCurrentUidTest(networkPref); + registerDefaultNetworkCallbacks(); + + setOemNetworkPreferenceAgentConnected(TRANSPORT_CELLULAR, true); + + mSystemDefaultNetworkCallback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent); + mDefaultNetworkCallback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent); + + mCellNetworkAgent.addCapability(NET_CAPABILITY_TEMPORARILY_NOT_METERED); + mSystemDefaultNetworkCallback.expectCapabilitiesThat(mCellNetworkAgent, nc -> + nc.hasCapability(NET_CAPABILITY_TEMPORARILY_NOT_METERED)); + mDefaultNetworkCallback.expectCapabilitiesThat(mCellNetworkAgent, nc -> + nc.hasCapability(NET_CAPABILITY_TEMPORARILY_NOT_METERED)); + + // default callbacks will be unregistered in tearDown } @Test