Fix race when starting NetworkMonitor

NetworkMonitor obtained LinkProperties and NetworkCapabilities via
synchronous calls to ConnectivityManager after receiving an asynchronous
notification, which is prone to races: the network could be gone before
the LinkProperties/NetworkCapabilities can be fetched.

Fix the race by passing LinkProperties/NetworkCapabilities directly to
NetworkMonitor in the asynchronous notifications.

Test: atest FrameworksNetTests NetworkStackTests
Test: booted, WiFi works
Bug: 129375892
Change-Id: I200ac7ca6ff79590b11c9be705f650c92fd3cb63

3 files changed, 132 insertions, 144 deletions

diff --git a/src/com/android/server/NetworkStackService.java b/src/com/android/server/NetworkStackService.java
index 19e9108..63f057c 100644
--- a/src/com/android/server/NetworkStackService.java
+++ b/src/com/android/server/NetworkStackService.java
@@ -35,7 +35,9 @@ import android.net.INetd;
 import android.net.INetworkMonitor;
 import android.net.INetworkMonitorCallbacks;
 import android.net.INetworkStackConnector;
+import android.net.LinkProperties;
 import android.net.Network;
+import android.net.NetworkCapabilities;
 import android.net.PrivateDnsConfigParcel;
 import android.net.dhcp.DhcpServer;
 import android.net.dhcp.DhcpServingParams;
@@ -307,9 +309,9 @@ public class NetworkStackService extends Service {
         }
 
         @Override
-        public void notifyNetworkConnected() {
+        public void notifyNetworkConnected(LinkProperties lp, NetworkCapabilities nc) {
             checkNetworkStackCallingPermission();
-            mNm.notifyNetworkConnected();
+            mNm.notifyNetworkConnected(lp, nc);
         }
 
         @Override
@@ -319,15 +321,15 @@ public class NetworkStackService extends Service {
         }
 
         @Override
-        public void notifyLinkPropertiesChanged() {
+        public void notifyLinkPropertiesChanged(LinkProperties lp) {
             checkNetworkStackCallingPermission();
-            mNm.notifyLinkPropertiesChanged();
+            mNm.notifyLinkPropertiesChanged(lp);
         }
 
         @Override
-        public void notifyNetworkCapabilitiesChanged() {
+        public void notifyNetworkCapabilitiesChanged(NetworkCapabilities nc) {
             checkNetworkStackCallingPermission();
-            mNm.notifyNetworkCapabilitiesChanged();
+            mNm.notifyNetworkCapabilitiesChanged(nc);
         }
     }
 }
diff --git a/src/com/android/server/connectivity/NetworkMonitor.java b/src/com/android/server/connectivity/NetworkMonitor.java
index f3476ed..c000fc6 100644
--- a/src/com/android/server/connectivity/NetworkMonitor.java
+++ b/src/com/android/server/connectivity/NetworkMonitor.java
@@ -78,6 +78,7 @@ import android.telephony.SignalStrength;
 import android.telephony.TelephonyManager;
 import android.text.TextUtils;
 import android.util.Log;
+import android.util.Pair;
 
 import com.android.internal.annotations.VisibleForTesting;
 import com.android.internal.util.RingBufferIndices;
@@ -221,19 +222,31 @@ public class NetworkMonitor extends StateMachine {
      * Message to self indicating captive portal detection is completed.
      * obj = CaptivePortalProbeResult for detection result;
      */
-    public static final int CMD_PROBE_COMPLETE = 16;
+    private static final int CMD_PROBE_COMPLETE = 16;
 
     /**
      * ConnectivityService notifies NetworkMonitor of DNS query responses event.
      * arg1 = returncode in OnDnsEvent which indicates the response code for the DNS query.
      */
-    public static final int EVENT_DNS_NOTIFICATION = 17;
+    private static final int EVENT_DNS_NOTIFICATION = 17;
 
     /**
      * ConnectivityService notifies NetworkMonitor that the user accepts partial connectivity and
      * NetworkMonitor should ignore the https probe.
      */
-    public static final int EVENT_ACCEPT_PARTIAL_CONNECTIVITY = 18;
+    private static final int EVENT_ACCEPT_PARTIAL_CONNECTIVITY = 18;
+
+    /**
+     * ConnectivityService notifies NetworkMonitor of changed LinkProperties.
+     * obj = new LinkProperties.
+     */
+    private static final int EVENT_LINK_PROPERTIES_CHANGED = 19;
+
+    /**
+     * ConnectivityService notifies NetworkMonitor of changed NetworkCapabilities.
+     * obj = new NetworkCapabilities.
+     */
+    private static final int EVENT_NETWORK_CAPABILITIES_CHANGED = 20;
 
     // Start mReevaluateDelayMs at this value and double.
     private static final int INITIAL_REEVALUATE_DELAY_MS = 1000;
@@ -379,10 +392,8 @@ public class NetworkMonitor extends StateMachine {
         mDataStallValidDnsTimeThreshold = getDataStallValidDnsTimeThreshold();
         mDataStallEvaluationType = getDataStallEvalutionType();
 
-        // mLinkProperties and mNetworkCapbilities must never be null or we will NPE.
-        // Provide empty objects in case we are started and the network disconnects before
-        // we can ever fetch them.
-        // TODO: Delete ASAP.
+        // Provide empty LinkProperties and NetworkCapabilities to make sure they are never null,
+        // even before notifyNetworkConnected.
         mLinkProperties = new LinkProperties();
         mNetworkCapabilities = new NetworkCapabilities(null);
     }
@@ -434,8 +445,16 @@ public class NetworkMonitor extends StateMachine {
     /**
      * Send a notification to NetworkMonitor indicating that the network is now connected.
      */
-    public void notifyNetworkConnected() {
-        sendMessage(CMD_NETWORK_CONNECTED);
+    public void notifyNetworkConnected(LinkProperties lp, NetworkCapabilities nc) {
+        sendMessage(CMD_NETWORK_CONNECTED, new Pair<>(
+                new LinkProperties(lp), new NetworkCapabilities(nc)));
+    }
+
+    private void updateConnectedNetworkAttributes(Message connectedMsg) {
+        final Pair<LinkProperties, NetworkCapabilities> attrs =
+                (Pair<LinkProperties, NetworkCapabilities>) connectedMsg.obj;
+        mLinkProperties = attrs.first;
+        mNetworkCapabilities = attrs.second;
     }
 
     /**
@@ -448,37 +467,15 @@ public class NetworkMonitor extends StateMachine {
     /**
      * Send a notification to NetworkMonitor indicating that link properties have changed.
      */
-    public void notifyLinkPropertiesChanged() {
-        getHandler().post(() -> {
-            updateLinkProperties();
-        });
-    }
-
-    private void updateLinkProperties() {
-        final LinkProperties lp = mCm.getLinkProperties(mNetwork);
-        // If null, we should soon get a message that the network was disconnected, and will stop.
-        if (lp != null) {
-            // TODO: send LinkProperties parceled in notifyLinkPropertiesChanged() and start().
-            mLinkProperties = lp;
-        }
+    public void notifyLinkPropertiesChanged(final LinkProperties lp) {
+        sendMessage(EVENT_LINK_PROPERTIES_CHANGED, new LinkProperties(lp));
     }
 
     /**
      * Send a notification to NetworkMonitor indicating that network capabilities have changed.
      */
-    public void notifyNetworkCapabilitiesChanged() {
-        getHandler().post(() -> {
-            updateNetworkCapabilities();
-        });
-    }
-
-    private void updateNetworkCapabilities() {
-        final NetworkCapabilities nc = mCm.getNetworkCapabilities(mNetwork);
-        // If null, we should soon get a message that the network was disconnected, and will stop.
-        if (nc != null) {
-            // TODO: send NetworkCapabilities parceled in notifyNetworkCapsChanged() and start().
-            mNetworkCapabilities = nc;
-        }
+    public void notifyNetworkCapabilitiesChanged(final NetworkCapabilities nc) {
+        sendMessage(EVENT_NETWORK_CAPABILITIES_CHANGED, new NetworkCapabilities(nc));
     }
 
     /**
@@ -547,16 +544,10 @@ public class NetworkMonitor extends StateMachine {
     // does not entail any real state (hence no enter() or exit() routines).
     private class DefaultState extends State {
         @Override
-        public void enter() {
-            // TODO: have those passed parceled in start() and remove this
-            updateLinkProperties();
-            updateNetworkCapabilities();
-        }
-
-        @Override
         public boolean processMessage(Message message) {
             switch (message.what) {
                 case CMD_NETWORK_CONNECTED:
+                    updateConnectedNetworkAttributes(message);
                     logNetworkEvent(NetworkEvent.NETWORK_CONNECTED);
                     transitionTo(mEvaluatingState);
                     return HANDLED;
@@ -660,6 +651,12 @@ public class NetworkMonitor extends StateMachine {
                 case EVENT_ACCEPT_PARTIAL_CONNECTIVITY:
                     mAcceptPartialConnectivity = true;
                     break;
+                case EVENT_LINK_PROPERTIES_CHANGED:
+                    mLinkProperties = (LinkProperties) message.obj;
+                    break;
+                case EVENT_NETWORK_CAPABILITIES_CHANGED:
+                    mNetworkCapabilities = (NetworkCapabilities) message.obj;
+                    break;
                 default:
                     break;
             }
@@ -684,6 +681,7 @@ public class NetworkMonitor extends StateMachine {
         public boolean processMessage(Message message) {
             switch (message.what) {
                 case CMD_NETWORK_CONNECTED:
+                    updateConnectedNetworkAttributes(message);
                     transitionTo(mValidatedState);
                     break;
                 case CMD_EVALUATE_PRIVATE_DNS:
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 {