Detect captive portals as they are tested, not upon notifying

In the previous code, ConnectivityService would mark a network
a captive portal when NetworkMonitor sends it a message to notify
the user for it. Theoretically, this is incorrect ; instead, the
marking should be done upon receiving the message that the network
was tested to be a captive portal.

In practice this hasn't been a problem so far. The captive portal
capability is observable in the app-received callbacks, but for
these purposes the old code and the new code behave the same, in
that in the callbacks, the network used to not have the portal
capability, then later it acquires it.

However, this is only not a problem as long as the captive portal
bit isn't used by network selection, which an upcoming change has
to implement for the "prefer bad wifi" feature. When that is the
case, network selection prefers the wrong result between the time
the network is marked as having been evaluated and the time the
network has been marked a captive portal. It should be marked as
both at the same time to avoid this discrepancy. This is what
this patch is accomplishing.

To do this correctly, this patch also has to disconnect the network
immediately upon it being tested as a captive portal if the old
CAPTIVE_PORTAL_MODE setting is set to CAPTIVE_PORTAL_MODE_AVOID,
instead of keeping it until NM sends the message to notify about
it.

Unfortunately because of the relative timing of the events this
patch has observable impact on what callbacks are sent because of
b/245893397 (the DoubleValidated bug). Before this patch, there
would be an extra useless callback when a captive portal validates
where the capabilities contain both VALIDATED and CAPTIVE_PORTAL,
then a second callback removing CAPTIVE_PORTAL. This patch fixes
this, but unfortunately introduces a symmetrical extra useless
callback when a captive portal network starts matching a callback,
where onCapabilitiesChanged is fired a second time after the
onAvailable sequence with the same capabilities. Fixing this is
very complex ; the best (and simplest) solution is really to
address b/245893397 decisively.

In the mean time, the behavior implemented by this patch is better
than the old one, because there no longer is an observable state
where the network is supposed to be a closed captive portal AND
validated, but instead there is a useless, but correct, caps
callback that mirrors exactly the one from b/245893397. When
b/245893397 is addressed this behavior goes away at the same time,
so this is probably the better behavior.

Test: FrameworksNetTests CtsNetTestCases
Change-Id: I6694db498507860e1d711e2aa0c591dfbfa90be2

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

@@ -3748,17 +3748,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 case EVENT_PROVISIONING_NOTIFICATION: {
                     final boolean visible = toBool(msg.arg1);
                     // If captive portal status has changed, update capabilities or disconnect.
-                    if (nai != null && (visible != nai.captivePortalDetected())) {
-                        nai.setCaptivePortalDetected(visible);
-                        if (visible && ConnectivitySettingsManager.CAPTIVE_PORTAL_MODE_AVOID
-                                == getCaptivePortalMode()) {
-                            if (DBG) log("Avoiding captive portal network: " + nai.toShortString());
-                            nai.onPreventAutomaticReconnect();
-                            teardownUnneededNetwork(nai);
-                            break;
-                        }
-                        updateCapabilitiesForNetwork(nai);
-                    }
                     if (!visible) {
                         // Only clear SIGN_IN and NETWORK_SWITCH notifications here, or else other
                         // notifications belong to the same network may be cleared unexpectedly.
@@ -3796,12 +3785,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 @NonNull NetworkAgentInfo nai, int testResult, @NonNull String redirectUrl) {
             final boolean valid = (testResult & NETWORK_VALIDATION_RESULT_VALID) != 0;
             final boolean partial = (testResult & NETWORK_VALIDATION_RESULT_PARTIAL) != 0;
-            final boolean captive = !TextUtils.isEmpty(redirectUrl);
+            final boolean portal = !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 someConnectivity = (valid || partial || portal);
             final boolean becameEvaluated = someConnectivity && nai.setEvaluated();
             // Because of b/245893397, if the score is updated when updateCapabilities is called,
             // any callback that receives onAvailable for that rematch receives an extra caps
@@ -3820,8 +3809,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
 
             final boolean wasValidated = nai.isValidated();
             final boolean wasPartial = nai.partialConnectivity();
-            nai.setPartialConnectivity((testResult & NETWORK_VALIDATION_RESULT_PARTIAL) != 0);
-            final boolean partialConnectivityChanged = (wasPartial != nai.partialConnectivity());
+            final boolean wasPortal = nai.captivePortalDetected();
+            nai.setPartialConnectivity(partial);
+            nai.setCaptivePortalDetected(portal);
+            nai.updateScoreForNetworkAgentUpdate();
+            final boolean partialConnectivityChanged = (wasPartial != partial);
+            final boolean portalChanged = (wasPortal != portal);
 
             if (DBG) {
                 final String logMsg = !TextUtils.isEmpty(redirectUrl)
@@ -3829,7 +3822,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                         : "";
                 log(nai.toShortString() + " validation " + (valid ? "passed" : "failed") + logMsg);
             }
-            if (valid != nai.isValidated()) {
+            if (valid != wasValidated) {
                 final FullScore oldScore = nai.getScore();
                 nai.setValidated(valid);
                 updateCapabilities(oldScore, nai, nai.networkCapabilities);
@@ -3852,6 +3845,16 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 }
             } else if (partialConnectivityChanged) {
                 updateCapabilitiesForNetwork(nai);
+            } else if (portalChanged) {
+                if (portal && ConnectivitySettingsManager.CAPTIVE_PORTAL_MODE_AVOID
+                        == getCaptivePortalMode()) {
+                    if (DBG) log("Avoiding captive portal network: " + nai.toShortString());
+                    nai.onPreventAutomaticReconnect();
+                    teardownUnneededNetwork(nai);
+                    return;
+                } else {
+                    updateCapabilitiesForNetwork(nai);
+                }
             } else if (becameEvaluated) {
                 // If valid or partial connectivity changed, updateCapabilities* has
                 // done the rematch.