Bug 1408472 - Read-only browserSettings should return false on write attempts, r?mixedpuppy draft
authorBob Silverberg <bsilverberg@mozilla.com>
Tue, 28 Nov 2017 12:11:23 -0500
changeset 705154 322adf9d4b9ec2ce785442703350daa939957705
parent 704194 5b33b070378ae0806bed0b5e5e34de429a29e7db
child 742269 9eb4705b946a2e2b06975499504076df96066607
push id91369
push userbmo:bob.silverberg@gmail.com
push dateWed, 29 Nov 2017 14:24:56 +0000
reviewersmixedpuppy
bugs1408472
milestone59.0a1
Bug 1408472 - Read-only browserSettings should return false on write attempts, r?mixedpuppy This updates browserSettings so that calling set and clear return false, rather than undefined, for read-only settings. It also returns "not_controllable" rather than "controllable_by_this_extension" for levelOfControl from calls to get for read-only settings. MozReview-Commit-ID: 9c0bKDaYIC7
browser/components/extensions/test/browser/browser_ext_chrome_settings_overrides_home.js
browser/components/extensions/test/xpcshell/test_ext_url_overrides_newtab.js
toolkit/components/extensions/ext-browserSettings.js
--- a/browser/components/extensions/test/browser/browser_ext_chrome_settings_overrides_home.js
+++ b/browser/components/extensions/test/browser/browser_ext_chrome_settings_overrides_home.js
@@ -8,19 +8,19 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");
 
 // Named this way so they correspond to the extensions
 const HOME_URI_2 = "http://example.com/";
 const HOME_URI_3 = "http://example.org/";
 const HOME_URI_4 = "http://example.net/";
 
-const CONTROLLABLE = "controllable_by_this_extension";
 const CONTROLLED_BY_THIS = "controlled_by_this_extension";
 const CONTROLLED_BY_OTHER = "controlled_by_other_extensions";
+const NOT_CONTROLLABLE = "not_controllable";
 
 const HOMEPAGE_URL_PREF = "browser.startup.homepage";
 
 const getHomePageURL = () => {
   return Services.prefs.getComplexValue(
     HOMEPAGE_URL_PREF, Ci.nsIPrefLocalizedString).data;
 };
 
@@ -30,21 +30,23 @@ add_task(async function test_multiple_ex
   function background() {
     browser.test.onMessage.addListener(async msg => {
       switch (msg) {
         case "checkHomepage":
           let homepage = await browser.browserSettings.homepageOverride.get({});
           browser.test.sendMessage("homepage", homepage);
           break;
         case "trySet":
-          await browser.browserSettings.homepageOverride.set({value: "foo"});
+          let setResult = await browser.browserSettings.homepageOverride.set({value: "foo"});
+          browser.test.assertFalse(setResult, "Calling homepageOverride.set returns false.");
           browser.test.sendMessage("homepageSet");
           break;
         case "tryClear":
-          await browser.browserSettings.homepageOverride.clear({});
+          let clearResult = await browser.browserSettings.homepageOverride.clear({});
+          browser.test.assertFalse(clearResult, "Calling homepageOverride.clear returns false.");
           browser.test.sendMessage("homepageCleared");
           break;
       }
     });
   }
 
   let extObj = {
     manifest: {
@@ -77,17 +79,17 @@ add_task(async function test_multiple_ex
     is(homepage.levelOfControl, expectedLevelOfControl,
        `homepageOverride setting returns the expected levelOfControl: ${expectedLevelOfControl}.`);
   }
 
   await ext1.startup();
 
   is(getHomePageURL(), defaultHomePage,
      "Home url should be the default");
-  await checkHomepageOverride(ext1, getHomePageURL(), CONTROLLABLE);
+  await checkHomepageOverride(ext1, getHomePageURL(), NOT_CONTROLLABLE);
 
   // Because we are expecting the pref to change when we start or unload, we
   // need to wait on a pref change.  This is because the pref management is
   // async and can happen after the startup/unload is finished.
   let prefPromise = promisePrefChangeObserved(HOMEPAGE_URL_PREF);
   await ext2.startup();
   await prefPromise;
 
@@ -151,17 +153,17 @@ add_task(async function test_multiple_ex
   prefPromise = promisePrefChangeObserved(HOMEPAGE_URL_PREF);
   await ext3.unload();
   await prefPromise;
 
   is(getHomePageURL(), defaultHomePage,
      "Home url should be reset to default");
 
   await ext5.startup();
-  await checkHomepageOverride(ext5, defaultHomePage, CONTROLLABLE);
+  await checkHomepageOverride(ext5, defaultHomePage, NOT_CONTROLLABLE);
   await ext5.unload();
 });
 
 const HOME_URI_1 = "http://example.com/";
 const USER_URI = "http://example.edu/";
 
 add_task(async function test_extension_setting_home_page_back() {
   let defaultHomePage = getHomePageURL();
--- a/browser/components/extensions/test/xpcshell/test_ext_url_overrides_newtab.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_url_overrides_newtab.js
@@ -39,33 +39,35 @@ function awaitEvent(eventName) {
 const DEFAULT_NEW_TAB_URL = aboutNewTabService.newTabURL;
 
 add_task(async function test_multiple_extensions_overriding_newtab_page() {
   const NEWTAB_URI_2 = "webext-newtab-1.html";
   const NEWTAB_URI_3 = "webext-newtab-2.html";
   const EXT_2_ID = "ext2@tests.mozilla.org";
   const EXT_3_ID = "ext3@tests.mozilla.org";
 
-  const CONTROLLABLE = "controllable_by_this_extension";
   const CONTROLLED_BY_THIS = "controlled_by_this_extension";
   const CONTROLLED_BY_OTHER = "controlled_by_other_extensions";
+  const NOT_CONTROLLABLE = "not_controllable";
 
   function background() {
     browser.test.onMessage.addListener(async msg => {
       switch (msg) {
         case "checkNewTabPage":
           let newTabPage = await browser.browserSettings.newTabPageOverride.get({});
           browser.test.sendMessage("newTabPage", newTabPage);
           break;
         case "trySet":
-          await browser.browserSettings.newTabPageOverride.set({value: "foo"});
+          let setResult = await browser.browserSettings.newTabPageOverride.set({value: "foo"});
+          browser.test.assertFalse(setResult, "Calling newTabPageOverride.set returns false.");
           browser.test.sendMessage("newTabPageSet");
           break;
         case "tryClear":
-          await browser.browserSettings.newTabPageOverride.clear({});
+          let clearResult = await browser.browserSettings.newTabPageOverride.clear({});
+          browser.test.assertFalse(clearResult, "Calling newTabPageOverride.clear returns false.");
           browser.test.sendMessage("newTabPageCleared");
           break;
       }
     });
   }
 
   async function checkNewTabPageOverride(ext, expectedValue, expectedLevelOfControl) {
     ext.sendMessage("checkNewTabPage");
@@ -100,17 +102,17 @@ add_task(async function test_multiple_ex
      "newTabURL is set to the default.");
 
   await promiseStartupManager();
 
   await ext1.startup();
   equal(aboutNewTabService.newTabURL, DEFAULT_NEW_TAB_URL,
        "newTabURL is still set to the default.");
 
-  await checkNewTabPageOverride(ext1, aboutNewTabService.newTabURL, CONTROLLABLE);
+  await checkNewTabPageOverride(ext1, aboutNewTabService.newTabURL, NOT_CONTROLLABLE);
 
   await ext2.startup();
   ok(aboutNewTabService.newTabURL.endsWith(NEWTAB_URI_2),
      "newTabURL is overriden by the second extension.");
   await checkNewTabPageOverride(ext1, NEWTAB_URI_2, CONTROLLED_BY_OTHER);
 
   // Verify that calling set and clear do nothing.
   ext2.sendMessage("trySet");
@@ -123,17 +125,17 @@ add_task(async function test_multiple_ex
 
   // Disable the second extension.
   let addon = await AddonManager.getAddonByID(EXT_2_ID);
   let disabledPromise = awaitEvent("shutdown");
   addon.userDisabled = true;
   await disabledPromise;
   equal(aboutNewTabService.newTabURL, DEFAULT_NEW_TAB_URL,
         "newTabURL url is reset to the default after second extension is disabled.");
-  await checkNewTabPageOverride(ext1, aboutNewTabService.newTabURL, CONTROLLABLE);
+  await checkNewTabPageOverride(ext1, aboutNewTabService.newTabURL, NOT_CONTROLLABLE);
 
   // Re-enable the second extension.
   let enabledPromise = awaitEvent("ready");
   addon.userDisabled = false;
   await enabledPromise;
   ok(aboutNewTabService.newTabURL.endsWith(NEWTAB_URI_2),
      "newTabURL is overriden by the second extension.");
   await checkNewTabPageOverride(ext2, NEWTAB_URI_2, CONTROLLED_BY_THIS);
--- a/toolkit/components/extensions/ext-browserSettings.js
+++ b/toolkit/components/extensions/ext-browserSettings.js
@@ -22,34 +22,41 @@ const HOMEPAGE_URL_PREF = "browser.start
 const URL_STORE_TYPE = "url_overrides";
 const NEW_TAB_OVERRIDE_SETTING = "newTabURL";
 
 const PERM_DENY_ACTION = Services.perms.DENY_ACTION;
 
 const getSettingsAPI = (extension, name, callback, storeType, readOnly = false) => {
   return {
     async get(details) {
-      return {
-        levelOfControl: details.incognito ?
+      let levelOfControl = details.incognito ?
+        "not_controllable" :
+        await ExtensionPreferencesManager.getLevelOfControl(
+          extension.id, name, storeType);
+      levelOfControl =
+        (readOnly && levelOfControl === "controllable_by_this_extension") ?
           "not_controllable" :
-          await ExtensionPreferencesManager.getLevelOfControl(
-            extension.id, name, storeType),
+          levelOfControl;
+      return {
+        levelOfControl,
         value: await callback(),
       };
     },
     set(details) {
       if (!readOnly) {
         return ExtensionPreferencesManager.setSetting(
           extension.id, name, details.value);
       }
+      return false;
     },
     clear(details) {
       if (!readOnly) {
         return ExtensionPreferencesManager.removeSetting(extension.id, name);
       }
+      return false;
     },
   };
 };
 
 // Add settings objects for supported APIs to the preferences manager.
 ExtensionPreferencesManager.addSetting("allowPopupsForUserEvents", {
   prefNames: [
     "dom.popup_allowed_events",