Rework unvalidated prompt into first eval timeout

This patch removes the 8-second unvalidated prompt timer
and replaces it with a 20-second timer that does the
same thing in addition to signalling the stack that the
concerned network has now undergone evaluation once.

Test: FrameworksNetTests
Change-Id: I806f2bdfbf35243fa7cdb1cd1f0a5d549cfcd9c8

service/src/com/android/server/ConnectivityService.java

@@ -353,6 +353,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
     // connect anyway?" dialog after the user selects a network that doesn't validate.
     private static final int PROMPT_UNVALIDATED_DELAY_MS = 8 * 1000;
 
+    // How long to wait before considering that a network is bad in the absence of any form
+    // of connectivity (valid, partial, captive portal). If none has been detected after this
+    // delay, the stack considers this network bad, which may affect how it's handled in ranking
+    // according to config_networkAvoidBadWifi.
+    private static final int INITIAL_EVALUATION_TIMEOUT_MS = 20 * 1000;
+
     // Default to 30s linger time-out, and 5s for nascent network. Modifiable only for testing.
     private static final String LINGER_DELAY_PROPERTY = "persist.netmon.linger";
     private static final int DEFAULT_LINGER_DELAY_MS = 30_000;
@@ -580,12 +586,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
      */
     private static final int EVENT_SET_ACCEPT_UNVALIDATED = 28;
 
-    /**
-     * used to ask the user to confirm a connection to an unvalidated network.
-     * obj  = network
-     */
-    private static final int EVENT_PROMPT_UNVALIDATED = 29;
-
     /**
      * used internally to (re)configure always-on networks.
      */
@@ -724,6 +724,14 @@ public class ConnectivityService extends IConnectivityManager.Stub
      */
     private static final int EVENT_INGRESS_RATE_LIMIT_CHANGED = 56;
 
+    /**
+     * The initial evaluation period is over for this network.
+     *
+     * If no form of connectivity has been found on this network (valid, partial, captive portal)
+     * then the stack will now consider it to have been determined bad.
+     */
+    private static final int EVENT_INITIAL_EVALUATION_TIMEOUT = 57;
+
     /**
      * Argument for {@link #EVENT_PROVISIONING_NOTIFICATION} to indicate that the notification
      * should be shown.
@@ -3791,7 +3799,17 @@ public class ConnectivityService extends IConnectivityManager.Stub
 
         private void handleNetworkTested(
                 @NonNull NetworkAgentInfo nai, int testResult, @NonNull String redirectUrl) {
-            final boolean valid = ((testResult & NETWORK_VALIDATION_RESULT_VALID) != 0);
+            final boolean valid = (testResult & NETWORK_VALIDATION_RESULT_VALID) != 0;
+            final boolean partial = (testResult & NETWORK_VALIDATION_RESULT_PARTIAL) != 0;
+            final boolean captive = !TextUtils.isEmpty(redirectUrl);
+
+            // If there is any kind of working networking, then the NAI has been evaluated
+            // once. {@see NetworkAgentInfo#setEvaluated}, which returns whether this is
+            // the first time this ever happened.
+            final boolean someConnectivity = (valid || partial || captive);
+            final boolean becameEvaluated = someConnectivity && nai.setEvaluated();
+            if (becameEvaluated) nai.updateScoreForNetworkAgentUpdate();
+
             if (!valid && shouldIgnoreValidationFailureAfterRoam(nai)) {
                 // Assume the validation failure is due to a temporary failure after roaming
                 // and ignore it. NetworkMonitor will continue to retry validation. If it
@@ -3834,8 +3852,13 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 }
             } else if (partialConnectivityChanged) {
                 updateCapabilitiesForNetwork(nai);
+            } else if (becameEvaluated) {
+                // If valid or partial connectivity changed, updateCapabilities* has
+                // done the rematch.
+                rematchAllNetworksAndRequests();
             }
             updateInetCondition(nai);
+
             // Let the NetworkAgent know the state of its network
             // TODO: Evaluate to update partial connectivity to status to NetworkAgent.
             nai.onValidationStatusChanged(
@@ -3843,13 +3866,13 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     redirectUrl);
 
             // If NetworkMonitor detects partial connectivity before
-            // EVENT_PROMPT_UNVALIDATED arrives, show the partial connectivity notification
+            // EVENT_INITIAL_EVALUATION_TIMEOUT arrives, show the partial connectivity notification
             // immediately. Re-notify partial connectivity silently if no internet
             // notification already there.
             if (!wasPartial && nai.partialConnectivity()) {
                 // Remove delayed message if there is a pending message.
-                mHandler.removeMessages(EVENT_PROMPT_UNVALIDATED, nai.network);
+                mHandler.removeMessages(EVENT_INITIAL_EVALUATION_TIMEOUT, nai.network);
-                handlePromptUnvalidated(nai.network);
+                handleInitialEvaluationTimeout(nai.network);
             }
 
             if (wasValidated && !nai.isValidated()) {
@@ -4949,16 +4972,11 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
     }
 
-    private void scheduleUnvalidatedPrompt(@NonNull final Network network) {
+    /** Schedule evaluation timeout */
-        scheduleUnvalidatedPrompt(network, PROMPT_UNVALIDATED_DELAY_MS);
-    }
-
-    /** Schedule unvalidated prompt for testing */
     @VisibleForTesting
-    public void scheduleUnvalidatedPrompt(@NonNull final Network network, final long delayMs) {
+    public void scheduleEvaluationTimeout(@NonNull final Network network, final long delayMs) {
-        if (VDBG) log("scheduleUnvalidatedPrompt " + network);
         mHandler.sendMessageDelayed(
-                mHandler.obtainMessage(EVENT_PROMPT_UNVALIDATED, network), delayMs);
+                mHandler.obtainMessage(EVENT_INITIAL_EVALUATION_TIMEOUT, network), delayMs);
     }
 
     @Override
@@ -5193,23 +5211,28 @@ public class ConnectivityService extends IConnectivityManager.Stub
         return false;
     }
 
-    private void handlePromptUnvalidated(Network network) {
+    private void handleInitialEvaluationTimeout(@NonNull final Network network) {
-        if (VDBG || DDBG) log("handlePromptUnvalidated " + network);
+        if (VDBG || DDBG) log("handleInitialEvaluationTimeout " + network);
-        NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network);
 
-        if (nai == null || !shouldPromptUnvalidated(nai)) {
+        NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network);
-            return;
+        if (null == nai) return;
+
+        if (nai.setEvaluated()) {
+            // If setEvaluated() returned true, the network never had any form of connectivity.
+            // This may have an impact on request matching if bad WiFi avoidance is off and the
+            // network was found not to have Internet access.
+            nai.updateScoreForNetworkAgentUpdate();
+            rematchAllNetworksAndRequests();
         }
 
+        if (!shouldPromptUnvalidated(nai)) return;
+
         // Stop automatically reconnecting to this network in the future. Automatically connecting
         // to a network that provides no or limited connectivity is not useful, because the user
         // cannot use that network except through the notification shown by this method, and the
         // notification is only shown if the network is explicitly selected by the user.
         nai.onPreventAutomaticReconnect();
 
-        // TODO: Evaluate if it's needed to wait 8 seconds for triggering notification when
-        // NetworkMonitor detects the network is partial connectivity. Need to change the design to
-        // popup the notification immediately when the network is partial connectivity.
         if (nai.partialConnectivity()) {
             showNetworkNotification(nai, NotificationType.PARTIAL_CONNECTIVITY);
         } else {
@@ -5348,8 +5371,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     handleSetAvoidUnvalidated((Network) msg.obj);
                     break;
                 }
-                case EVENT_PROMPT_UNVALIDATED: {
+                case EVENT_INITIAL_EVALUATION_TIMEOUT: {
-                    handlePromptUnvalidated((Network) msg.obj);
+                    handleInitialEvaluationTimeout((Network) msg.obj);
                     break;
                 }
                 case EVENT_CONFIGURE_ALWAYS_ON_NETWORKS: {
@@ -9213,7 +9236,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 networkAgent.networkMonitor().notifyNetworkConnected(params.linkProperties,
                         params.networkCapabilities);
             }
-            scheduleUnvalidatedPrompt(networkAgent.network);
+            scheduleEvaluationTimeout(networkAgent.network, INITIAL_EVALUATION_TIMEOUT_MS);
 
             // Whether a particular NetworkRequest listen should cause signal strength thresholds to
             // be communicated to a particular NetworkAgent depends only on the network's immutable,

service/src/com/android/server/connectivity/NetworkAgentInfo.java

@@ -61,6 +61,7 @@ import android.util.Log;
 import android.util.Pair;
 import android.util.SparseArray;
 
+import com.android.internal.annotations.VisibleForTesting;
 import com.android.internal.util.IndentingPrintWriter;
 import com.android.internal.util.WakeupMessage;
 import com.android.modules.utils.build.SdkLevel;
@@ -399,6 +400,12 @@ public class NetworkAgentInfo implements NetworkRanker.Scoreable {
         return true;
     }
 
+    /** When this network ever concluded its first evaluation, or 0 if this never happened. */
+    @VisibleForTesting
+    public long getFirstEvaluationConcludedTime() {
+        return mFirstEvaluationConcludedTime;
+    }
+
     // Delay between when the network is disconnected and when the native network is destroyed.
     public int teardownDelayMs;
 

tests/unit/java/com/android/server/ConnectivityServiceTest.java

@@ -3539,7 +3539,7 @@ public class ConnectivityServiceTest {
      **/
     private int expectUnvalidationCheckWillNotify(TestNetworkAgentWrapper agent,
             NotificationType type) {
-        mService.scheduleUnvalidatedPrompt(agent.getNetwork(), 0 /* delayMs */);
+        mService.scheduleEvaluationTimeout(agent.getNetwork(), 0 /* delayMs */);
         waitForIdle();
         return expectNotification(agent, type);
     }
@@ -3561,7 +3561,7 @@ public class ConnectivityServiceTest {
      * @return the notification ID.
      **/
     private void expectUnvalidationCheckWillNotNotify(TestNetworkAgentWrapper agent) {
-        mService.scheduleUnvalidatedPrompt(agent.getNetwork(), 0 /*delayMs */);
+        mService.scheduleEvaluationTimeout(agent.getNetwork(), 0 /*delayMs */);
         waitForIdle();
         verify(mNotificationManager, never()).notifyAsUser(anyString(), anyInt(), any(), any());
     }
@@ -3784,6 +3784,63 @@ public class ConnectivityServiceTest {
         callback.expectCallback(CallbackEntry.LOST, mCellNetworkAgent);
     }
 
+    private void doTestFirstEvaluation(
+            @NonNull final Consumer<TestNetworkAgentWrapper> doConnect,
+            final boolean waitForSecondCaps,
+            final boolean evaluatedByValidation)
+            throws Exception {
+        final NetworkRequest request = new NetworkRequest.Builder()
+                .addTransportType(TRANSPORT_WIFI)
+                .build();
+        TestNetworkCallback callback = new TestNetworkCallback();
+        mCm.registerNetworkCallback(request, callback);
+
+        mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI);
+        doConnect.accept(mWiFiNetworkAgent);
+        // Expect the available callbacks, but don't require specific values for their arguments
+        // since this method doesn't know how the network was connected.
+        callback.expectCallback(CallbackEntry.AVAILABLE, mWiFiNetworkAgent);
+        callback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mWiFiNetworkAgent);
+        callback.expectCallback(CallbackEntry.LINK_PROPERTIES_CHANGED, mWiFiNetworkAgent);
+        callback.expectCallback(CallbackEntry.BLOCKED_STATUS, mWiFiNetworkAgent);
+        if (waitForSecondCaps) {
+            // This is necessary because of b/245893397, the same bug that happens where we use
+            // expectAvailableDoubleValidatedCallbacks.
+            callback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mWiFiNetworkAgent);
+        }
+        final NetworkAgentInfo nai =
+                mService.getNetworkAgentInfoForNetwork(mWiFiNetworkAgent.getNetwork());
+        final long firstEvaluation = nai.getFirstEvaluationConcludedTime();
+        if (evaluatedByValidation) {
+            assertNotEquals(0L, firstEvaluation);
+        } else {
+            assertEquals(0L, firstEvaluation);
+        }
+        mService.scheduleEvaluationTimeout(mWiFiNetworkAgent.getNetwork(), 0L /* timeout */);
+        waitForIdle();
+        if (evaluatedByValidation) {
+            assertEquals(firstEvaluation, nai.getFirstEvaluationConcludedTime());
+        } else {
+            assertNotEquals(0L, nai.getFirstEvaluationConcludedTime());
+        }
+        mWiFiNetworkAgent.disconnect();
+        callback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent);
+
+        mCm.unregisterNetworkCallback(callback);
+    }
+
+    @Test
+    public void testEverEvaluated() throws Exception {
+        doTestFirstEvaluation(naw -> naw.connect(true /* validated */),
+                true /* waitForSecondCaps */, true /* immediatelyEvaluated */);
+        doTestFirstEvaluation(naw -> naw.connectWithPartialConnectivity(),
+                true /* waitForSecondCaps */, true /* immediatelyEvaluated */);
+        doTestFirstEvaluation(naw -> naw.connectWithCaptivePortal(TEST_REDIRECT_URL, false),
+                true /* waitForSecondCaps */, true /* immediatelyEvaluated */);
+        doTestFirstEvaluation(naw -> naw.connect(false /* validated */),
+                false /* waitForSecondCaps */, false /* immediatelyEvaluated */);
+    }
+
     private void tryNetworkFactoryRequests(int capability) throws Exception {
         // Verify NOT_RESTRICTED is set appropriately
         final NetworkCapabilities nc = new NetworkRequest.Builder().addCapability(capability)