Bug 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference, r?aswan draft
authorBob Silverberg <bsilverberg@mozilla.com>
Mon, 05 Jun 2017 14:22:05 -0400
changeset 592687 0c2a86f4b283c30c79457a3074bc6124697f5f92
parent 592551 f9605772a0c9098ed1bcaa98089b2c944ed69e9b
child 632907 c90f9a77edb4328e05bc1db62e11b8380b551f33
push id63479
push userbmo:bob.silverberg@gmail.com
push dateMon, 12 Jun 2017 17:29:14 +0000
reviewersaswan
bugs1368545
milestone55.0a1
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() {