Bug 1502410 - Don't use deleteBranch in Normandy. r=Gijs,adw, a=ritu
authorMichael Cooper <mcooper@mozilla.com>
Wed, 14 Nov 2018 16:47:14 +0000
changeset 501226 331cafa0d8476c853ee44ba53353383cbbd8656f
parent 501225 567929ba659f6488b3c9feeb5d2f6ec1f4ed13ba
child 501227 bc1cf13afc82c38989573aaaa3a658ac3ea0e45c
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, adw, ritu
bugs1502410, 1505941
milestone64.0
Bug 1502410 - Don't use deleteBranch in Normandy. r=Gijs,adw, a=ritu nsIPrefBranch.deleteBranch doesn't work as documented when the preference's default value was set very early after Firefox has started, such as when Normandy sets startup branches. This is filed as bug 1505941. In order to work around this problem, this patch makes Normandy never use deleteBranch, except in tests where it is safe to do so. With this patch, an experiment that is run on the default branch for a preference that does not have a default value in the tree cannot be promptly unenrolled, instead we must wait until the preference is naturally cleared when Firefox restarts. This is better than never unenrolling though. Differential Revision: https://phabricator.services.mozilla.com/D11383
browser/components/tests/browser/browser_urlbar_matchBuckets_migration60.js
toolkit/components/normandy/lib/PrefUtils.jsm
toolkit/components/normandy/lib/PreferenceExperiments.jsm
toolkit/components/normandy/lib/PreferenceRollouts.jsm
toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js
--- a/browser/components/tests/browser/browser_urlbar_matchBuckets_migration60.js
+++ b/browser/components/tests/browser/browser_urlbar_matchBuckets_migration60.js
@@ -73,16 +73,20 @@ add_task(async function setDefaultPrefAn
 
 
 // Installs the study using the pref that the overwhelming majority of users
 // will see ("ratio": 97, "value": "suggestion:4,general:5") and migrates.  The
 // study should be stopped and the pref should remain cleared.
 add_task(async function installStudyAndMigrate() {
   await sanityCheckInitialState();
 
+  // Normandy can't unset the pref if it didn't already have a value, so give it
+  // a value that will be treated as empty by the migration.
+  Services.prefs.getDefaultBranch(PREF_NAME).setCharPref("", "");
+
   // Install the study.  It should set the pref.
   await PreferenceExperiments.start(newExperimentOpts());
   Assert.ok(await PreferenceExperiments.has(STUDY_NAME),
             "Study installed");
   Assert.equal(Services.prefs.getCharPref(PREF_NAME, ""),
                PREF_VALUE_SUGGESTIONS_FIRST,
                "Pref should be set by study");
 
--- a/toolkit/components/normandy/lib/PrefUtils.jsm
+++ b/toolkit/components/normandy/lib/PrefUtils.jsm
@@ -1,17 +1,23 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * 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";
 
 ChromeUtils.import("resource://gre/modules/Services.jsm");
+ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
+ChromeUtils.defineModuleGetter(this, "LogManager", "resource://normandy/lib/LogManager.jsm");
 
 var EXPORTED_SYMBOLS = ["PrefUtils"];
 
+XPCOMUtils.defineLazyGetter(this, "log", () => {
+  return LogManager.getLogger("preference-experiments");
+});
+
 const kPrefBranches = {
   user: Services.prefs,
   default: Services.prefs.getDefaultBranch(""),
 };
 
 var PrefUtils = {
   /**
    * Get a preference from the named branch
@@ -89,19 +95,12 @@ var PrefUtils = {
    * Remove a preference from a branch.
    * @param {string} branchName One of "default" or "user"
    * @param {string} pref
    */
   clearPref(branchName, pref) {
     if (branchName === "user") {
       kPrefBranches.user.clearUserPref(pref);
     } else if (branchName === "default") {
-      // deleteBranch will affect the user branch as well. Get the user-branch
-      // value, and re-set it after clearing the pref.
-      const hadUserValue = Services.prefs.prefHasUserValue(pref);
-      const originalValue = this.getPref("user", pref, null);
-      kPrefBranches.default.deleteBranch(pref);
-      if (hadUserValue) {
-        this.setPref(branchName, pref, originalValue);
-      }
+      log.warn(`Cannot not reset pref ${pref} on the default branch. Pref will be cleared at next restart.`);
     }
   },
 };
--- a/toolkit/components/normandy/lib/PreferenceExperiments.jsm
+++ b/toolkit/components/normandy/lib/PreferenceExperiments.jsm
@@ -211,17 +211,19 @@ var PreferenceExperiments = {
    * 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 pref of prefBranch.getChildList("")) {
+      prefBranch.clearUserPref(pref);
+    }
 
     // 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) {
@@ -515,22 +517,22 @@ var PreferenceExperiments = {
       const preferences = PreferenceBranchType[preferenceBranchType];
 
       if (previousPreferenceValue !== null) {
         setPref(preferences, preferenceName, preferenceType, previousPreferenceValue);
       } 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);
+        log.warn(
+          `Can't revert pref for experiment ${experimentName} because it had no default value. `
+          + `Preference will be reset at the next restart.`
+        );
+        // It would seem that Services.prefs.deleteBranch() could be used for
+        // this, but in Normandy's case it does not work. See bug 1502410.
       }
     }
 
     experiment.expired = true;
     store.saveSoon();
 
     TelemetryEnvironment.setExperimentInactive(experimentName);
     TelemetryEvents.sendEvent("unenroll", "preference_study", experimentName, {
--- a/toolkit/components/normandy/lib/PreferenceRollouts.jsm
+++ b/toolkit/components/normandy/lib/PreferenceRollouts.jsm
@@ -217,17 +217,19 @@ var PreferenceRollouts = {
   },
 
   /**
    * Save in-progress preference rollouts in a sub-branch of the normandy prefs.
    * On startup, we read these to set the rollout values.
    */
   async saveStartupPrefs() {
     const prefBranch = Services.prefs.getBranch(STARTUP_PREFS_BRANCH);
-    prefBranch.deleteBranch("");
+    for (const pref of prefBranch.getChildList("")) {
+      prefBranch.clearUserPref(pref);
+    }
 
     for (const rollout of await this.getAllActive()) {
       for (const prefSpec of rollout.preferences) {
         PrefUtils.setPref("user", STARTUP_PREFS_BRANCH + prefSpec.preferenceName, prefSpec.value);
       }
     }
   },
 
--- a/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
+++ b/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
@@ -1065,17 +1065,17 @@ decorate_task(
     // Now stop the experiment. It should remove the preference
     await PreferenceExperiments.stop("test");
     is(
       Services.prefs.getCharPref(prefName, "DEFAULT"),
       "DEFAULT",
       "Preference should be absent",
     );
   },
-);
+).skip(/* bug 1502410 and bug 1505941 */);
 
 // stop should pass "unknown" to telemetry event for `reason` if none is specified
 decorate_task(
   withMockExperiments([experimentFactory({ name: "test", preferenceName: "fake.preference" })]),
   withMockPreferences,
   withStub(PreferenceExperiments, "stopObserver"),
   withSendEventStub,
   async function testStopUnknownReason(experiments, mockPreferences, stopObserverStub, sendEventStub) {
--- a/toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js
@@ -106,17 +106,18 @@ decorate_task(
       {preferenceName: "test.pref2", value: 2},
       // pref3 is added
       {preferenceName: "test.pref3", value: 2},
     ];
     action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
 
-    is(Services.prefs.getPrefType("test.pref1"), Services.prefs.PREF_INVALID, "pref1 should be removed");
+    /* Todo because of bug 1502410 and bug 1505941 */
+    todo_is(Services.prefs.getPrefType("test.pref1"), Services.prefs.PREF_INVALID, "pref1 should be removed");
     is(Services.prefs.getIntPref("test.pref2"), 2, "pref2 should be updated");
     is(Services.prefs.getIntPref("test.pref3"), 2, "pref3 should be added");
 
     is(Services.prefs.getPrefType(
       "app.normandy.startupRolloutPrefs.test.pref1"),
       Services.prefs.PREF_INVALID,
       "startup pref1 should be removed",
     );