Bug 1600109 - Fix network change handling and test telemetry. r=dragana

Differential Revision: https://phabricator.services.mozilla.com/D58196

--HG--
extra : moz-landing-system : lando

browser/extensions/doh-rollout/background.js

@@ -189,7 +189,7 @@ const stateManager = {
     return !doorhangerShown;
   },
 
-  async showDoorHangerAndEnableDoH() {
+  async showDoorhanger() {
     browser.experiments.doorhanger.onDoorhangerAccept.addListener(
       rollout.doorhangerAcceptListener
     );
@@ -208,10 +208,6 @@ const stateManager = {
         "doorhangerButtonCancelAccessKey"
       ),
     });
-
-    // By default, enable DoH when showing the doorhanger,
-    // if heuristics returned no reason to not run.
-    await stateManager.setState("enabled");
   },
 };
 
@@ -246,13 +242,19 @@ const rollout = {
   },
 
   async heuristics(evaluateReason) {
+    let shouldRunHeuristics = await stateManager.shouldRunHeuristics();
+
+    if (!shouldRunHeuristics) {
+      return;
+    }
+
     // Run heuristics defined in heuristics.js and experiments/heuristics/api.js
     let results;
 
     if (await rollout.isTesting()) {
       results = await browser.experiments.preferences.getCharPref(
         MOCK_HEURISTICS_PREF,
-        "disable_doh"
+        `{ "test": "disable_doh" }`
       );
       results = JSON.parse(results);
     } else {
@@ -270,7 +272,14 @@ const rollout = {
     results.evaluateReason = evaluateReason;
     browser.experiments.heuristics.sendHeuristicsPing(decision, results);
 
-    return decision;
+    if (decision === "disable_doh") {
+      await stateManager.setState("disabled");
+    } else {
+      await stateManager.setState("enabled");
+      if (await stateManager.shouldShowDoorhanger()) {
+        await stateManager.showDoorhanger();
+      }
+    }
   },
 
   async getSetting(name, defaultValue) {
@@ -472,81 +481,67 @@ const rollout = {
       await this.enterprisePolicyCheck("startup", results);
     }
 
-    if (await stateManager.shouldRunHeuristics()) {
-      await this.runStartupHeuristics();
+    if (!(await stateManager.shouldRunHeuristics())) {
+      return;
+    }
+
+    let networkStatus = (await browser.networkStatus.getLinkInfo()).status;
+    let captiveState = "unknown";
+    try {
+      captiveState = await browser.captivePortal.getState();
+    } catch (e) {
+      // Captive Portal Service is disabled.
+    }
+
+    if (networkStatus == "up" && captiveState != "locked_portal") {
+      await rollout.heuristics("startup");
     }
 
     // Listen for network change events to run heuristics again
-    browser.networkStatus.onConnectionChanged.addListener(async () => {
-      log("onConnectionChanged");
-
-      let linkInfo = await browser.networkStatus.getLinkInfo();
-      if (linkInfo.status !== "up") {
-        log("Link down.");
-        if (rollout.networkSettledTimeout) {
-          log("Canceling queued heuristics run.");
-          clearTimeout(rollout.networkSettledTimeout);
-          rollout.networkSettledTimeout = null;
-        }
-        return;
-      }
-
-      log("Queing a heuristics run in 60s, will cancel if network fluctuates.");
-      let gracePeriod = (await rollout.isTesting()) ? 0 : 60000;
-      rollout.networkSettledTimeout = setTimeout(async () => {
-        log("No network fluctuation for 60 seconds, running heuristics.");
-        // Only run the heuristics if user hasn't explicitly enabled/disabled DoH
-        let shouldRunHeuristics = await stateManager.shouldRunHeuristics();
-        let shouldShowDoorhanger = await stateManager.shouldShowDoorhanger();
-
-        if (!shouldRunHeuristics) {
-          return;
-        }
-
-        const netChangeDecision = await rollout.heuristics("netChange");
-
-        if (netChangeDecision === "disable_doh") {
-          await stateManager.setState("disabled");
-        } else if (shouldShowDoorhanger) {
-          await stateManager.showDoorHangerAndEnableDoH();
-        } else {
-          await stateManager.setState("enabled");
-        }
-      }, gracePeriod);
-    });
+    browser.networkStatus.onConnectionChanged.addListener(
+      rollout.onConnectionChanged
+    );
 
     // Listen to the captive portal when it unlocks
-    browser.captivePortal.onConnectivityAvailable.addListener(async () => {
-      log("Captive portal onConnectivityAvailable, running heuristics.");
-      if (rollout.networkSettledTimeout) {
-        log("Canceling queued heuristics run.");
-        clearTimeout(rollout.networkSettledTimeout);
-        rollout.networkSettledTimeout = null;
-      }
-
-      let shouldRunHeuristics = await stateManager.shouldRunHeuristics();
-
-      if (!shouldRunHeuristics) {
-        return;
-      }
-
-      await this.runStartupHeuristics();
-    });
+    try {
+      browser.captivePortal.onStateChange.addListener(
+        rollout.onCaptiveStateChanged
+      );
+    } catch (e) {
+      // Captive Portal Service is disabled.
+    }
   },
 
-  async runStartupHeuristics() {
-    let decision = await this.heuristics("startup");
-    let shouldShowDoorhanger = await stateManager.shouldShowDoorhanger();
-    if (decision === "disable_doh") {
-      await stateManager.setState("disabled");
+  async onConnectionChanged({ status }) {
+    log("onConnectionChanged", status);
 
-      // If the heuristics say to enable DoH, determine if the doorhanger
-      // should be shown
-    } else if (shouldShowDoorhanger) {
-      await stateManager.showDoorHangerAndEnableDoH();
-    } else {
-      // Doorhanger has been shown before and did not opt-out
-      await stateManager.setState("enabled");
+    if (status != "up") {
+      return;
+    }
+
+    let captiveState = "unknown";
+    try {
+      captiveState = await browser.captivePortal.getState();
+    } catch (e) {
+      // Captive Portal Service is disabled.
+    }
+
+    if (captiveState == "locked_portal") {
+      return;
+    }
+
+    // The network is up and we don't know that we're in a locked portal.
+    // Run heuristics. If we detect a portal later, we'll run heuristics again
+    // when it's unlocked. In that case, this run will likely have failed.
+    await rollout.heuristics("netchange");
+  },
+
+  async onCaptiveStateChanged({ state }) {
+    log("onCaptiveStateChanged", state);
+    // unlocked_portal means we were previously in a locked portal and then
+    // network access was granted.
+    if (state == "unlocked_portal") {
+      await rollout.heuristics("netchange");
     }
   },
 };

browser/extensions/doh-rollout/test/browser/browser_cleanFlow.js

@@ -27,6 +27,7 @@ add_task(async function testCleanFlow() {
   await promise;
 
   await ensureTRRMode(2);
+  await checkHeuristicsTelemetry("enable_doh", "startup");
 
   await BrowserTestUtils.waitForCondition(() => {
     return Preferences.get(prefs.DOH_DOORHANGER_SHOWN_PREF);
@@ -51,24 +52,26 @@ add_task(async function testCleanFlow() {
   setFailingHeuristics();
   simulateNetworkChange();
   await ensureTRRMode(0);
+  await checkHeuristicsTelemetry("disable_doh", "netchange");
 
   // Trigger another network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(0);
+  await checkHeuristicsTelemetry("disable_doh", "netchange");
 
   // Restart the add-on for good measure.
   await restartAddon();
   await ensureNoTRRModeChange(0);
+  await checkHeuristicsTelemetry("disable_doh", "startup");
 
   // Set a passing environment and simulate a network change.
   setPassingHeuristics();
   simulateNetworkChange();
   await ensureTRRMode(2);
+  await checkHeuristicsTelemetry("enable_doh", "netchange");
 
   // Again, repeat and check nothing changed.
   simulateNetworkChange();
   await ensureNoTRRModeChange(2);
-
-  // Clean up.
-  await resetPrefsAndRestartAddon();
+  await checkHeuristicsTelemetry("enable_doh", "netchange");
 });

browser/extensions/doh-rollout/test/browser/browser_dirtyEnable.js

@@ -22,19 +22,20 @@ add_task(async function testDirtyEnable() {
     "Breadcrumb not saved."
   );
   await ensureNoTRRModeChange(2);
+  checkHeuristicsTelemetry("prefHasUserValue", "first_run");
 
   // Simulate a network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(2);
+  ensureNoHeuristicsTelemetry();
 
   // Restart for good measure.
   await restartAddon();
   await ensureNoTRRModeChange(2);
+  ensureNoHeuristicsTelemetry();
 
   // Simulate a network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(2);
-
-  // Clean up.
-  await resetPrefsAndRestartAddon();
+  ensureNoHeuristicsTelemetry();
 });

browser/extensions/doh-rollout/test/browser/browser_doorhangerUserReject.js

@@ -21,6 +21,7 @@ add_task(async function testDoorhangerUserReject() {
   );
 
   await ensureTRRMode(2);
+  await checkHeuristicsTelemetry("enable_doh", "startup");
 
   // Click the doorhanger's "reject" button.
   let button = panel.querySelector(".popup-notification-secondary-button");
@@ -49,20 +50,21 @@ add_task(async function testDoorhangerUserReject() {
   );
 
   await ensureTRRMode(5);
+  await checkHeuristicsTelemetry("disable_doh", "doorhangerDecline");
 
   // Simulate a network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(5);
+  ensureNoHeuristicsTelemetry();
 
   // Restart the add-on for good measure.
   await restartAddon();
   await ensureNoTRRModeChange(5);
+  ensureNoHeuristicsTelemetry();
 
   // Set failing environment and trigger another network change.
   setFailingHeuristics();
   simulateNetworkChange();
   await ensureNoTRRModeChange(5);
-
-  // Clean up.
-  await resetPrefsAndRestartAddon();
+  ensureNoHeuristicsTelemetry();
 });

browser/extensions/doh-rollout/test/browser/browser_policyOverride.js

@@ -38,25 +38,28 @@ add_task(async function testPolicyOverride() {
     "Breadcrumb not saved."
   );
   await ensureNoTRRModeChange(0);
+  await checkHeuristicsTelemetry("policy_without_doh", "first_run");
 
   // Simulate a network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(0);
+  ensureNoHeuristicsTelemetry();
 
   // Restart for good measure.
   await restartAddon();
   await ensureNoTRRModeChange(0);
+  await checkHeuristicsTelemetry("policy_without_doh", "startup");
 
   // Simulate a network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(0);
+  ensureNoHeuristicsTelemetry();
 
   // Clean up.
   await EnterprisePolicyTesting.setupPolicyEngineWithJson({
     policies: {},
   });
   EnterprisePolicyTesting.resetRunOnceState();
-  await resetPrefsAndRestartAddon();
 
   is(
     Services.policies.status,

browser/extensions/doh-rollout/test/browser/browser_userInterference.js

@@ -42,6 +42,7 @@ add_task(async function testUserInterference() {
   );
 
   await ensureTRRMode(2);
+  await checkHeuristicsTelemetry("enable_doh", "startup");
 
   // Set the TRR mode pref manually and ensure we respect this.
   Preferences.set(prefs.TRR_MODE_PREF, 0);
@@ -49,6 +50,7 @@ add_task(async function testUserInterference() {
   // Simulate a network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(0);
+  await checkHeuristicsTelemetry("disable_doh", "userModified");
 
   is(
     Preferences.get(prefs.DOH_DISABLED_PREF, false),
@@ -64,15 +66,15 @@ add_task(async function testUserInterference() {
   // Simulate another network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(0);
+  ensureNoHeuristicsTelemetry();
 
   // Restart the add-on for good measure.
   await restartAddon();
   await ensureNoTRRModeChange(0);
+  ensureNoHeuristicsTelemetry();
 
   // Simulate another network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(0);
-
-  // Clean up.
-  await resetPrefsAndRestartAddon();
+  ensureNoHeuristicsTelemetry();
 });

browser/extensions/doh-rollout/test/browser/head.js

@@ -59,12 +59,52 @@ async function setup() {
   Services.telemetry.canRecordExtended = true;
   Services.telemetry.clearEvents();
 
-  registerCleanupFunction(() => {
+  registerCleanupFunction(async () => {
     Services.telemetry.canRecordExtended = oldCanRecord;
     Services.telemetry.clearEvents();
+    await resetPrefsAndRestartAddon();
   });
 }
 
+async function checkHeuristicsTelemetry(decision, evaluateReason) {
+  let events;
+  await BrowserTestUtils.waitForCondition(() => {
+    events = Services.telemetry.snapshotEvents(
+      Ci.nsITelemetry.DATASET_PRERELEASE_CHANNELS
+    ).dynamic;
+    return events;
+  });
+  events = events.filter(
+    e => e[1] == "doh" && e[2] == "evaluate" && e[3] == "heuristics"
+  );
+  is(events.length, 1, "Found the expected heuristics event.");
+  is(events[0][4], decision, "The event records the expected decision");
+  if (evaluateReason) {
+    is(events[0][5].evaluateReason, evaluateReason, "Got the expected reason.");
+  }
+
+  // After checking the event, clear all telemetry. Since we check for a single
+  // event above, this ensures all heuristics events are intentional and tested.
+  // TODO: Test events other than heuristics. Those tests would also work the
+  // same way, so as to test one event at a time, and this clearEvents() call
+  // will continue to exist as-is.
+  Services.telemetry.clearEvents();
+}
+
+function ensureNoHeuristicsTelemetry() {
+  let events = Services.telemetry.snapshotEvents(
+    Ci.nsITelemetry.DATASET_PRERELEASE_CHANNELS
+  ).dynamic;
+  if (!events) {
+    ok(true, "Found no heuristics events.");
+    return;
+  }
+  events = events.filter(
+    e => e[1] == "doh" && e[2] == "evaluate" && e[3] == "heuristics"
+  );
+  is(events.length, 0, "Found no heuristics events.");
+}
+
 function setPassingHeuristics() {
   Preferences.set(prefs.MOCK_HEURISTICS_PREF, fakePassingHeuristics);
 }
@@ -101,7 +141,12 @@ async function waitForDoorhanger() {
 }
 
 function simulateNetworkChange() {
-  Services.obs.notifyObservers(null, "network:link-status-changed", "down");
+  // The networkStatus API does not actually propagate the link status we supply
+  // here, but rather sends the link status from the NetworkLinkService.
+  // This means there's no point sending a down and then an up - the extension
+  // will just receive "up" twice.
+  // TODO: Implement a mock NetworkLinkService and use it to also simulate
+  // network down events.
   Services.obs.notifyObservers(null, "network:link-status-changed", "up");
 }