Bug 1477380 - Don't set startup prefs for user-branch Normandy pref-experiments. r=Gijs, a=RyanVM
authorAndreea Pavel <apavel@mozilla.com>
Mon, 30 Jul 2018 11:22:16 +0300
changeset 473819 24dd1a9e2c2c
parent 473818 b5d39c025741
child 473820 d22d9ccbec14
push id1750
push userryanvm@gmail.com
push dateMon, 06 Aug 2018 15:17:36 +0000
treeherdermozilla-release@d22d9ccbec14 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, RyanVM
bugs1477380
milestone61.0.2
Bug 1477380 - Don't set startup prefs for user-branch Normandy pref-experiments. r=Gijs, a=RyanVM Reviewers: Gijs Reviewed By: Gijs Bug #: 1477380 Differential Revision: https://phabricator.services.mozilla.com/D2305
toolkit/components/normandy/lib/PreferenceExperiments.jsm
toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
--- a/toolkit/components/normandy/lib/PreferenceExperiments.jsm
+++ b/toolkit/components/normandy/lib/PreferenceExperiments.jsm
@@ -201,42 +201,52 @@ var PreferenceExperiments = {
       );
 
       // Watch for changes to the experiment's preference
       this.startObserver(experiment.name, experiment.preferenceName, experiment.preferenceType, experiment.preferenceValue);
     }
   },
 
   /**
-   * Save in-progress preference experiments in a sub-branch of the shield
-   * prefs. On startup, we read these to set the experimental values.
+   * Save in-progress, default-branch preference experiments in a sub-branch of
+   * the normandy preferences. On startup, we read these to set the
+   * experimental values.
+   *
+   * This is needed because the default branch does not persist between Firefox
+   * restarts. To compensate for that, Normandy sets the default branch to the
+   * experiment values again every startup. The values to set the preferences
+   * to are stored in user-branch preferences because preferences have minimal
+   * impact on the performance of startup.
    */
   async saveStartupPrefs() {
     const prefBranch = Services.prefs.getBranch(STARTUP_EXPERIMENT_PREFS_BRANCH);
     prefBranch.deleteBranch("");
 
-    for (const experiment of await this.getAllActive()) {
-      const name = experiment.preferenceName;
-      const value = experiment.preferenceValue;
-
-      switch (typeof value) {
+    // Filter out non-default-branch experiments (user-branch), because they
+    // don't need to be set on the default branch during early startup. Doing so
+    // would make the user branch and the default branch the same, which would
+    // cause the user branch to not be saved, and the user branch preference
+    // would be erased.
+    const defaultBranchExperiments = (await this.getAllActive()).filter(exp => exp.preferenceBranchType === "default");
+    for (const {preferenceName, preferenceValue} of defaultBranchExperiments) {
+      switch (typeof preferenceValue) {
         case "string":
-          prefBranch.setCharPref(name, value);
+          prefBranch.setCharPref(preferenceName, preferenceValue);
           break;
 
         case "number":
-          prefBranch.setIntPref(name, value);
+          prefBranch.setIntPref(preferenceName, preferenceValue);
           break;
 
         case "boolean":
-          prefBranch.setBoolPref(name, value);
+          prefBranch.setBoolPref(preferenceName, preferenceValue);
           break;
 
         default:
-          throw new Error(`Invalid preference type ${typeof value}`);
+          throw new Error(`Invalid preference type ${typeof preferenceValue}`);
       }
     }
   },
 
   /**
    * Test wrapper that temporarily replaces the stored experiment data with fake
    * data for testing.
    */
@@ -458,17 +468,17 @@ var PreferenceExperiments = {
 
   /**
    * Stop an active experiment, deactivate preference watchers, and optionally
    * reset the associated preference to its previous value.
    * @param {string} experimentName
    * @param {Object} options
    * @param {boolean} [options.resetValue = true]
    *   If true, reset the preference to its original value prior to
-   *   the experiment. Optional, defauls to true.
+   *   the experiment. Optional, defaults to true.
    * @param {String} [options.reason = "unknown"]
    *   Reason that the experiment is ending. Optional, defaults to
    *   "unknown".
    * @rejects {Error}
    *   If there is no stored experiment with the given name, or if the
    *   experiment has already expired.
    */
   async stop(experimentName, {resetValue = true, reason = "unknown"} = {}) {
--- a/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
+++ b/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
@@ -579,17 +579,17 @@ decorate_task(
     experiments.test = experimentFactory({
       name: "test",
       expired: false,
       branch: "fakebranch",
       preferenceName: "fake.preference",
       preferenceValue: "experimentvalue",
       preferenceType: "string",
       previousPreferenceValue: "oldvalue",
-      peferenceBranchType: "default",
+      preferenceBranchType: "default",
     });
 
     await PreferenceExperiments.stop("test", {reason: "test-reason", resetValue: false});
     is(
       DefaultPreferences.get("fake.preference"),
       "customvalue",
       "stop did not modify the preference",
     );
@@ -952,16 +952,48 @@ decorate_task(
     await Assert.rejects(
       PreferenceExperiments.saveStartupPrefs(),
       /invalid preference type/i,
       "saveStartupPrefs throws if an experiment has an invalid preference value type",
     );
   },
 );
 
+// saveStartupPrefs should not store values for user-branch recipes
+decorate_task(
+  withMockExperiments,
+  async function testSaveStartupPrefsUserBranch(experiments) {
+    experiments.defaultBranchRecipe = experimentFactory({
+      preferenceName: "fake.default",
+      preferenceValue: "experiment value",
+      branch: "default",
+    });
+    experiments.userBranchRecipe = experimentFactory({
+      preferenceName: "fake.user",
+      preferenceValue: "experiment value",
+      branch: "user",
+    });
+
+    await PreferenceExperiments.saveStartupPrefs();
+
+    is(
+      Services.prefs.getCharPref(`${startupPrefs}.fake.default`, "fallback value"),
+      "experiment value",
+      "The startup value for fake.default was set",
+    );
+    ok(
+      Services.prefs.getPrefType(`${startupPrefs}.fake.user`),
+      Services.prefs.PREF_INVALID,
+      "The startup value for fake.user was not set",
+    );
+
+    Services.prefs.deleteBranch(startupPrefs);
+  },
+);
+
 // test that default branch prefs restore to the right value if the default pref changes
 decorate_task(
   withMockExperiments,
   withMockPreferences,
   withStub(PreferenceExperiments, "startObserver"),
   withStub(PreferenceExperiments, "stopObserver"),
   async function testDefaultBranchStop(mockExperiments, mockPreferences, stopObserverStub) {
     const prefName = "fake.preference";