Bug 1638502 - [DoH] Persist TRR-selection dry-run result. r=valentin,johannh
☠☠ backed out by f4424a8a020d ☠ ☠
authorNihanth Subramanya <nhnt11@gmail.com>
Wed, 20 May 2020 13:31:15 +0000
changeset 531817 46fe0af8f03dadf116cb3cf8a86377d594b2a7ac
parent 531816 6191dcf6f130dd30fe25b93836231b0d122bc3e9
child 531818 b32ad3762d7d64aa0c1ed07f81cb058c8a7b041c
push id116892
push usernhnt11@gmail.com
push dateSun, 24 May 2020 07:52:09 +0000
treeherderautoland@46fe0af8f03d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvalentin, johannh
bugs1638502
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1638502 - [DoH] Persist TRR-selection dry-run result. r=valentin,johannh Differential Revision: https://phabricator.services.mozilla.com/D75660
browser/extensions/doh-rollout/background.js
browser/extensions/doh-rollout/experiments/trrselect/api.js
browser/extensions/doh-rollout/experiments/trrselect/schema.json
browser/extensions/doh-rollout/test/browser/browser.ini
browser/extensions/doh-rollout/test/browser/browser_cleanFlow.js
browser/extensions/doh-rollout/test/browser/browser_dirtyEnable.js
browser/extensions/doh-rollout/test/browser/browser_doorhangerUserReject.js
browser/extensions/doh-rollout/test/browser/browser_doorhanger_newProfile.js
browser/extensions/doh-rollout/test/browser/browser_policyOverride.js
browser/extensions/doh-rollout/test/browser/browser_rollback.js
browser/extensions/doh-rollout/test/browser/browser_trrSelect.js
browser/extensions/doh-rollout/test/browser/browser_userInterference.js
browser/extensions/doh-rollout/test/browser/head.js
--- a/browser/extensions/doh-rollout/background.js
+++ b/browser/extensions/doh-rollout/background.js
@@ -511,18 +511,18 @@ const rollout = {
       await this.enterprisePolicyCheck("startup", results);
     }
 
     if (!(await stateManager.shouldRunHeuristics())) {
       return;
     }
 
     // Perform TRR selection before running heuristics.
-    await browser.experiments.trrselect.dryRun();
-    log("TRR selection dry run complete!");
+    await browser.experiments.trrselect.run();
+    log("TRR selection complete!");
 
     let networkStatus = (await browser.networkStatus.getLinkInfo()).status;
     let captiveState = "unknown";
     try {
       captiveState = await browser.captivePortal.getState();
     } catch (e) {
       // Captive Portal Service is disabled.
     }
--- a/browser/extensions/doh-rollout/experiments/trrselect/api.js
+++ b/browser/extensions/doh-rollout/experiments/trrselect/api.js
@@ -2,32 +2,45 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 /* global Cc, Ci, ExtensionAPI, TRRRacer  */
 
 ChromeUtils.import("resource://gre/modules/Services.jsm", this);
-ChromeUtils.import("resource:///modules/TRRPerformance.jsm", this);
 
+const kCommitSelectionPref = "doh-rollout.trr-selection.commit-result";
 const kDryRunResultPref = "doh-rollout.trr-selection.dry-run-result";
+const kRolloutURIPref = "doh-rollout.uri";
+const kTRRListPref = "network.trr.resolvers";
 
 const TRRSELECT_TELEMETRY_CATEGORY = "security.doh.trrPerformance";
 
 Services.telemetry.setEventRecordingEnabled(TRRSELECT_TELEMETRY_CATEGORY, true);
 
 this.trrselect = class trrselect extends ExtensionAPI {
   getAPI() {
     return {
       experiments: {
         trrselect: {
           async dryRun() {
             if (Services.prefs.prefHasUserValue(kDryRunResultPref)) {
-              return;
+              // Check whether the existing dry-run-result is in the default
+              // list of TRRs. If it is, all good. Else, run the dry run again.
+              let dryRunResult = Services.prefs.getCharPref(kDryRunResultPref);
+              let defaultTRRs = JSON.parse(
+                Services.prefs.getDefaultBranch("").getCharPref(kTRRListPref)
+              );
+              let dryRunResultIsValid = defaultTRRs.some(
+                trr => trr.url == dryRunResult
+              );
+              if (dryRunResultIsValid) {
+                return;
+              }
             }
 
             let setDryRunResultAndRecordTelemetry = trr => {
               Services.prefs.setCharPref(kDryRunResultPref, trr);
               Services.telemetry.recordEvent(
                 TRRSELECT_TELEMETRY_CATEGORY,
                 "trrselect",
                 "dryrunresult",
@@ -37,21 +50,45 @@ this.trrselect = class trrselect extends
 
             if (Cu.isInAutomation) {
               // For mochitests, just record telemetry with a dummy result.
               // TRRPerformance.jsm is tested in xpcshell.
               setDryRunResultAndRecordTelemetry("dummyTRR");
               return;
             }
 
+            // Importing the module here saves us from having to do it at add-on
+            // startup, and ensures tests have time to set prefs before the
+            // module initializes.
+            let { TRRRacer } = ChromeUtils.import(
+              "resource:///modules/TRRPerformance.jsm"
+            );
             await new Promise(resolve => {
               let racer = new TRRRacer(() => {
                 setDryRunResultAndRecordTelemetry(racer.getFastestTRR(true));
                 resolve();
               });
               racer.run();
             });
           },
+
+          async run() {
+            if (Services.prefs.prefHasUserValue(kRolloutURIPref)) {
+              return;
+            }
+
+            await this.dryRun();
+
+            // If persisting the selection is disabled, don't commit the value.
+            if (!Services.prefs.getBoolPref(kCommitSelectionPref, false)) {
+              return;
+            }
+
+            Services.prefs.setCharPref(
+              kRolloutURIPref,
+              Services.prefs.getCharPref(kDryRunResultPref)
+            );
+          },
         },
       },
     };
   }
 };
--- a/browser/extensions/doh-rollout/experiments/trrselect/schema.json
+++ b/browser/extensions/doh-rollout/experiments/trrselect/schema.json
@@ -1,15 +1,15 @@
 [
   {
     "namespace": "experiments.trrselect",
     "description": "API for running TRR performance measurement",
     "functions": [
       {
-        "name": "dryRun",
+        "name": "run",
         "type": "function",
-        "description": "Runs TRR performance measurement and stores fastest TRR in a pref",
+        "description": "Runs TRR performance measurement if necessary and commits best TRR for the client",
         "parameters": [],
         "async": true
       }
     ]
   }
 ]
--- a/browser/extensions/doh-rollout/test/browser/browser.ini
+++ b/browser/extensions/doh-rollout/test/browser/browser.ini
@@ -5,10 +5,11 @@ skip-if = debug # Bug 1548006 - reloadin
 [browser_cleanFlow.js]
 [browser_dirtyEnable.js]
 [browser_doorhangerUserReject.js]
 [browser_doorhanger_newProfile.js]
 [browser_policyOverride.js]
 skip-if = (!debug && bits == 64) #Bug 1605297
 [browser_rollback.js]
 skip-if = asan && !debug && os == 'linux' && bits == 64 && os_version == '18.04' # Bug 1613994 for linux1804
+[browser_trrSelect.js]
 [browser_userInterference.js]
 skip-if = asan && !debug && os == 'linux' && bits == 64 && os_version == '18.04' # Bug 1615815 for linux1804
--- a/browser/extensions/doh-rollout/test/browser/browser_cleanFlow.js
+++ b/browser/extensions/doh-rollout/test/browser/browser_cleanFlow.js
@@ -8,19 +8,19 @@ add_task(async function testCleanFlow() 
   let promise = waitForDoorhanger();
   Preferences.set(prefs.DOH_ENABLED_PREF, true);
 
   await BrowserTestUtils.waitForCondition(() => {
     return Preferences.get(prefs.DOH_SELF_ENABLED_PREF);
   });
   is(Preferences.get(prefs.DOH_SELF_ENABLED_PREF), true, "Breadcrumb saved.");
   is(
-    Preferences.get(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF),
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
     "dummyTRR",
-    "TRR selection dry run complete."
+    "TRR selection complete."
   );
   await checkTRRSelectionTelemetry();
 
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, EXAMPLE_URL);
   let panel = await promise;
   is(
     Preferences.get(prefs.DOH_DOORHANGER_SHOWN_PREF),
     undefined,
--- a/browser/extensions/doh-rollout/test/browser/browser_dirtyEnable.js
+++ b/browser/extensions/doh-rollout/test/browser/browser_dirtyEnable.js
@@ -17,19 +17,19 @@ add_task(async function testDirtyEnable(
     "Disabled state recorded."
   );
   is(
     Preferences.get(prefs.DOH_SELF_ENABLED_PREF),
     undefined,
     "Breadcrumb not saved."
   );
   is(
-    Preferences.get(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF),
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
     undefined,
-    "TRR selection dry run not performed."
+    "TRR selection not performed."
   );
   ensureNoTRRSelectionTelemetry();
   await ensureNoTRRModeChange(2);
   checkHeuristicsTelemetry("prefHasUserValue", "first_run");
 
   // Simulate a network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(2);
--- a/browser/extensions/doh-rollout/test/browser/browser_doorhangerUserReject.js
+++ b/browser/extensions/doh-rollout/test/browser/browser_doorhangerUserReject.js
@@ -8,19 +8,19 @@ add_task(async function testDoorhangerUs
   let promise = waitForDoorhanger();
   Preferences.set(prefs.DOH_ENABLED_PREF, true);
 
   await BrowserTestUtils.waitForCondition(() => {
     return Preferences.get(prefs.DOH_SELF_ENABLED_PREF);
   });
   is(Preferences.get(prefs.DOH_SELF_ENABLED_PREF), true, "Breadcrumb saved.");
   is(
-    Preferences.get(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF),
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
     "dummyTRR",
-    "TRR selection dry run complete."
+    "TRR selection complete."
   );
   await checkTRRSelectionTelemetry();
 
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, EXAMPLE_URL);
   let panel = await promise;
   is(
     Preferences.get(prefs.DOH_DOORHANGER_SHOWN_PREF),
     undefined,
--- a/browser/extensions/doh-rollout/test/browser/browser_doorhanger_newProfile.js
+++ b/browser/extensions/doh-rollout/test/browser/browser_doorhanger_newProfile.js
@@ -8,19 +8,19 @@ add_task(async function testDoorhanger()
   setPassingHeuristics();
   Preferences.set(prefs.DOH_ENABLED_PREF, true);
 
   await BrowserTestUtils.waitForCondition(() => {
     return Preferences.get(prefs.DOH_SELF_ENABLED_PREF);
   });
   is(Preferences.get(prefs.DOH_SELF_ENABLED_PREF), true, "Breadcrumb saved.");
   is(
-    Preferences.get(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF),
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
     "dummyTRR",
-    "TRR selection dry run complete."
+    "TRR selection complete."
   );
   await checkTRRSelectionTelemetry();
 
   await BrowserTestUtils.waitForCondition(() => {
     return Preferences.get(prefs.DOH_DOORHANGER_SHOWN_PREF);
   });
   is(
     Preferences.get(prefs.DOH_DOORHANGER_SHOWN_PREF),
--- a/browser/extensions/doh-rollout/test/browser/browser_policyOverride.js
+++ b/browser/extensions/doh-rollout/test/browser/browser_policyOverride.js
@@ -33,19 +33,19 @@ add_task(async function testPolicyOverri
     "skipHeuristicsCheck pref is set to remember not to run heuristics."
   );
   is(
     Preferences.get(prefs.DOH_SELF_ENABLED_PREF),
     undefined,
     "Breadcrumb not saved."
   );
   is(
-    Preferences.get(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF),
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
     undefined,
-    "TRR selection dry run not performed."
+    "TRR selection not performed."
   );
   ensureNoTRRSelectionTelemetry();
   await ensureNoTRRModeChange(0);
   await checkHeuristicsTelemetry("policy_without_doh", "first_run");
 
   // Simulate a network change.
   simulateNetworkChange();
   await ensureNoTRRModeChange(0);
--- a/browser/extensions/doh-rollout/test/browser/browser_rollback.js
+++ b/browser/extensions/doh-rollout/test/browser/browser_rollback.js
@@ -10,19 +10,19 @@ add_task(async function testRollback() {
   let promise = waitForDoorhanger();
   Preferences.set(prefs.DOH_ENABLED_PREF, true);
 
   await BrowserTestUtils.waitForCondition(() => {
     return Preferences.get(prefs.DOH_SELF_ENABLED_PREF);
   });
   is(Preferences.get(prefs.DOH_SELF_ENABLED_PREF), true, "Breadcrumb saved.");
   is(
-    Preferences.get(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF),
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
     "dummyTRR",
-    "TRR selection dry run complete."
+    "TRR selection complete."
   );
   await checkTRRSelectionTelemetry();
 
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, EXAMPLE_URL);
   let panel = await promise;
   is(
     Preferences.get(prefs.DOH_DOORHANGER_SHOWN_PREF),
     undefined,
new file mode 100644
--- /dev/null
+++ b/browser/extensions/doh-rollout/test/browser/browser_trrSelect.js
@@ -0,0 +1,99 @@
+"use strict";
+
+add_task(setup);
+
+add_task(async function testTRRSelect() {
+  // Set up the resolver lists in the default and user pref branches.
+  // dummyTRR3 which only exists in the user-branch value should be ignored.
+  let oldResolverList = Services.prefs.getCharPref("network.trr.resolvers");
+  Services.prefs
+    .getDefaultBranch("")
+    .setCharPref(
+      "network.trr.resolvers",
+      `[{"url": "dummyTRR"}, {"url": "dummyTRR2"}]`
+    );
+  Services.prefs.setCharPref(
+    "network.trr.resolvers",
+    `[{"url": "dummyTRR"}, {"url": "dummyTRR2"}, {"url": "dummyTRR3"}]`
+  );
+
+  // Clean start: doh-rollout.uri should be set after init.
+  setPassingHeuristics();
+  Preferences.set(prefs.DOH_ENABLED_PREF, true);
+  await BrowserTestUtils.waitForCondition(() => {
+    return Preferences.get(prefs.DOH_SELF_ENABLED_PREF);
+  });
+  is(Preferences.get(prefs.DOH_SELF_ENABLED_PREF), true, "Breadcrumb saved.");
+  is(
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
+    "dummyTRR",
+    "TRR selection complete."
+  );
+
+  // Reset and restart add-on for good measure.
+  Preferences.reset(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF);
+  Preferences.reset(prefs.DOH_TRR_SELECT_URI_PREF);
+  await restartAddon();
+  await BrowserTestUtils.waitForCondition(() => {
+    return Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF);
+  });
+  is(
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
+    "dummyTRR",
+    "TRR selection complete."
+  );
+
+  // Reset and restart add-on again, but this time with committing disabled.
+  // dry-run-result should be recorded but not be committed.
+  Preferences.reset(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF);
+  Preferences.reset(prefs.DOH_TRR_SELECT_URI_PREF);
+  Preferences.set(prefs.DOH_TRR_SELECT_COMMIT_PREF, false);
+  await restartAddon();
+  try {
+    await BrowserTestUtils.waitForCondition(() => {
+      return Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF);
+    });
+    ok(false, "Dry run result got committed, fail!");
+  } catch (e) {
+    ok(true, "Dry run result did not get committed");
+  }
+  is(
+    Preferences.get(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF),
+    "dummyTRR",
+    "TRR selection complete, dry-run result recorded."
+  );
+  Preferences.set(prefs.DOH_TRR_SELECT_COMMIT_PREF, true);
+
+  // Reset doh-rollout.uri, and change the dry-run-result to another one on the
+  // default list. After init, the existing dry-run-result should be committed.
+  Preferences.reset(prefs.DOH_TRR_SELECT_URI_PREF);
+  Preferences.set(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF, "dummyTRR2");
+  await restartAddon();
+  await BrowserTestUtils.waitForCondition(() => {
+    return Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF);
+  });
+  is(
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
+    "dummyTRR2",
+    "TRR selection complete, existing dry-run-result committed."
+  );
+
+  // Reset doh-rollout.uri, and change the dry-run-result to another one NOT on
+  // default list. After init, a new TRR should be selected and committed.
+  Preferences.reset(prefs.DOH_TRR_SELECT_URI_PREF);
+  Preferences.set(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF, "dummyTRR3");
+  await restartAddon();
+  await BrowserTestUtils.waitForCondition(() => {
+    return Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF);
+  });
+  is(
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
+    "dummyTRR",
+    "TRR selection complete, existing dry-run-result discarded and refreshed."
+  );
+
+  Services.prefs
+    .getDefaultBranch("")
+    .setCharPref("network.trr.resolvers", oldResolverList);
+  Services.prefs.clearUserPref("network.trr.resolvers");
+});
--- a/browser/extensions/doh-rollout/test/browser/browser_userInterference.js
+++ b/browser/extensions/doh-rollout/test/browser/browser_userInterference.js
@@ -8,19 +8,19 @@ add_task(async function testUserInterfer
   let promise = waitForDoorhanger();
   Preferences.set(prefs.DOH_ENABLED_PREF, true);
 
   await BrowserTestUtils.waitForCondition(() => {
     return Preferences.get(prefs.DOH_SELF_ENABLED_PREF);
   });
   is(Preferences.get(prefs.DOH_SELF_ENABLED_PREF), true, "Breadcrumb saved.");
   is(
-    Preferences.get(prefs.DOH_TRR_SELECT_DRY_RUN_RESULT_PREF),
+    Preferences.get(prefs.DOH_TRR_SELECT_URI_PREF),
     "dummyTRR",
-    "TRR selection dry run complete."
+    "TRR selection complete."
   );
   await checkTRRSelectionTelemetry();
 
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, EXAMPLE_URL);
   let panel = await promise;
   is(
     Preferences.get(prefs.DOH_DOORHANGER_SHOWN_PREF),
     undefined,
--- a/browser/extensions/doh-rollout/test/browser/head.js
+++ b/browser/extensions/doh-rollout/test/browser/head.js
@@ -27,16 +27,18 @@ const prefs = {
   DOH_PREVIOUS_TRR_MODE_PREF: "doh-rollout.previous.trr.mode",
   DOH_DOORHANGER_SHOWN_PREF: "doh-rollout.doorhanger-shown",
   DOH_DOORHANGER_USER_DECISION_PREF: "doh-rollout.doorhanger-decision",
   DOH_DISABLED_PREF: "doh-rollout.disable-heuristics",
   DOH_SKIP_HEURISTICS_PREF: "doh-rollout.skipHeuristicsCheck",
   DOH_DONE_FIRST_RUN_PREF: "doh-rollout.doneFirstRun",
   DOH_BALROG_MIGRATION_PREF: "doh-rollout.balrog-migration-done",
   DOH_DEBUG_PREF: "doh-rollout.debug",
+  DOH_TRR_SELECT_URI_PREF: "doh-rollout.uri",
+  DOH_TRR_SELECT_COMMIT_PREF: "doh-rollout.trr-selection.commit-result",
   DOH_TRR_SELECT_DRY_RUN_RESULT_PREF:
     "doh-rollout.trr-selection.dry-run-result",
   MOCK_HEURISTICS_PREF: "doh-rollout.heuristics.mockValues",
   PROFILE_CREATION_THRESHOLD_PREF: "doh-rollout.profileCreationThreshold",
 };
 
 const fakePassingHeuristics = JSON.stringify({
   google: "enable_doh",
@@ -68,16 +70,20 @@ async function setup() {
   Services.telemetry.canRecordExtended = true;
   Services.telemetry.clearEvents();
 
   // Set the profile creation threshold to very far in the future by defualt,
   // so that we can test the doorhanger. browser_doorhanger_newProfile.js
   // overrides this.
   Preferences.set(prefs.PROFILE_CREATION_THRESHOLD_PREF, "99999999999999");
 
+  // Enable committing the TRR selection. This pref ships false by default so
+  // it can be controlled e.g. via Normandy, but for testing let's set enable.
+  Preferences.set(prefs.DOH_TRR_SELECT_COMMIT_PREF, true);
+
   registerCleanupFunction(async () => {
     Services.telemetry.canRecordExtended = oldCanRecord;
     Services.telemetry.clearEvents();
     await resetPrefsAndRestartAddon();
   });
 }
 
 async function checkTRRSelectionTelemetry() {