Merge "Fix race when starting NetworkMonitor"

1 files changed, 84 insertions, 96 deletions

diff --git a/tests/src/com/android/server/connectivity/NetworkMonitorTest.java b/tests/src/com/android/server/connectivity/NetworkMonitorTest.java
index d732c4e..6665aae 100644
--- a/tests/src/com/android/server/connectivity/NetworkMonitorTest.java
+++ b/tests/src/com/android/server/connectivity/NetworkMonitorTest.java
@@ -26,6 +26,8 @@ import static android.provider.Settings.Global.DATA_STALL_EVALUATION_TYPE_DNS;
 import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertFalse;
 
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.anyString;
@@ -60,6 +62,7 @@ import android.net.wifi.WifiManager;
 import android.os.Bundle;
 import android.os.ConditionVariable;
 import android.os.Handler;
+import android.os.RemoteException;
 import android.os.SystemClock;
 import android.provider.Settings;
 import android.telephony.CellSignalStrength;
@@ -73,6 +76,7 @@ import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.mockito.Spy;
@@ -107,6 +111,7 @@ public class NetworkMonitorTest {
     private @Spy Network mNetwork = new Network(TEST_NETID);
     private @Mock DataStallStatsUtils mDataStallStatsUtils;
     private @Mock WifiInfo mWifiInfo;
+    private @Captor ArgumentCaptor<String> mNetworkTestedRedirectUrlCaptor;
 
     private static final int TEST_NETID = 4242;
 
@@ -122,7 +127,7 @@ public class NetworkMonitorTest {
 
     private static final int HANDLER_TIMEOUT_MS = 1000;
 
-    private static final LinkProperties TEST_LINKPROPERTIES = new LinkProperties();
+    private static final LinkProperties TEST_LINK_PROPERTIES = new LinkProperties();
 
     private static final NetworkCapabilities METERED_CAPABILITIES = new NetworkCapabilities()
             .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR)
@@ -182,10 +187,6 @@ public class NetworkMonitorTest {
                 InetAddresses.parseNumericAddress("192.168.0.0")
         }).when(mNetwork).getAllByName(any());
 
-        // Default values. Individual tests can override these.
-        when(mCm.getLinkProperties(any())).thenReturn(TEST_LINKPROPERTIES);
-        when(mCm.getNetworkCapabilities(any())).thenReturn(METERED_CAPABILITIES);
-
         setMinDataStallEvaluateInterval(500);
         setDataStallEvaluationType(DATA_STALL_EVALUATION_TYPE_DNS);
         setValidDataStallDnsTimeThreshold(500);
@@ -195,10 +196,9 @@ public class NetworkMonitorTest {
     private class WrappedNetworkMonitor extends NetworkMonitor {
         private long mProbeTime = 0;
 
-        WrappedNetworkMonitor(Context context, Network network, IpConnectivityLog logger,
-                Dependencies deps, DataStallStatsUtils statsUtils) {
-                super(context, mCallbacks, network, logger,
-                        new SharedLog("test_nm"), deps, statsUtils);
+        WrappedNetworkMonitor() {
+                super(mContext, mCallbacks, mNetwork, mLogger, mValidationLogger, mDependencies,
+                        mDataStallStatsUtils);
         }
 
         @Override
@@ -216,33 +216,30 @@ public class NetworkMonitorTest {
         }
     }
 
-    private WrappedNetworkMonitor makeMeteredWrappedNetworkMonitor() {
-        final WrappedNetworkMonitor nm = new WrappedNetworkMonitor(
-                mContext, mNetwork, mLogger, mDependencies, mDataStallStatsUtils);
-        when(mCm.getNetworkCapabilities(any())).thenReturn(METERED_CAPABILITIES);
+    private WrappedNetworkMonitor makeMonitor() {
+        final WrappedNetworkMonitor nm = new WrappedNetworkMonitor();
         nm.start();
         waitForIdle(nm.getHandler());
         return nm;
     }
 
-    private WrappedNetworkMonitor makeNotMeteredWrappedNetworkMonitor() {
-        final WrappedNetworkMonitor nm = new WrappedNetworkMonitor(
-                mContext, mNetwork, mLogger, mDependencies, mDataStallStatsUtils);
-        when(mCm.getNetworkCapabilities(any())).thenReturn(NOT_METERED_CAPABILITIES);
-        nm.start();
-        waitForIdle(nm.getHandler());
+    private WrappedNetworkMonitor makeMeteredNetworkMonitor() {
+        final WrappedNetworkMonitor nm = makeMonitor();
+        setNetworkCapabilities(nm, METERED_CAPABILITIES);
         return nm;
     }
 
-    private NetworkMonitor makeMonitor() {
-        final NetworkMonitor nm = new NetworkMonitor(
-                mContext, mCallbacks, mNetwork, mLogger, mValidationLogger,
-                mDependencies, mDataStallStatsUtils);
-        nm.start();
-        waitForIdle(nm.getHandler());
+    private WrappedNetworkMonitor makeNotMeteredNetworkMonitor() {
+        final WrappedNetworkMonitor nm = makeMonitor();
+        setNetworkCapabilities(nm, NOT_METERED_CAPABILITIES);
         return nm;
     }
 
+    private void setNetworkCapabilities(NetworkMonitor nm, NetworkCapabilities nc) {
+        nm.notifyNetworkCapabilitiesChanged(nc);
+        waitForIdle(nm.getHandler());
+    }
+
     private void waitForIdle(Handler handler) {
         final ConditionVariable cv = new ConditionVariable(false);
         handler.post(cv::open);
@@ -256,7 +253,7 @@ public class NetworkMonitorTest {
         setSslException(mHttpsConnection);
         setPortal302(mHttpConnection);
 
-        assertPortal(makeMonitor().isCaptivePortal());
+        runPortalNetworkTest();
     }
 
     @Test
@@ -264,17 +261,7 @@ public class NetworkMonitorTest {
         setStatus(mHttpsConnection, 204);
         setStatus(mHttpConnection, 500);
 
-        assertNotPortal(makeMonitor().isCaptivePortal());
-    }
-
-    @Test
-    public void testIsCaptivePortal_HttpsProbeFailedHttpSuccessNotUsed() throws IOException {
-        setSslException(mHttpsConnection);
-        // Even if HTTP returns a 204, do not use the result unless HTTPS succeeded
-        setStatus(mHttpConnection, 204);
-        setStatus(mFallbackConnection, 500);
-
-        assertFailed(makeMonitor().isCaptivePortal());
+        runNotPortalNetworkTest();
     }
 
     @Test
@@ -283,17 +270,17 @@ public class NetworkMonitorTest {
         setStatus(mHttpConnection, 500);
         setPortal302(mFallbackConnection);
 
-        assertPortal(makeMonitor().isCaptivePortal());
+        runPortalNetworkTest();
     }
 
     @Test
     public void testIsCaptivePortal_FallbackProbeIsNotPortal() throws IOException {
         setSslException(mHttpsConnection);
         setStatus(mHttpConnection, 500);
-        setStatus(mFallbackConnection, 204);
+        setStatus(mFallbackConnection, 500);
 
         // Fallback probe did not see portal, HTTPS failed -> inconclusive
-        assertFailed(makeMonitor().isCaptivePortal());
+        runFailedNetworkTest();
     }
 
     @Test
@@ -310,15 +297,15 @@ public class NetworkMonitorTest {
         // TEST_OTHER_FALLBACK_URL is third
         when(mRandom.nextInt()).thenReturn(2);
 
-        final NetworkMonitor monitor = makeMonitor();
-
         // First check always uses the first fallback URL: inconclusive
-        assertFailed(monitor.isCaptivePortal());
+        final NetworkMonitor monitor = runNetworkTest(NETWORK_TEST_RESULT_INVALID);
+        assertNull(mNetworkTestedRedirectUrlCaptor.getValue());
         verify(mFallbackConnection, times(1)).getResponseCode();
         verify(mOtherFallbackConnection, never()).getResponseCode();
 
         // Second check uses the URL chosen by Random
-        assertPortal(monitor.isCaptivePortal());
+        final CaptivePortalProbeResult result = monitor.isCaptivePortal();
+        assertTrue(result.isPortal());
         verify(mOtherFallbackConnection, times(1)).getResponseCode();
     }
 
@@ -328,7 +315,7 @@ public class NetworkMonitorTest {
         setStatus(mHttpConnection, 500);
         setStatus(mFallbackConnection, 404);
 
-        assertFailed(makeMonitor().isCaptivePortal());
+        runFailedNetworkTest();
         verify(mFallbackConnection, times(1)).getResponseCode();
         verify(mOtherFallbackConnection, never()).getResponseCode();
     }
@@ -342,7 +329,7 @@ public class NetworkMonitorTest {
         setStatus(mHttpConnection, 500);
         setPortal302(mOtherFallbackConnection);
 
-        assertPortal(makeMonitor().isCaptivePortal());
+        runPortalNetworkTest();
         verify(mOtherFallbackConnection, times(1)).getResponseCode();
         verify(mFallbackConnection, never()).getResponseCode();
     }
@@ -360,12 +347,12 @@ public class NetworkMonitorTest {
     }
 
     @Test
-    public void testIsCaptivePortal_FallbackSpecIsNotPortal() throws IOException {
+    public void testIsCaptivePortal_FallbackSpecIsPartial() throws IOException {
         setupFallbackSpec();
         set302(mOtherFallbackConnection, "https://www.google.com/test?q=3");
 
-        // HTTPS failed, fallback spec did not see a portal -> inconclusive
-        assertFailed(makeMonitor().isCaptivePortal());
+        // HTTPS failed, fallback spec went through -> partial connectivity
+        runPartialConnectivityNetworkTest();
         verify(mOtherFallbackConnection, times(1)).getResponseCode();
         verify(mFallbackConnection, never()).getResponseCode();
     }
@@ -375,7 +362,7 @@ public class NetworkMonitorTest {
         setupFallbackSpec();
         set302(mOtherFallbackConnection, "http://login.portal.example.com");
 
-        assertPortal(makeMonitor().isCaptivePortal());
+        runPortalNetworkTest();
     }
 
     @Test
@@ -384,20 +371,20 @@ public class NetworkMonitorTest {
         setSslException(mHttpsConnection);
         setPortal302(mHttpConnection);
 
-        assertNotPortal(makeMonitor().isCaptivePortal());
+        runNotPortalNetworkTest();
     }
 
     @Test
     public void testIsDataStall_EvaluationDisabled() {
         setDataStallEvaluationType(0);
-        WrappedNetworkMonitor wrappedMonitor = makeMeteredWrappedNetworkMonitor();
+        WrappedNetworkMonitor wrappedMonitor = makeMeteredNetworkMonitor();
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 100);
         assertFalse(wrappedMonitor.isDataStall());
     }
 
     @Test
     public void testIsDataStall_EvaluationDnsOnNotMeteredNetwork() {
-        WrappedNetworkMonitor wrappedMonitor = makeNotMeteredWrappedNetworkMonitor();
+        WrappedNetworkMonitor wrappedMonitor = makeNotMeteredNetworkMonitor();
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 100);
         makeDnsTimeoutEvent(wrappedMonitor, DEFAULT_DNS_TIMEOUT_THRESHOLD);
         assertTrue(wrappedMonitor.isDataStall());
@@ -405,7 +392,7 @@ public class NetworkMonitorTest {
 
     @Test
     public void testIsDataStall_EvaluationDnsOnMeteredNetwork() {
-        WrappedNetworkMonitor wrappedMonitor = makeMeteredWrappedNetworkMonitor();
+        WrappedNetworkMonitor wrappedMonitor = makeMeteredNetworkMonitor();
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 100);
         assertFalse(wrappedMonitor.isDataStall());
 
@@ -416,7 +403,7 @@ public class NetworkMonitorTest {
 
     @Test
     public void testIsDataStall_EvaluationDnsWithDnsTimeoutCount() {
-        WrappedNetworkMonitor wrappedMonitor = makeMeteredWrappedNetworkMonitor();
+        WrappedNetworkMonitor wrappedMonitor = makeMeteredNetworkMonitor();
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 1000);
         makeDnsTimeoutEvent(wrappedMonitor, 3);
         assertFalse(wrappedMonitor.isDataStall());
@@ -430,7 +417,7 @@ public class NetworkMonitorTest {
 
         // Set the value to larger than the default dns log size.
         setConsecutiveDnsTimeoutThreshold(51);
-        wrappedMonitor = makeMeteredWrappedNetworkMonitor();
+        wrappedMonitor = makeMeteredNetworkMonitor();
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 1000);
         makeDnsTimeoutEvent(wrappedMonitor, 50);
         assertFalse(wrappedMonitor.isDataStall());
@@ -442,7 +429,7 @@ public class NetworkMonitorTest {
     @Test
     public void testIsDataStall_EvaluationDnsWithDnsTimeThreshold() {
         // Test dns events happened in valid dns time threshold.
-        WrappedNetworkMonitor wrappedMonitor = makeMeteredWrappedNetworkMonitor();
+        WrappedNetworkMonitor wrappedMonitor = makeMeteredNetworkMonitor();
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 100);
         makeDnsTimeoutEvent(wrappedMonitor, DEFAULT_DNS_TIMEOUT_THRESHOLD);
         assertFalse(wrappedMonitor.isDataStall());
@@ -451,7 +438,7 @@ public class NetworkMonitorTest {
 
         // Test dns events happened before valid dns time threshold.
         setValidDataStallDnsTimeThreshold(0);
-        wrappedMonitor = makeMeteredWrappedNetworkMonitor();
+        wrappedMonitor = makeMeteredNetworkMonitor();
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 100);
         makeDnsTimeoutEvent(wrappedMonitor, DEFAULT_DNS_TIMEOUT_THRESHOLD);
         assertFalse(wrappedMonitor.isDataStall());
@@ -464,24 +451,13 @@ public class NetworkMonitorTest {
         setSslException(mHttpsConnection);
         setStatus(mHttpConnection, 500);
         setStatus(mFallbackConnection, 404);
-        when(mCm.getNetworkCapabilities(any())).thenReturn(METERED_CAPABILITIES);
 
-        final NetworkMonitor nm = makeMonitor();
-        nm.notifyNetworkConnected();
-
-        verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(1))
-                .notifyNetworkTested(NETWORK_TEST_RESULT_INVALID, null);
+        runFailedNetworkTest();
     }
 
     @Test
     public void testNoInternetCapabilityValidated() throws Exception {
-        when(mCm.getNetworkCapabilities(any())).thenReturn(NO_INTERNET_CAPABILITIES);
-
-        final NetworkMonitor nm = makeMonitor();
-        nm.notifyNetworkConnected();
-
-        verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(1))
-                .notifyNetworkTested(NETWORK_TEST_RESULT_VALID, null);
+        runNetworkTest(NO_INTERNET_CAPABILITIES, NETWORK_TEST_RESULT_VALID);
         verify(mNetwork, never()).openConnection(any());
     }
 
@@ -491,7 +467,7 @@ public class NetworkMonitorTest {
         setPortal302(mHttpConnection);
 
         final NetworkMonitor nm = makeMonitor();
-        nm.notifyNetworkConnected();
+        nm.notifyNetworkConnected(TEST_LINK_PROPERTIES, METERED_CAPABILITIES);
 
         verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(1))
                 .showProvisioningNotification(any(), any());
@@ -522,7 +498,7 @@ public class NetworkMonitorTest {
 
     @Test
     public void testDataStall_StallSuspectedAndSendMetrics() throws IOException {
-        WrappedNetworkMonitor wrappedMonitor = makeNotMeteredWrappedNetworkMonitor();
+        WrappedNetworkMonitor wrappedMonitor = makeNotMeteredNetworkMonitor();
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 1000);
         makeDnsTimeoutEvent(wrappedMonitor, 5);
         assertTrue(wrappedMonitor.isDataStall());
@@ -531,7 +507,7 @@ public class NetworkMonitorTest {
 
     @Test
     public void testDataStall_NoStallSuspectedAndSendMetrics() throws IOException {
-        WrappedNetworkMonitor wrappedMonitor = makeNotMeteredWrappedNetworkMonitor();
+        WrappedNetworkMonitor wrappedMonitor = makeNotMeteredNetworkMonitor();
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 1000);
         makeDnsTimeoutEvent(wrappedMonitor, 3);
         assertFalse(wrappedMonitor.isDataStall());
@@ -540,7 +516,7 @@ public class NetworkMonitorTest {
 
     @Test
     public void testCollectDataStallMetrics() {
-        WrappedNetworkMonitor wrappedMonitor = makeNotMeteredWrappedNetworkMonitor();
+        WrappedNetworkMonitor wrappedMonitor = makeNotMeteredNetworkMonitor();
 
         when(mTelephony.getDataNetworkType()).thenReturn(TelephonyManager.NETWORK_TYPE_LTE);
         when(mTelephony.getNetworkOperator()).thenReturn(TEST_MCCMNC);
@@ -578,14 +554,11 @@ public class NetworkMonitorTest {
         setSslException(mHttpsConnection);
         setStatus(mHttpConnection, 204);
 
-        final NetworkMonitor nm = makeMonitor();
-        nm.notifyNetworkConnected();
-        verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(1))
-                .notifyNetworkTested(NETWORK_TEST_RESULT_PARTIAL_CONNECTIVITY, null);
+        final NetworkMonitor nm = runNetworkTest(NETWORK_TEST_RESULT_PARTIAL_CONNECTIVITY);
 
         nm.setAcceptPartialConnectivity();
         verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(1))
-                .notifyNetworkTested(NETWORK_TEST_RESULT_VALID, null);
+                .notifyNetworkTested(eq(NETWORK_TEST_RESULT_VALID), any());
     }
 
     @Test
@@ -593,12 +566,12 @@ public class NetworkMonitorTest {
         setStatus(mHttpsConnection, 500);
         setStatus(mHttpConnection, 204);
         setStatus(mFallbackConnection, 500);
-        assertPartialConnectivity(makeMonitor().isCaptivePortal());
+        runPartialConnectivityNetworkTest();
 
         setStatus(mHttpsConnection, 500);
         setStatus(mHttpConnection, 500);
         setStatus(mFallbackConnection, 204);
-        assertPartialConnectivity(makeMonitor().isCaptivePortal());
+        runPartialConnectivityNetworkTest();
     }
 
     private void makeDnsTimeoutEvent(WrappedNetworkMonitor wrappedMonitor, int count) {
@@ -660,26 +633,41 @@ public class NetworkMonitorTest {
                 eq(Settings.Global.CAPTIVE_PORTAL_MODE), anyInt())).thenReturn(mode);
     }
 
-    private void assertPortal(CaptivePortalProbeResult result) {
-        assertTrue(result.isPortal());
-        assertFalse(result.isFailed());
-        assertFalse(result.isSuccessful());
+    private void runPortalNetworkTest() {
+        runNetworkTest(NETWORK_TEST_RESULT_INVALID);
+        assertNotNull(mNetworkTestedRedirectUrlCaptor.getValue());
     }
 
-    private void assertNotPortal(CaptivePortalProbeResult result) {
-        assertFalse(result.isPortal());
-        assertFalse(result.isFailed());
-        assertTrue(result.isSuccessful());
+    private void runNotPortalNetworkTest() {
+        runNetworkTest(NETWORK_TEST_RESULT_VALID);
+        assertNull(mNetworkTestedRedirectUrlCaptor.getValue());
     }
 
-    private void assertFailed(CaptivePortalProbeResult result) {
-        assertFalse(result.isPortal());
-        assertTrue(result.isFailed());
-        assertFalse(result.isSuccessful());
+    private void runFailedNetworkTest() {
+        runNetworkTest(NETWORK_TEST_RESULT_INVALID);
+        assertNull(mNetworkTestedRedirectUrlCaptor.getValue());
     }
 
-    private void assertPartialConnectivity(CaptivePortalProbeResult result) {
-        assertTrue(result.isPartialConnectivity());
+    private void runPartialConnectivityNetworkTest() {
+        runNetworkTest(NETWORK_TEST_RESULT_PARTIAL_CONNECTIVITY);
+        assertNull(mNetworkTestedRedirectUrlCaptor.getValue());
+    }
+
+    private NetworkMonitor runNetworkTest(int testResult) {
+        return runNetworkTest(METERED_CAPABILITIES, testResult);
+    }
+
+    private NetworkMonitor runNetworkTest(NetworkCapabilities nc, int testResult) {
+        final NetworkMonitor monitor = makeMonitor();
+        monitor.notifyNetworkConnected(TEST_LINK_PROPERTIES, nc);
+        try {
+            verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(1))
+                    .notifyNetworkTested(eq(testResult), mNetworkTestedRedirectUrlCaptor.capture());
+        } catch (RemoteException e) {
+            fail("Unexpected exception: " + e);
+        }
+
+        return monitor;
     }
 
     private void setSslException(HttpURLConnection connection) throws IOException {