Merge "Track default upstream when system is ready" am: d9eeba6fd1 am: 8e4f9929e7

am: 25e0a60024

Change-Id: I72c041051f3bd1735a438f8ce7ded83cf26136f9

5 files changed, 97 insertions, 84 deletions

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index eda9fe15fe36..89194e43bf95 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -1968,6 +1968,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
     void systemReady() {
         mProxyTracker.loadGlobalProxy();
         registerNetdEventCallback();
+        mTethering.systemReady();
 
         synchronized (this) {
             mSystemReady = true;
diff --git a/services/core/java/com/android/server/connectivity/Tethering.java b/services/core/java/com/android/server/connectivity/Tethering.java
index d75601be23e3..9dfdddbea18a 100644
--- a/services/core/java/com/android/server/connectivity/Tethering.java
+++ b/services/core/java/com/android/server/connectivity/Tethering.java
@@ -1382,7 +1382,7 @@ public class Tethering extends BaseNetworkObserver {
                     return;
                 }
 
-                mUpstreamNetworkMonitor.start(mDeps.getDefaultNetworkRequest());
+                mUpstreamNetworkMonitor.startObserveAllNetworks();
 
                 // TODO: De-duplicate with updateUpstreamWanted() below.
                 if (upstreamWanted()) {
@@ -1658,6 +1658,10 @@ public class Tethering extends BaseNetworkObserver {
         }
     }
 
+    public void systemReady() {
+        mUpstreamNetworkMonitor.startTrackDefaultNetwork(mDeps.getDefaultNetworkRequest());
+    }
+
     @Override
     public void dump(FileDescriptor fd, PrintWriter writer, String[] args) {
         // Binder.java closes the resource for us.
diff --git a/services/core/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitor.java b/services/core/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitor.java
index 3e5d5aa6ca54..3ac311b3e13a 100644
--- a/services/core/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitor.java
+++ b/services/core/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitor.java
@@ -55,10 +55,13 @@ import java.util.Set;
  * A class to centralize all the network and link properties information
  * pertaining to the current and any potential upstream network.
  *
- * Calling #start() registers two callbacks: one to track the system default
- * network and a second to observe all networks.  The latter is necessary
- * while the expression of preferred upstreams remains a list of legacy
- * connectivity types.  In future, this can be revisited.
+ * The owner of UNM gets it to register network callbacks by calling the
+ * following methods :
+ * Calling #startTrackDefaultNetwork() to track the system default network.
+ * Calling #startObserveAllNetworks() to observe all networks. Listening all
+ * networks is necessary while the expression of preferred upstreams remains
+ * a list of legacy connectivity types.  In future, this can be revisited.
+ * Calling #registerMobileNetworkRequest() to bring up mobile DUN/HIPRI network.
  *
  * The methods and data members of this class are only to be accessed and
  * modified from the tethering master state machine thread. Any other
@@ -119,33 +122,31 @@ public class UpstreamNetworkMonitor {
         mCM = cm;
     }
 
-    public void start(NetworkRequest defaultNetworkRequest) {
+    public void startTrackDefaultNetwork(NetworkRequest defaultNetworkRequest) {
+        // This is not really a "request", just a way of tracking the system default network.
+        // It's guaranteed not to actually bring up any networks because it's the same request
+        // as the ConnectivityService default request, and thus shares fate with it. We can't
+        // use registerDefaultNetworkCallback because it will not track the system default
+        // network if there is a VPN that applies to our UID.
+        if (mDefaultNetworkCallback == null) {
+            final NetworkRequest trackDefaultRequest = new NetworkRequest(defaultNetworkRequest);
+            mDefaultNetworkCallback = new UpstreamNetworkCallback(CALLBACK_DEFAULT_INTERNET);
+            cm().requestNetwork(trackDefaultRequest, mDefaultNetworkCallback, mHandler);
+        }
+    }
+
+    public void startObserveAllNetworks() {
         stop();
 
         final NetworkRequest listenAllRequest = new NetworkRequest.Builder()
                 .clearCapabilities().build();
         mListenAllCallback = new UpstreamNetworkCallback(CALLBACK_LISTEN_ALL);
         cm().registerNetworkCallback(listenAllRequest, mListenAllCallback, mHandler);
-
-        if (defaultNetworkRequest != null) {
-            // This is not really a "request", just a way of tracking the system default network.
-            // It's guaranteed not to actually bring up any networks because it's the same request
-            // as the ConnectivityService default request, and thus shares fate with it. We can't
-            // use registerDefaultNetworkCallback because it will not track the system default
-            // network if there is a VPN that applies to our UID.
-            final NetworkRequest trackDefaultRequest = new NetworkRequest(defaultNetworkRequest);
-            mDefaultNetworkCallback = new UpstreamNetworkCallback(CALLBACK_DEFAULT_INTERNET);
-            cm().requestNetwork(trackDefaultRequest, mDefaultNetworkCallback, mHandler);
-        }
     }
 
     public void stop() {
         releaseMobileNetworkRequest();
 
-        releaseCallback(mDefaultNetworkCallback);
-        mDefaultNetworkCallback = null;
-        mDefaultInternetNetwork = null;
-
         releaseCallback(mListenAllCallback);
         mListenAllCallback = null;
 
@@ -264,9 +265,7 @@ public class UpstreamNetworkMonitor {
         mNetworkMap.put(network, new NetworkState(null, null, null, network, null, null));
     }
 
-    private void handleNetCap(int callbackType, Network network, NetworkCapabilities newNc) {
-        if (callbackType == CALLBACK_DEFAULT_INTERNET) mDefaultInternetNetwork = network;
-
+    private void handleNetCap(Network network, NetworkCapabilities newNc) {
         final NetworkState prev = mNetworkMap.get(network);
         if (prev == null || newNc.equals(prev.networkCapabilities)) {
             // Ignore notifications about networks for which we have not yet
@@ -315,31 +314,25 @@ public class UpstreamNetworkMonitor {
         notifyTarget(EVENT_ON_LINKPROPERTIES, network);
     }
 
-    private void handleSuspended(int callbackType, Network network) {
-        if (callbackType != CALLBACK_LISTEN_ALL) return;
+    private void handleSuspended(Network network) {
         if (!network.equals(mTetheringUpstreamNetwork)) return;
         mLog.log("SUSPENDED current upstream: " + network);
     }
 
-    private void handleResumed(int callbackType, Network network) {
-        if (callbackType != CALLBACK_LISTEN_ALL) return;
+    private void handleResumed(Network network) {
         if (!network.equals(mTetheringUpstreamNetwork)) return;
         mLog.log("RESUMED current upstream: " + network);
     }
 
-    private void handleLost(int callbackType, Network network) {
-        if (network.equals(mDefaultInternetNetwork)) {
-            mDefaultInternetNetwork = null;
-            // There are few TODOs within ConnectivityService's rematching code
-            // pertaining to spurious onLost() notifications.
-            //
-            // TODO: simplify this, probably if favor of code that:
-            //     - selects a new upstream if mTetheringUpstreamNetwork has
-            //       been lost (by any callback)
-            //     - deletes the entry from the map only when the LISTEN_ALL
-            //       callback gets  notified.
-            if (callbackType == CALLBACK_DEFAULT_INTERNET) return;
-        }
+    private void handleLost(Network network) {
+        // There are few TODOs within ConnectivityService's rematching code
+        // pertaining to spurious onLost() notifications.
+        //
+        // TODO: simplify this, probably if favor of code that:
+        //     - selects a new upstream if mTetheringUpstreamNetwork has
+        //       been lost (by any callback)
+        //     - deletes the entry from the map only when the LISTEN_ALL
+        //       callback gets notified.
 
         if (!mNetworkMap.containsKey(network)) {
             // Ignore loss of networks about which we had not previously
@@ -393,11 +386,17 @@ public class UpstreamNetworkMonitor {
 
         @Override
         public void onCapabilitiesChanged(Network network, NetworkCapabilities newNc) {
-            handleNetCap(mCallbackType, network, newNc);
+            if (mCallbackType == CALLBACK_DEFAULT_INTERNET) {
+                mDefaultInternetNetwork = network;
+                return;
+            }
+            handleNetCap(network, newNc);
         }
 
         @Override
         public void onLinkPropertiesChanged(Network network, LinkProperties newLp) {
+            if (mCallbackType == CALLBACK_DEFAULT_INTERNET) return;
+
             handleLinkProp(network, newLp);
             // Any non-LISTEN_ALL callback will necessarily concern a network that will
             // also match the LISTEN_ALL callback by construction of the LISTEN_ALL callback.
@@ -409,17 +408,25 @@ public class UpstreamNetworkMonitor {
 
         @Override
         public void onNetworkSuspended(Network network) {
-            handleSuspended(mCallbackType, network);
+            if (mCallbackType == CALLBACK_LISTEN_ALL) {
+                handleSuspended(network);
+            }
         }
 
         @Override
         public void onNetworkResumed(Network network) {
-            handleResumed(mCallbackType, network);
+            if (mCallbackType == CALLBACK_LISTEN_ALL) {
+                handleResumed(network);
+            }
         }
 
         @Override
         public void onLost(Network network) {
-            handleLost(mCallbackType, network);
+            if (mCallbackType == CALLBACK_DEFAULT_INTERNET) {
+                mDefaultInternetNetwork = null;
+                return;
+            }
+            handleLost(network);
             // Any non-LISTEN_ALL callback will necessarily concern a network that will
             // also match the LISTEN_ALL callback by construction of the LISTEN_ALL callback.
             // So it's not useful to do this work for non-LISTEN_ALL callbacks.
diff --git a/tests/net/java/com/android/server/connectivity/TetheringTest.java b/tests/net/java/com/android/server/connectivity/TetheringTest.java
index 80818120fa3c..bca9be772704 100644
--- a/tests/net/java/com/android/server/connectivity/TetheringTest.java
+++ b/tests/net/java/com/android/server/connectivity/TetheringTest.java
@@ -71,7 +71,6 @@ import android.net.MacAddress;
 import android.net.Network;
 import android.net.NetworkCapabilities;
 import android.net.NetworkInfo;
-import android.net.NetworkRequest;
 import android.net.NetworkState;
 import android.net.NetworkUtils;
 import android.net.RouteInfo;
@@ -130,10 +129,6 @@ public class TetheringTest {
     private static final String TEST_USB_IFNAME = "test_rndis0";
     private static final String TEST_WLAN_IFNAME = "test_wlan0";
 
-    // Actual contents of the request don't matter for this test. The lack of
-    // any specific TRANSPORT_* is sufficient to identify this request.
-    private static final NetworkRequest mDefaultRequest = new NetworkRequest.Builder().build();
-
     @Mock private ApplicationInfo mApplicationInfo;
     @Mock private Context mContext;
     @Mock private INetworkManagementService mNMService;
@@ -257,11 +252,6 @@ public class TetheringTest {
             isTetheringSupportedCalls++;
             return true;
         }
-
-        @Override
-        public NetworkRequest getDefaultNetworkRequest() {
-            return mDefaultRequest;
-        }
     }
 
     private static NetworkState buildMobileUpstreamState(boolean withIPv4, boolean withIPv6,
@@ -496,7 +486,7 @@ public class TetheringTest {
                 TEST_WLAN_IFNAME, WifiManager.IFACE_IP_MODE_LOCAL_ONLY);
         verifyNoMoreInteractions(mWifiManager);
         verifyTetheringBroadcast(TEST_WLAN_IFNAME, EXTRA_ACTIVE_LOCAL_ONLY);
-        verify(mUpstreamNetworkMonitor, times(1)).start(any(NetworkRequest.class));
+        verify(mUpstreamNetworkMonitor, times(1)).startObserveAllNetworks();
         // TODO: Figure out why this isn't exactly once, for sendTetherStateChangedBroadcast().
         assertTrue(1 <= mTetheringDependencies.isTetheringSupportedCalls);
 
@@ -730,7 +720,7 @@ public class TetheringTest {
                 TEST_WLAN_IFNAME, WifiManager.IFACE_IP_MODE_TETHERED);
         verifyNoMoreInteractions(mWifiManager);
         verifyTetheringBroadcast(TEST_WLAN_IFNAME, EXTRA_ACTIVE_TETHER);
-        verify(mUpstreamNetworkMonitor, times(1)).start(any(NetworkRequest.class));
+        verify(mUpstreamNetworkMonitor, times(1)).startObserveAllNetworks();
         // In tethering mode, in the default configuration, an explicit request
         // for a mobile network is also made.
         verify(mUpstreamNetworkMonitor, times(1)).registerMobileNetworkRequest();
diff --git a/tests/net/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitorTest.java b/tests/net/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitorTest.java
index a22cbd4c95d8..0afd607d1457 100644
--- a/tests/net/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitorTest.java
+++ b/tests/net/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitorTest.java
@@ -24,6 +24,7 @@ import static android.net.NetworkCapabilities.NET_CAPABILITY_DUN;
 import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET;
 import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR;
 import static android.net.NetworkCapabilities.TRANSPORT_WIFI;
+
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -53,7 +54,6 @@ import android.net.NetworkCapabilities;
 import android.net.NetworkRequest;
 import android.net.NetworkState;
 import android.net.util.SharedLog;
-
 import android.support.test.filters.SmallTest;
 import android.support.test.runner.AndroidJUnit4;
 
@@ -65,7 +65,6 @@ import org.junit.Before;
 import org.junit.runner.RunWith;
 import org.junit.Test;
 import org.mockito.Mock;
-import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 
 import java.util.ArrayList;
@@ -126,7 +125,7 @@ public class UpstreamNetworkMonitorTest {
     }
 
     @Test
-    public void testDoesNothingBeforeStarted() {
+    public void testDoesNothingBeforeTrackDefaultAndStarted() throws Exception {
         assertTrue(mCM.hasNoCallbacks());
         assertFalse(mUNM.mobileNetworkRequested());
 
@@ -138,37 +137,40 @@ public class UpstreamNetworkMonitorTest {
 
     @Test
     public void testDefaultNetworkIsTracked() throws Exception {
-        assertEquals(0, mCM.trackingDefault.size());
+        assertTrue(mCM.hasNoCallbacks());
+        mUNM.startTrackDefaultNetwork(mDefaultRequest);
 
-        mUNM.start(mDefaultRequest);
+        mUNM.startObserveAllNetworks();
         assertEquals(1, mCM.trackingDefault.size());
 
         mUNM.stop();
-        assertTrue(mCM.hasNoCallbacks());
+        assertTrue(mCM.onlyHasDefaultCallbacks());
     }
 
     @Test
     public void testListensForAllNetworks() throws Exception {
         assertTrue(mCM.listening.isEmpty());
 
-        mUNM.start(mDefaultRequest);
+        mUNM.startTrackDefaultNetwork(mDefaultRequest);
+        mUNM.startObserveAllNetworks();
         assertFalse(mCM.listening.isEmpty());
         assertTrue(mCM.isListeningForAll());
 
         mUNM.stop();
-        assertTrue(mCM.hasNoCallbacks());
+        assertTrue(mCM.onlyHasDefaultCallbacks());
     }
 
     @Test
     public void testCallbacksRegistered() {
-        mUNM.start(mDefaultRequest);
-        verify(mCM, times(1)).registerNetworkCallback(
-                any(NetworkRequest.class), any(NetworkCallback.class), any(Handler.class));
+        mUNM.startTrackDefaultNetwork(mDefaultRequest);
         verify(mCM, times(1)).requestNetwork(
                 eq(mDefaultRequest), any(NetworkCallback.class), any(Handler.class));
+        mUNM.startObserveAllNetworks();
+        verify(mCM, times(1)).registerNetworkCallback(
+                any(NetworkRequest.class), any(NetworkCallback.class), any(Handler.class));
 
         mUNM.stop();
-        verify(mCM, times(2)).unregisterNetworkCallback(any(NetworkCallback.class));
+        verify(mCM, times(1)).unregisterNetworkCallback(any(NetworkCallback.class));
     }
 
     @Test
@@ -176,7 +178,7 @@ public class UpstreamNetworkMonitorTest {
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());
 
-        mUNM.start(mDefaultRequest);
+        mUNM.startObserveAllNetworks();
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());
 
@@ -199,11 +201,9 @@ public class UpstreamNetworkMonitorTest {
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());
 
-        mUNM.start(mDefaultRequest);
+        mUNM.startObserveAllNetworks();
         verify(mCM, times(1)).registerNetworkCallback(
                 any(NetworkRequest.class), any(NetworkCallback.class), any(Handler.class));
-        verify(mCM, times(1)).requestNetwork(
-                eq(mDefaultRequest), any(NetworkCallback.class), any(Handler.class));
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());
 
@@ -227,7 +227,7 @@ public class UpstreamNetworkMonitorTest {
         assertTrue(mCM.isDunRequested());
 
         mUNM.stop();
-        verify(mCM, times(3)).unregisterNetworkCallback(any(NetworkCallback.class));
+        verify(mCM, times(2)).unregisterNetworkCallback(any(NetworkCallback.class));
 
         verifyNoMoreInteractions(mCM);
     }
@@ -237,7 +237,7 @@ public class UpstreamNetworkMonitorTest {
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());
 
-        mUNM.start(mDefaultRequest);
+        mUNM.startObserveAllNetworks();
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());
 
@@ -257,7 +257,7 @@ public class UpstreamNetworkMonitorTest {
 
     @Test
     public void testUpdateMobileRequiresDun() throws Exception {
-        mUNM.start(mDefaultRequest);
+        mUNM.startObserveAllNetworks();
 
         // Test going from no-DUN to DUN correctly re-registers callbacks.
         mUNM.updateMobileRequiresDun(false);
@@ -285,7 +285,8 @@ public class UpstreamNetworkMonitorTest {
         final Collection<Integer> preferredTypes = new ArrayList<>();
         preferredTypes.add(TYPE_WIFI);
 
-        mUNM.start(mDefaultRequest);
+        mUNM.startTrackDefaultNetwork(mDefaultRequest);
+        mUNM.startObserveAllNetworks();
         // There are no networks, so there is nothing to select.
         assertSatisfiesLegacyType(TYPE_NONE, mUNM.selectPreferredUpstreamType(preferredTypes));
 
@@ -350,7 +351,8 @@ public class UpstreamNetworkMonitorTest {
 
     @Test
     public void testGetCurrentPreferredUpstream() throws Exception {
-        mUNM.start(mDefaultRequest);
+        mUNM.startTrackDefaultNetwork(mDefaultRequest);
+        mUNM.startObserveAllNetworks();
         mUNM.updateMobileRequiresDun(false);
 
         // [0] Mobile connects, DUN not required -> mobile selected.
@@ -389,7 +391,8 @@ public class UpstreamNetworkMonitorTest {
 
     @Test
     public void testLocalPrefixes() throws Exception {
-        mUNM.start(mDefaultRequest);
+        mUNM.startTrackDefaultNetwork(mDefaultRequest);
+        mUNM.startObserveAllNetworks();
 
         // [0] Test minimum set of local prefixes.
         Set<IpPrefix> local = mUNM.getLocalPrefixes();
@@ -521,11 +524,19 @@ public class UpstreamNetworkMonitorTest {
         }
 
         boolean hasNoCallbacks() {
-            return allCallbacks.isEmpty() &&
-                   trackingDefault.isEmpty() &&
-                   listening.isEmpty() &&
-                   requested.isEmpty() &&
-                   legacyTypeMap.isEmpty();
+            return allCallbacks.isEmpty()
+                    && trackingDefault.isEmpty()
+                    && listening.isEmpty()
+                    && requested.isEmpty()
+                    && legacyTypeMap.isEmpty();
+        }
+
+        boolean onlyHasDefaultCallbacks() {
+            return (allCallbacks.size() == 1)
+                    && (trackingDefault.size() == 1)
+                    && listening.isEmpty()
+                    && requested.isEmpty()
+                    && legacyTypeMap.isEmpty();
         }
 
         boolean isListeningForAll() {