Bug 1502410 - Don't use deleteBranch in Normandy r=Gijs

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

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

toolkit/components/normandy/lib/PrefUtils.jsm

@@ -4,9 +4,15 @@
 "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(""),
@@ -94,14 +100,7 @@ var PrefUtils = {
     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.`);
     }
   },
 };

toolkit/components/normandy/lib/PreferenceExperiments.jsm

@@ -216,7 +216,9 @@ var PreferenceExperiments = {
    */
   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
@@ -520,12 +522,12 @@ var PreferenceExperiments = {
         // 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.
       }
     }
 

toolkit/components/normandy/lib/PreferenceRollouts.jsm

@@ -225,7 +225,9 @@ var PreferenceRollouts = {
    */
   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) {

toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js

@@ -1077,7 +1077,7 @@ decorate_task(
       "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(

toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js

@@ -111,7 +111,8 @@ decorate_task(
     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");