Bug 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference, r=aswan
authorBob Silverberg <bsilverberg@mozilla.com>
Mon, 05 Jun 2017 14:22:05 -0400
changeset 363644 5583ea8ddb57bcfb00f9f50ba516c196f099e3f3
parent 363643 9bd53c082a051ed1269ee92785f9c0949e2e1178
child 363645 48fc83d872c1fc73fc55b063cd568d9962497450
push id44626
push userbsilverberg@mozilla.com
push dateTue, 13 Jun 2017 11:47:36 +0000
treeherderautoland@5583ea8ddb57 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaswan
bugs1368545
milestone56.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 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference, r=aswan Prior to this change the code that automatically updates prefs when an add-on is disabled, reenabled or uninstalled could overwrite changes to a pref that were made manually by a user, either via the UI or via about:config. This change introduces a check into each of those actions that verifies that the current state of the pref is what we expect it to be based on the data we have about add-on settings, and if it is not then we do not change the pref. MozReview-Commit-ID: 5DpEg2fGwIW
toolkit/components/extensions/ExtensionPreferencesManager.jsm
toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js
toolkit/components/extensions/test/xpcshell/test_ext_privacy.js
--- a/toolkit/components/extensions/ExtensionPreferencesManager.jsm
+++ b/toolkit/components/extensions/ExtensionPreferencesManager.jsm
@@ -75,39 +75,71 @@ function initialValueCallback() {
   let initialValue = {};
   for (let pref of this.prefNames) {
     initialValue[pref] = Preferences.get(pref);
   }
   return initialValue;
 }
 
 /**
- * Takes an item returned by the ExtensionSettingsStore and conditionally sets
- * preferences based on the item's contents.
+ * Loops through a set of prefs, either setting or resetting them.
  *
+ * @param {Object} setting
+ *        An object that represents a setting, which will have a setCallback
+ *        property.
+ * @param {Object} item
+ *        An object that represents an item handed back from the setting store
+ *        from which the new pref values can be calculated.
+*/
+function setPrefs(setting, item) {
+  let prefs = item.initialValue || setting.setCallback(item.value);
+  for (let pref in prefs) {
+    if (prefs[pref] === undefined) {
+      Preferences.reset(pref);
+    } else {
+      Preferences.set(pref, prefs[pref]);
+    }
+  }
+}
+
+/**
+ * Commits a change to a setting and conditionally sets preferences.
+ *
+ * If the change to the setting causes a different extension to gain
+ * control of the pref (or removes all extensions with control over the pref)
+ * then the prefs should be updated, otherwise they should not be.
+ * In addition, if the current value of any of the prefs does not
+ * match what we expect the value to be (which could be the result of a
+ * user manually changing the pref value), then we do not change any
+ * of the prefs.
+ *
+ * @param {Extension} extension
+ *        The extension for which a setting is being modified.
  * @param {string} name
  *        The name of the setting being processed.
- * @param {Object|null} item
- *        Either null, or an object with a value property which indicates the
- *        value stored for the setting in the settings store.
+ * @param {string} action
+ *        The action that is being performed. Will be one of disable, enable
+ *        or removeSetting.
 
  * @returns {Promise}
- *          Resolves to true if the preferences were changed and to false if
- *          the preferences were not changed.
+ *          Resolves to true if preferences were set as a result and to false
+ *          if preferences were not set.
 */
-async function processItem(name, item) {
+async function processSetting(extension, name, action) {
+  let expectedItem = await ExtensionSettingsStore.getSetting(STORE_TYPE, name);
+  let item = await ExtensionSettingsStore[action](extension, STORE_TYPE, name);
   if (item) {
-    let prefs = item.initialValue || await settingsMap.get(name).setCallback(item.value);
-    for (let pref in prefs) {
-      if (prefs[pref] === undefined) {
-        Preferences.reset(pref);
-      } else {
-        Preferences.set(pref, prefs[pref]);
-      }
+    let setting = settingsMap.get(name);
+    let expectedPrefs = expectedItem.initialValue
+      || setting.setCallback(expectedItem.value);
+    if (Object.keys(expectedPrefs).some(
+        pref => expectedPrefs[pref] && Preferences.get(pref) != expectedPrefs[pref])) {
+      return false;
     }
+    setPrefs(setting, item);
     return true;
   }
   return false;
 }
 
 this.ExtensionPreferencesManager = {
   /**
    * Adds a setting to the settingsMap. This is how an API tells the
@@ -150,71 +182,70 @@ this.ExtensionPreferencesManager = {
    * @returns {Promise}
    *          Resolves to true if the preferences were changed and to false if
    *          the preferences were not changed.
    */
   async setSetting(extension, name, value) {
     let setting = settingsMap.get(name);
     let item = await ExtensionSettingsStore.addSetting(
       extension, STORE_TYPE, name, value, initialValueCallback.bind(setting));
-    return await processItem(name, item);
+    if (item) {
+      setPrefs(setting, item);
+      return true;
+    }
+    return false;
   },
 
   /**
    * Indicates that this extension wants to temporarily cede control over the
    * given setting.
    *
    * @param {Extension} extension
    *        The extension for which a preference setting is being removed.
    * @param {string} name
    *        The unique id of the setting.
    *
    * @returns {Promise}
    *          Resolves to true if the preferences were changed and to false if
    *          the preferences were not changed.
    */
-  async disableSetting(extension, name) {
-    let item = await ExtensionSettingsStore.disable(
-      extension, STORE_TYPE, name);
-    return await processItem(name, item);
+  disableSetting(extension, name) {
+    return processSetting(extension, name, "disable");
   },
 
   /**
    * Enable a setting that has been disabled.
    *
    * @param {Extension} extension
    *        The extension for which a setting is being enabled.
    * @param {string} name
    *        The unique id of the setting.
    *
    * @returns {Promise}
    *          Resolves to true if the preferences were changed and to false if
    *          the preferences were not changed.
    */
-  async enableSetting(extension, name) {
-    let item = await ExtensionSettingsStore.enable(extension, STORE_TYPE, name);
-    return await processItem(name, item);
+  enableSetting(extension, name) {
+    return processSetting(extension, name, "enable");
   },
 
   /**
    * Indicates that this extension no longer wants to set the given setting.
    *
    * @param {Extension} extension
    *        The extension for which a preference setting is being removed.
    * @param {string} name
    *        The unique id of the setting.
    *
    * @returns {Promise}
    *          Resolves to true if the preferences were changed and to false if
    *          the preferences were not changed.
    */
-  async removeSetting(extension, name) {
-    let item = await ExtensionSettingsStore.removeSetting(
-      extension, STORE_TYPE, name);
-    return await processItem(name, item);
+  removeSetting(extension, name) {
+    return processSetting(extension, name, "removeSetting");
   },
 
   /**
    * Disables all previously set settings for an extension. This can be called when
    * an extension is being disabled, for example.
    *
    * @param {Extension} extension
    *        The extension for which all settings are being unset.
--- a/toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js
@@ -205,16 +205,51 @@ add_task(async function test_preference_
       equal(Preferences.get(settingObj.prefNames[i]), settingObj.initalValues[i],
         "removeAll unset the pref.");
     }
   }
 
   setSettings = await ExtensionSettingsStore.getAllForExtension(extensions[0], STORE_TYPE);
   deepEqual(setSettings, [], "removeAll removed all settings.");
 
+  // Tests for preventing automatic changes to manually edited prefs.
+  for (let setting in SETTINGS) {
+    let apiValue = "newValue";
+    let manualValue = "something different";
+    let settingObj = SETTINGS[setting];
+    let extension = extensions[1];
+    await ExtensionPreferencesManager.setSetting(extension, setting, apiValue);
+
+    let checkResetPrefs = (method) => {
+      let prefNames = settingObj.prefNames;
+      for (let i = 0; i < prefNames.length; i++) {
+        if (i === 0) {
+          equal(Preferences.get(prefNames[0]), manualValue,
+                `${method} did not change a manually set pref.`);
+        } else {
+          equal(Preferences.get(prefNames[i]),
+                settingObj.valueFn(prefNames[i], apiValue),
+                `${method} did not change another pref when a pref was manually set.`);
+        }
+      }
+    };
+
+    // Manually set the preference to a different value.
+    Preferences.set(settingObj.prefNames[0], manualValue);
+
+    await ExtensionPreferencesManager.disableAll(extension);
+    checkResetPrefs("disableAll");
+
+    await ExtensionPreferencesManager.enableAll(extension);
+    checkResetPrefs("enableAll");
+
+    await ExtensionPreferencesManager.removeAll(extension);
+    checkResetPrefs("removeAll");
+  }
+
   // Test with an uninitialized pref.
   let setting = "singlePref";
   let settingObj = SETTINGS[setting];
   let pref = settingObj.prefNames[0];
   let newValue = "newValue";
   Preferences.reset(pref);
   await ExtensionPreferencesManager.setSetting(extensions[1], setting, newValue);
   equal(Preferences.get(pref), settingObj.valueFn(pref, newValue),
--- a/toolkit/components/extensions/test/xpcshell/test_ext_privacy.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_privacy.js
@@ -20,17 +20,21 @@ AddonTestUtils.init(this);
 createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
 
 add_task(async function test_privacy() {
   // Create an object to hold the values to which we will initialize the prefs.
   const SETTINGS = {
     "network.networkPredictionEnabled": {
       "network.predictor.enabled": true,
       "network.prefetch-next": true,
-      "network.http.speculative-parallel-limit": 10,
+      // This pref starts with a numerical value and we need to use whatever the
+      // default is or we encounter issues when the pref is reset during the test.
+      "network.http.speculative-parallel-limit":
+        ExtensionPreferencesManager.getDefaultValue(
+          "network.http.speculative-parallel-limit"),
       "network.dns.disablePrefetch": false,
     },
     "websites.hyperlinkAuditingEnabled": {
       "browser.send_pings": true,
     },
   };
 
   async function background() {