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/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 mobileRequestRequired = tryCell && (dunRequired || !autoUpstream);
-        final boolean mobileRequestIsRequired = tryCell && (dunRequired || !autoUpstream);
         final boolean dunRequiredChanged = (mDunRequired != dunRequired);
 
         mTryCell = tryCell;
         mDunRequired = dunRequired;
         mAutoUpstream = autoUpstream;
 
-        if (dunRequiredChanged && mobileNetworkRequested()) {
+        if (mobileRequestRequired && !mobileNetworkRequested()) {
-            releaseMobileNetworkRequest();
             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);

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(
+        mUNM = new UpstreamNetworkMonitor(mCM, mSM, mLog, EVENT_UNM_UPDATE);
-                (ConnectivityManager) 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
+        assertEquals(1, mCM.requested.size());
-        // triggered by code over in Tethering, but once that has been moved
+        assertTrue(isDunRequested());
-        // into UNM we should test for this here.
 
         // [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