Merge pull request #1119 from mythmon/revert-to-nothing

PR #1100 - Revert study preferences to empty when appropriate.

recipe-client-addon/bootstrap.js

@@ -45,7 +45,7 @@ const log = Log.repository.getLogger(BOOTSTRAP_LOGGER_NAME);
 log.addAppender(new Log.ConsoleAppender(new Log.BasicFormatter()));
 log.level = Services.prefs.getIntPref(PREF_LOGGING_LEVEL, Log.Level.Warn);
 
-const studyPrefsChanged = {};
+let studyPrefsChanged = {};
 
 this.Bootstrap = {
   initShieldPrefs(defaultPrefs) {
@@ -68,6 +68,7 @@ this.Bootstrap = {
   },
 
   initExperimentPrefs() {
+    studyPrefsChanged = {};
     const defaultBranch = Services.prefs.getDefaultBranch("");
     const experimentBranch = Services.prefs.getBranch(STARTUP_EXPERIMENT_PREFS_BRANCH);
 
@@ -80,31 +81,40 @@ this.Bootstrap = {
         continue;
       }
 
+      // record the value of the default branch before setting it
+      switch (realPrefType) {
+        case Services.prefs.PREF_STRING:
+          studyPrefsChanged[prefName] = defaultBranch.getCharPref(prefName);
+          break;
+
+        case Services.prefs.PREF_INT:
+          studyPrefsChanged[prefName] = defaultBranch.getIntPref(prefName);
+          break;
+
+        case Services.prefs.PREF_BOOL:
+          studyPrefsChanged[prefName] = defaultBranch.getBoolPref(prefName);
+          break;
+
+        case Services.prefs.PREF_INVALID:
+          studyPrefsChanged[prefName] = null;
+          break;
+
+        default:
+          // This should never happen
+          log.error(`Error getting startup pref ${prefName}; unknown value type ${experimentPrefType}.`);
+      }
+
+      // now set the new default value
       switch (experimentPrefType) {
         case Services.prefs.PREF_STRING:
-          try {  // eslint-disable-line mozilla/use-default-preference-values
-            studyPrefsChanged[prefName] = defaultBranch.getCharPref(prefName);
-          } catch (e) {
-            Cu.reportError(e);
-          }
           defaultBranch.setCharPref(prefName, experimentBranch.getCharPref(prefName));
           break;
 
         case Services.prefs.PREF_INT:
-          try {  // eslint-disable-line mozilla/use-default-preference-values
-            studyPrefsChanged[prefName] = defaultBranch.getIntPref(prefName);
-          } catch (e) {
-            Cu.reportError(e);
-          }
           defaultBranch.setIntPref(prefName, experimentBranch.getIntPref(prefName));
           break;
 
         case Services.prefs.PREF_BOOL:
-          try {  // eslint-disable-line mozilla/use-default-preference-values
-            studyPrefsChanged[prefName] = defaultBranch.getBoolPref(prefName);
-          } catch (e) {
-            Cu.reportError(e);
-          }
           defaultBranch.setBoolPref(prefName, experimentBranch.getBoolPref(prefName));
           break;
 

recipe-client-addon/lib/PreferenceExperiments.jsm

@@ -102,16 +102,22 @@ const log = LogManager.getLogger("preference-experiments");
 let experimentObservers = new Map();
 CleanupManager.addCleanupHandler(() => PreferenceExperiments.stopAllObservers());
 
-function getPref(prefBranch, prefName, prefType, defaultVal) {
+function getPref(prefBranch, prefName, prefType) {
+  if (prefBranch.getPrefType(prefName) === 0) {
+    // pref doesn't exist
+    return null;
+  }
+
   switch (prefType) {
-    case "boolean":
-      return prefBranch.getBoolPref(prefName, defaultVal);
+    case "boolean": {
+      return prefBranch.getBoolPref(prefName);
+    }
 
     case "string":
-      return prefBranch.getStringPref(prefName, defaultVal);
+      return prefBranch.getStringPref(prefName);
 
     case "integer":
-      return prefBranch.getIntPref(prefName, defaultVal);
+      return prefBranch.getIntPref(prefName);
 
     default:
       throw new TypeError(`Unexpected preference type (${prefType}) for ${prefName}.`);
@@ -171,7 +177,7 @@ this.PreferenceExperiments = {
 
     for (const experiment of await this.getAllActive()) {
       // Check that the current value of the preference is still what we set it to
-      if (getPref(UserPreferences, experiment.preferenceName, experiment.preferenceType, undefined) !== experiment.preferenceValue) {
+      if (getPref(UserPreferences, experiment.preferenceName, experiment.preferenceType) !== experiment.preferenceValue) {
         // if not, stop the experiment, and skip the remaining steps
         log.info(`Stopping experiment "${experiment.name}" because its value changed`);
         await this.stop(experiment.name, false);
@@ -299,7 +305,7 @@ this.PreferenceExperiments = {
       preferenceName,
       preferenceValue,
       preferenceType,
-      previousPreferenceValue: getPref(preferences, preferenceName, preferenceType, undefined),
+      previousPreferenceValue: getPref(preferences, preferenceName, preferenceType),
       preferenceBranchType,
     };
 
@@ -347,7 +353,7 @@ this.PreferenceExperiments = {
     const observerInfo = {
       preferenceName,
       observer() {
-        const newValue = getPref(UserPreferences, preferenceName, preferenceType, undefined);
+        const newValue = getPref(UserPreferences, preferenceName, preferenceType);
         if (newValue !== preferenceValue) {
           PreferenceExperiments.stop(experimentName, false)
                                .catch(Cu.reportError);
@@ -448,13 +454,19 @@ this.PreferenceExperiments = {
     if (resetValue) {
       const {preferenceName, preferenceType, previousPreferenceValue, preferenceBranchType} = experiment;
       const preferences = PreferenceBranchType[preferenceBranchType];
-      if (previousPreferenceValue !== undefined) {
+
+      if (previousPreferenceValue !== null) {
         setPref(preferences, preferenceName, preferenceType, previousPreferenceValue);
-      } else {
-        // This does nothing if we're on the default branch, which is fine. The
-        // preference will be reset on next restart, and most preferences should
-        // have had a default value set before the experiment anyway.
+      } else if (preferenceBranchType === "user") {
+        // Remove the "user set" value (which Shield set), but leave the default intact.
         preferences.clearUserPref(preferenceName);
+      } else {
+        // Remove both the user and default branch preference. This
+        // is ok because we only do this when studies expire, not
+        // when users actively leave a study by changing the
+        // preference, so there should not be a user branch value at
+        // this point.
+        Services.prefs.getDefaultBranch("").deleteBranch(preferenceName);
       }
     }
 

recipe-client-addon/test/browser/browser_PreferenceExperiments.js

@@ -427,7 +427,7 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc
     preferenceName: "fake.preference",
     preferenceValue: "experimentvalue",
     preferenceType: "string",
-    previousPreferenceValue: undefined,
+    previousPreferenceValue: null,
     preferenceBranchType: "user",
   });
 
@@ -694,7 +694,7 @@ decorate_task(
   },
 );
 
-// test that default branch prefs restore to the right value
+// test that default branch prefs restore to the right value if the default pref changes
 decorate_task(
   withMockExperiments,
   withMockPreferences,
@@ -741,3 +741,50 @@ decorate_task(
     );
   },
 );
+
+// test that default branch prefs restore to the right value if the preference is removed
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  withStub(PreferenceExperiments, "startObserver"),
+  withStub(PreferenceExperiments, "stopObserver"),
+  async function testDefaultBranchStop(mockExperiments, mockPreferences, stopObserverStub) {
+    const prefName = "fake.preference";
+    mockPreferences.set(prefName, "old version's value", "default");
+
+    // start an experiment
+    await PreferenceExperiments.start({
+      name: "test",
+      branch: "branch",
+      preferenceName: prefName,
+      preferenceValue: "experiment value",
+      preferenceBranchType: "default",
+      preferenceType: "string",
+    });
+
+    is(
+      Services.prefs.getCharPref(prefName),
+      "experiment value",
+      "Starting an experiment should change the pref",
+    );
+
+    // Now pretend that firefox has updated and restarted to a version
+    // where fake.preference has been removed in the default pref set.
+    // Bootstrap has run and changed the pref to the experimental
+    // value, and produced the call to recordOriginalValues below.
+    PreferenceExperiments.recordOriginalValues({ [prefName]: null });
+    is(
+      Services.prefs.getCharPref(prefName),
+      "experiment value",
+      "Recording original values shouldn't affect the preference."
+    );
+
+    // Now stop the experiment. It should remove the preference
+    await PreferenceExperiments.stop("test");
+    is(
+      Services.prefs.getCharPref(prefName, "DEFAULT"),
+      "DEFAULT",
+      "Preference should be absent",
+    );
+  },
+);

recipe-client-addon/test/browser/browser_bootstrap.js

@@ -1,6 +1,7 @@
 "use strict";
 
 Cu.import("resource://shield-recipe-client/lib/ShieldRecipeClient.jsm", this);
+Cu.import("resource://shield-recipe-client/lib/PreferenceExperiments.jsm", this);
 
 // We can't import bootstrap.js directly since it isn't in the jar manifest, but
 // we can use Addon.getResourceURI to get a path to the file and import using
@@ -23,6 +24,12 @@ function withBootstrap(testFunction) {
 const initPref1 = "test.initShieldPrefs1";
 const initPref2 = "test.initShieldPrefs2";
 const initPref3 = "test.initShieldPrefs3";
+
+const experimentPref1 = "test.initExperimentPrefs1";
+const experimentPref2 = "test.initExperimentPrefs2";
+const experimentPref3 = "test.initExperimentPrefs3";
+const experimentPref4 = "test.initExperimentPrefs4";
+
 decorate_task(
   withPrefEnv({
     clear: [[initPref1], [initPref2], [initPref3]],
@@ -80,9 +87,6 @@ decorate_task(
   },
 );
 
-const experimentPref1 = "test.initExperimentPrefs1";
-const experimentPref2 = "test.initExperimentPrefs2";
-const experimentPref3 = "test.initExperimentPrefs3";
 decorate_task(
   withPrefEnv({
     set: [
@@ -204,3 +208,47 @@ decorate_task(
     }
   },
 );
+
+// During startup, preferences that are changed for experiments should
+// be record by calling PreferenceExperiments.recordOriginalValues.
+decorate_task(
+  withPrefEnv({
+    set: [
+      [`extensions.shield-recipe-client.startupExperimentPrefs.${experimentPref1}`, true],
+      [`extensions.shield-recipe-client.startupExperimentPrefs.${experimentPref2}`, 2],
+      [`extensions.shield-recipe-client.startupExperimentPrefs.${experimentPref3}`, "string"],
+      [`extensions.shield-recipe-client.startupExperimentPrefs.${experimentPref4}`, "another string"],
+    ],
+    clear: [
+      [experimentPref1],
+      [experimentPref2],
+      [experimentPref3],
+      [experimentPref4],
+      ["extensions.shield-recipe-client.startupExperimentPrefs.existingPref"],
+    ],
+  }),
+  withBootstrap,
+  withStub(PreferenceExperiments, "recordOriginalValues"),
+  async function testInitExperimentPrefs(Bootstrap, recordOriginalValuesStub) {
+    const defaultBranch = Services.prefs.getDefaultBranch("");
+
+    defaultBranch.setBoolPref(experimentPref1, false);
+    defaultBranch.setIntPref(experimentPref2, 1);
+    defaultBranch.setCharPref(experimentPref3, "original string");
+    // experimentPref4 is left unset
+
+    Bootstrap.initExperimentPrefs();
+    await Bootstrap.finishStartup();
+
+    Assert.deepEqual(
+      recordOriginalValuesStub.getCall(0).args,
+      [{
+        [experimentPref1]: false,
+        [experimentPref2]: 1,
+        [experimentPref3]: "original string",
+        [experimentPref4]: null,  // null because it was not initially set.
+      }],
+      "finishStartup should record original values of the prefs initExperimentPrefs changed",
+    );
+  },
+);