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
--- 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() {