Don't report probe status to ConnectivityService.
Late in the Q release cycle, NetworkMonitor started sending
probe results as well as evaluation results to
ConnectivityService. This resulted in passing information to
NetworkAgents at the wrong time. For example, as soon as it
connected to a network, it would report INVALID_NETWORK to
the NetworkAgents, causing Wi-Fi to disable autojoin and
telephony to detect a data stall and initiate recovery.
Unfortunately there is not enough information to correctly
suppress these messages in ConnectivityService. So, just
stop sending them. ConnectivityService doesn't use them
anyway.
Bug: 134446021
Test: atest NetworkStackTests
Change-Id: I9e3d9b9057fd017e202d056246ca2711f73d28c7
diff --git a/src/com/android/server/connectivity/NetworkMonitor.java b/src/com/android/server/connectivity/NetworkMonitor.java
index 4e40ba4..9a8b304 100644
--- a/src/com/android/server/connectivity/NetworkMonitor.java
+++ b/src/com/android/server/connectivity/NetworkMonitor.java
@@ -1034,7 +1034,7 @@
mPrivateDnsConfig = null;
validationLog("Strict mode hostname resolution failed: " + uhe.getMessage());
}
- mEvaluationState.reportProbeResult(NETWORK_VALIDATION_PROBE_PRIVDNS,
+ mEvaluationState.noteProbeResult(NETWORK_VALIDATION_PROBE_PRIVDNS,
(mPrivateDnsConfig != null) /* succeeded */);
}
@@ -1086,7 +1086,7 @@
String.format("%dms - Error: %s", time, uhe.getMessage()));
}
logValidationProbe(time, PROBE_PRIVDNS, success ? DNS_SUCCESS : DNS_FAILURE);
- mEvaluationState.reportProbeResult(NETWORK_VALIDATION_PROBE_PRIVDNS, success);
+ mEvaluationState.noteProbeResult(NETWORK_VALIDATION_PROBE_PRIVDNS, success);
return success;
}
}
@@ -2067,11 +2067,21 @@
}
// Class to keep state of evaluation results and probe results.
- // The main purpose is to ensure NetworkMonitor can notify ConnectivityService of probe results
+ //
+ // The main purpose was to ensure NetworkMonitor can notify ConnectivityService of probe results
// as soon as they happen, without triggering any other changes. This requires keeping state on
- // the most recent evaluation result. Calling reportProbeResult will ensure that the results
+ // the most recent evaluation result. Calling noteProbeResult will ensure that the results
// reported to ConnectivityService contain the previous evaluation result, and thus won't
// trigger a validation or partial connectivity state change.
+ //
+ // Note that this class is not currently being used for this purpose. The reason is that some
+ // of the system behaviour triggered by reporting network validation - notably, NetworkAgent
+ // behaviour - depends not only on the value passed by notifyNetworkTested, but also on the
+ // fact that notifyNetworkTested was called. For example, telephony triggers network recovery
+ // any time it is told that validation failed, i.e., if the result does not contain
+ // NETWORK_VALIDATION_RESULT_VALID. But with this scheme, the first two or three validation
+ // reports are all failures, because they are "HTTP succeeded but validation not yet passed",
+ // "HTTP and HTTPS succeeded but validation not yet passed", etc.
@VisibleForTesting
protected class EvaluationState {
// The latest validation result for this network. This is a bitmask of
@@ -2088,13 +2098,12 @@
}
// Probe result for http probe should be updated from reportHttpProbeResult().
- protected void reportProbeResult(int probeResult, boolean succeeded) {
+ protected void noteProbeResult(int probeResult, boolean succeeded) {
if (succeeded) {
mProbeResults |= probeResult;
} else {
mProbeResults &= ~probeResult;
}
- notifyNetworkTested(getNetworkTestResult(), mRedirectUrl);
}
protected void reportEvaluationResult(int result, @Nullable String redirectUrl) {
@@ -2138,6 +2147,6 @@
if (succeeded) {
probeResult |= NETWORK_VALIDATION_PROBE_DNS;
}
- mEvaluationState.reportProbeResult(probeResult, succeeded);
+ mEvaluationState.noteProbeResult(probeResult, succeeded);
}
}
diff --git a/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java b/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
index e4c1d17..801c5a0 100644
--- a/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
+++ b/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
@@ -684,8 +684,7 @@
@Test
public void testNoInternetCapabilityValidated() throws Exception {
- runNetworkTest(NO_INTERNET_CAPABILITIES, NETWORK_VALIDATION_RESULT_VALID,
- getGeneralVerification());
+ runNetworkTest(NO_INTERNET_CAPABILITIES, NETWORK_VALIDATION_RESULT_VALID);
verify(mCleartextDnsNetwork, never()).openConnection(any());
}
@@ -773,8 +772,9 @@
wnm.notifyPrivateDnsSettingsChanged(new PrivateDnsConfig("dns.bad", new InetAddress[0]));
// Strict mode hostname resolve fail. Expect only notification for evaluation fail. No probe
// notification.
- verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(1))
- .notifyNetworkTested(eq(VALIDATION_RESULT_VALID), eq(null));
+ verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS))
+ .notifyNetworkTested(eq(NETWORK_VALIDATION_PROBE_DNS
+ | NETWORK_VALIDATION_PROBE_HTTPS), eq(null));
// Change configuration back to working again, but make private DNS not work.
// Expect validation to fail.
@@ -930,21 +930,9 @@
nm.forceReevaluation(Process.myUid());
final ArgumentCaptor<Integer> intCaptor = ArgumentCaptor.forClass(Integer.class);
// Expect to send HTTP, HTTPs, FALLBACK and evaluation results.
- verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(4))
- .notifyNetworkTested(intCaptor.capture(), any());
- List<Integer> intArgs = intCaptor.getAllValues();
-
- // None of these exact values can be known in advance except for intArgs.get(0) because the
- // HTTP and HTTPS probes race and the order in which they complete is non-deterministic.
- // Thus, check only exact value for intArgs.get(0) and only check the validation result for
- // the rest ones.
- assertEquals(Integer.valueOf(NETWORK_VALIDATION_PROBE_DNS
- | NETWORK_VALIDATION_PROBE_FALLBACK | NETWORK_VALIDATION_RESULT_VALID),
- intArgs.get(0));
- assertTrue((intArgs.get(1) & NETWORK_VALIDATION_RESULT_VALID) != 0);
- assertTrue((intArgs.get(2) & NETWORK_VALIDATION_RESULT_VALID) != 0);
- assertTrue((intArgs.get(3) & NETWORK_VALIDATION_RESULT_PARTIAL) != 0);
- assertTrue((intArgs.get(3) & NETWORK_VALIDATION_RESULT_VALID) == 0);
+ verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS))
+ .notifyNetworkTested(eq(NETWORK_VALIDATION_PROBE_DNS |
+ NETWORK_VALIDATION_PROBE_FALLBACK | NETWORK_VALIDATION_RESULT_PARTIAL), any());
}
@Test
@@ -966,8 +954,6 @@
// Verify result should be appended and notifyNetworkTested callback is triggered once.
assertEquals(nm.getEvaluationState().getNetworkTestResult(),
VALIDATION_RESULT_VALID | NETWORK_VALIDATION_PROBE_HTTP);
- verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(1)).notifyNetworkTested(
- eq(VALIDATION_RESULT_VALID | NETWORK_VALIDATION_PROBE_HTTP), any());
nm.reportHttpProbeResult(NETWORK_VALIDATION_PROBE_HTTP, CaptivePortalProbeResult.FAILED);
// Verify DNS probe result should not be cleared.
@@ -1057,14 +1043,7 @@
}
private void runPortalNetworkTest(int result) {
- // The network test event will be triggered twice with the same result. Expect to capture
- // the second one with direct url.
- runPortalNetworkTest(result,
- (VerificationWithTimeout) timeout(HANDLER_TIMEOUT_MS).times(2));
- }
-
- private void runPortalNetworkTest(int result, VerificationWithTimeout mode) {
- runNetworkTest(result, mode);
+ runNetworkTest(result);
assertEquals(1, mRegisteredReceivers.size());
assertNotNull(mNetworkTestedRedirectUrlCaptor.getValue());
}
@@ -1101,19 +1080,14 @@
}
private NetworkMonitor runNetworkTest(int testResult) {
- return runNetworkTest(METERED_CAPABILITIES, testResult, getGeneralVerification());
+ return runNetworkTest(METERED_CAPABILITIES, testResult);
}
- private NetworkMonitor runNetworkTest(int testResult, VerificationWithTimeout mode) {
- return runNetworkTest(METERED_CAPABILITIES, testResult, mode);
- }
-
- private NetworkMonitor runNetworkTest(NetworkCapabilities nc, int testResult,
- VerificationWithTimeout mode) {
+ private NetworkMonitor runNetworkTest(NetworkCapabilities nc, int testResult) {
final NetworkMonitor monitor = makeMonitor(nc);
monitor.notifyNetworkConnected(TEST_LINK_PROPERTIES, nc);
try {
- verify(mCallbacks, mode)
+ verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS))
.notifyNetworkTested(eq(testResult), mNetworkTestedRedirectUrlCaptor.capture());
} catch (RemoteException e) {
fail("Unexpected exception: " + e);
@@ -1146,9 +1120,5 @@
}
}
- private VerificationWithTimeout getGeneralVerification() {
- return (VerificationWithTimeout) timeout(HANDLER_TIMEOUT_MS).atLeastOnce();
- }
-
}