From 2bcde0fd97ea4370c0812ada2c7a8aeff4230e4a Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 22 Jan 2021 00:33:40 +0900 Subject: [PATCH] Make Tethering file NetworkRequests only if needed. Currently, Tethering files NetworkRequests even when config_tether_upstream_automatic is enabled. This is incorrect: when the automatic upstream selection is enabled, the tethering upstream should always follow the default network and there is no need to file any requests. These requests are harmful when tethering is not using cellular as its upstream, because: - If the device does not use mobile data always on, the request causes the cellular network to be brought up, causing power draw. - Even if the device does use mobile data always on, the request causes the cellular network to come to the foreground, which allows all apps to access it, causing potential data usage. Amend the existing testGetCurrentPreferredUpstream to cover these changes, by making that test case always set automatic upstream mode. This does not result in any loss of meaningful test coverage, because getCurrentPreferredUpstream is only ever called when chooseUpstreamAutomatically is enabled. Bug: 173068192 Test: atest TetheringTests Change-Id: I068a5338699a3ed04f24f97f785ea89ff5890e50 --- .../tethering/UpstreamNetworkMonitor.java | 22 +++++++------- .../tethering/UpstreamNetworkMonitorTest.java | 30 ++++++++++++++----- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java b/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java index eb22448a73..513f07c735 100644 --- a/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java +++ b/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java @@ -199,17 +199,22 @@ public class UpstreamNetworkMonitor { private void reevaluateUpstreamRequirements(boolean tryCell, boolean autoUpstream, boolean dunRequired) { - final boolean mobileRequestWasRequired = mTryCell && (mDunRequired || !mAutoUpstream); - final boolean mobileRequestIsRequired = tryCell && (dunRequired || !autoUpstream); + final boolean mobileRequestRequired = tryCell && (dunRequired || !autoUpstream); final boolean dunRequiredChanged = (mDunRequired != dunRequired); mTryCell = tryCell; mDunRequired = dunRequired; mAutoUpstream = autoUpstream; - if (dunRequiredChanged && mobileNetworkRequested()) { - releaseMobileNetworkRequest(); + if (mobileRequestRequired && !mobileNetworkRequested()) { registerMobileNetworkRequest(); + } else if (mobileNetworkRequested() && !mobileRequestRequired) { + releaseMobileNetworkRequest(); + } else if (mobileNetworkRequested() && dunRequiredChanged) { + releaseMobileNetworkRequest(); + if (mobileRequestRequired) { + registerMobileNetworkRequest(); + } } } @@ -221,11 +226,6 @@ public class UpstreamNetworkMonitor { */ public void setTryCell(boolean tryCell) { reevaluateUpstreamRequirements(tryCell, mAutoUpstream, mDunRequired); - if (tryCell) { - registerMobileNetworkRequest(); - } else { - releaseMobileNetworkRequest(); - } } /** Informs UpstreamNetworkMonitor of upstream configuration parameters. */ @@ -274,7 +274,9 @@ public class UpstreamNetworkMonitor { // TODO: Change the timeout from 0 (no onUnavailable callback) to some // moderate callback timeout. This might be useful for updating some UI. // Additionally, we log a message to aid in any subsequent debugging. - mLog.i("requesting mobile upstream network: " + mobileUpstreamRequest); + mLog.i("requesting mobile upstream network: " + mobileUpstreamRequest + + " mTryCell=" + mTryCell + " mAutoUpstream=" + mAutoUpstream + + " mDunRequired=" + mDunRequired); cm().requestNetwork(mobileUpstreamRequest, 0, legacyType, mHandler, mMobileNetworkCallback); diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java index 4376b61a48..43d32b5e36 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java @@ -41,7 +41,6 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import android.content.Context; -import android.net.ConnectivityManager; import android.net.ConnectivityManager.NetworkCallback; import android.net.IConnectivityManager; import android.net.IpPrefix; @@ -111,8 +110,7 @@ public class UpstreamNetworkMonitorTest { mCM = spy(new TestConnectivityManager(mContext, mCS, sDefaultRequest)); mSM = new TestStateMachine(); - mUNM = new UpstreamNetworkMonitor( - (ConnectivityManager) mCM, mSM, mLog, EVENT_UNM_UPDATE); + mUNM = new UpstreamNetworkMonitor(mCM, mSM, mLog, EVENT_UNM_UPDATE); } @After public void tearDown() throws Exception { @@ -373,38 +371,44 @@ public class UpstreamNetworkMonitorTest { public void testGetCurrentPreferredUpstream() throws Exception { mUNM.startTrackDefaultNetwork(sDefaultRequest, mEntitleMgr); mUNM.startObserveAllNetworks(); - mUNM.setUpstreamConfig(false /* autoUpstream */, false /* dunRequired */); + mUNM.setUpstreamConfig(true /* autoUpstream */, false /* dunRequired */); + mUNM.setTryCell(true); // [0] Mobile connects, DUN not required -> mobile selected. final TestNetworkAgent cellAgent = new TestNetworkAgent(mCM, CELL_CAPABILITIES); cellAgent.fakeConnect(); mCM.makeDefaultNetwork(cellAgent); assertEquals(cellAgent.networkId, mUNM.getCurrentPreferredUpstream().network); + assertEquals(0, mCM.requested.size()); // [1] Mobile connects but not permitted -> null selected when(mEntitleMgr.isCellularUpstreamPermitted()).thenReturn(false); assertEquals(null, mUNM.getCurrentPreferredUpstream()); when(mEntitleMgr.isCellularUpstreamPermitted()).thenReturn(true); + assertEquals(0, mCM.requested.size()); // [2] WiFi connects but not validated/promoted to default -> mobile selected. final TestNetworkAgent wifiAgent = new TestNetworkAgent(mCM, WIFI_CAPABILITIES); wifiAgent.fakeConnect(); assertEquals(cellAgent.networkId, mUNM.getCurrentPreferredUpstream().network); + assertEquals(0, mCM.requested.size()); // [3] WiFi validates and is promoted to the default network -> WiFi selected. mCM.makeDefaultNetwork(wifiAgent); assertEquals(wifiAgent.networkId, mUNM.getCurrentPreferredUpstream().network); + assertEquals(0, mCM.requested.size()); // [4] DUN required, no other changes -> WiFi still selected mUNM.setUpstreamConfig(false /* autoUpstream */, true /* dunRequired */); assertEquals(wifiAgent.networkId, mUNM.getCurrentPreferredUpstream().network); + assertEquals(1, mCM.requested.size()); + assertTrue(isDunRequested()); // [5] WiFi no longer validated, mobile becomes default, DUN required -> null selected. mCM.makeDefaultNetwork(cellAgent); assertEquals(null, mUNM.getCurrentPreferredUpstream()); - // TODO: make sure that a DUN request has been filed. This is currently - // triggered by code over in Tethering, but once that has been moved - // into UNM we should test for this here. + assertEquals(1, mCM.requested.size()); + assertTrue(isDunRequested()); // [6] DUN network arrives -> DUN selected final TestNetworkAgent dunAgent = new TestNetworkAgent(mCM, CELL_CAPABILITIES); @@ -412,10 +416,22 @@ public class UpstreamNetworkMonitorTest { dunAgent.networkCapabilities.removeCapability(NET_CAPABILITY_INTERNET); dunAgent.fakeConnect(); assertEquals(dunAgent.networkId, mUNM.getCurrentPreferredUpstream().network); + assertEquals(1, mCM.requested.size()); // [7] Mobile is not permitted -> null selected when(mEntitleMgr.isCellularUpstreamPermitted()).thenReturn(false); assertEquals(null, mUNM.getCurrentPreferredUpstream()); + assertEquals(1, mCM.requested.size()); + + // [7] Mobile is permitted again -> DUN selected + when(mEntitleMgr.isCellularUpstreamPermitted()).thenReturn(true); + assertEquals(dunAgent.networkId, mUNM.getCurrentPreferredUpstream().network); + assertEquals(1, mCM.requested.size()); + + // [8] DUN no longer required -> request is withdrawn + mUNM.setUpstreamConfig(true /* autoUpstream */, false /* dunRequired */); + assertEquals(0, mCM.requested.size()); + assertFalse(isDunRequested()); } @Test