Bug 1381605 - Do not load ExtensionSettingsStore's JSON file synchronously, r?aswan draft
authorBob Silverberg <bsilverberg@mozilla.com>
Mon, 24 Jul 2017 15:49:30 -0400
changeset 616648 2b02aa2e35f61ca72906f5afddea3858749da672
parent 616647 e5693cea1ec944ca077c7a46c5f127c828a90f1b
child 639536 06ccffcc0f901a75f64925b95b340e54da12c475
push id70752
push userbmo:bob.silverberg@gmail.com
push dateThu, 27 Jul 2017 09:04:51 +0000
reviewersaswan
bugs1381605
milestone56.0a1
Bug 1381605 - Do not load ExtensionSettingsStore's JSON file synchronously, r?aswan MozReview-Commit-ID: 7lQp9hL9pNd
browser/components/extensions/ext-url-overrides.js
toolkit/components/extensions/ExtensionPreferencesManager.jsm
toolkit/components/extensions/ExtensionSettingsStore.jsm
toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js
--- a/browser/components/extensions/ext-url-overrides.js
+++ b/browser/components/extensions/ext-url-overrides.js
@@ -22,16 +22,18 @@ this.urlOverrides = class extends Extens
       aboutNewTabService.newTabURL = item.value || item.initialValue;
     }
   }
 
   async onManifestEntry(entryName) {
     let {extension} = this;
     let {manifest} = extension;
 
+    await ExtensionSettingsStore.initialize();
+
     if (manifest.chrome_url_overrides.newtab) {
       // Set up the shutdown code for the setting.
       extension.callOnClose({
         close: () => {
           switch (extension.shutdownReason) {
             case "ADDON_DISABLE":
               this.processNewTabSetting("disable");
               break;
--- a/toolkit/components/extensions/ExtensionPreferencesManager.jsm
+++ b/toolkit/components/extensions/ExtensionPreferencesManager.jsm
@@ -119,18 +119,19 @@ function setPrefs(setting, item) {
  *        The action that is being performed. Will be one of disable, enable
  *        or removeSetting.
 
  * @returns {Promise}
  *          Resolves to true if preferences were set as a result and to false
  *          if preferences were not set.
 */
 async function processSetting(extension, name, action) {
-  let expectedItem = await ExtensionSettingsStore.getSetting(STORE_TYPE, name);
-  let item = await ExtensionSettingsStore[action](extension, STORE_TYPE, name);
+  await ExtensionSettingsStore.initialize();
+  let expectedItem = ExtensionSettingsStore.getSetting(STORE_TYPE, name);
+  let item = ExtensionSettingsStore[action](extension, STORE_TYPE, name);
   if (item) {
     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;
     }
@@ -180,16 +181,17 @@ this.ExtensionPreferencesManager = {
    *        group of preferences.
    *
    * @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);
+    await ExtensionSettingsStore.initialize();
     let item = await ExtensionSettingsStore.addSetting(
       extension, STORE_TYPE, name, value, initialValueCallback.bind(setting));
     if (item) {
       setPrefs(setting, item);
       return true;
     }
     return false;
   },
@@ -246,49 +248,52 @@ this.ExtensionPreferencesManager = {
   /**
    * 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.
    */
   async disableAll(extension) {
-    let settings = await ExtensionSettingsStore.getAllForExtension(extension, STORE_TYPE);
+    await ExtensionSettingsStore.initialize();
+    let settings = ExtensionSettingsStore.getAllForExtension(extension, STORE_TYPE);
     let disablePromises = [];
     for (let name of settings) {
       disablePromises.push(this.disableSetting(extension, name));
     }
     await Promise.all(disablePromises);
   },
 
   /**
    * Enables all disabled settings for an extension. This can be called when
    * an extension has finsihed updating or is being re-enabled, for example.
    *
    * @param {Extension} extension
    *        The extension for which all settings are being enabled.
    */
   async enableAll(extension) {
-    let settings = await ExtensionSettingsStore.getAllForExtension(extension, STORE_TYPE);
+    await ExtensionSettingsStore.initialize();
+    let settings = ExtensionSettingsStore.getAllForExtension(extension, STORE_TYPE);
     let enablePromises = [];
     for (let name of settings) {
       enablePromises.push(this.enableSetting(extension, name));
     }
     await Promise.all(enablePromises);
   },
 
   /**
    * Removes all previously set settings for an extension. This can be called when
    * an extension is being uninstalled, for example.
    *
    * @param {Extension} extension
    *        The extension for which all settings are being unset.
    */
   async removeAll(extension) {
-    let settings = await ExtensionSettingsStore.getAllForExtension(extension, STORE_TYPE);
+    await ExtensionSettingsStore.initialize();
+    let settings = ExtensionSettingsStore.getAllForExtension(extension, STORE_TYPE);
     let removePromises = [];
     for (let name of settings) {
       removePromises.push(this.removeSetting(extension, name));
     }
     await Promise.all(removePromises);
   },
 
   /**
@@ -305,11 +310,12 @@ this.ExtensionPreferencesManager = {
    *          Resolves to the level of control of the extension over the setting.
    */
   async getLevelOfControl(extension, name) {
     for (let prefName of settingsMap.get(name).prefNames) {
       if (Preferences.locked(prefName)) {
         return "not_controllable";
       }
     }
+    await ExtensionSettingsStore.initialize();
     return ExtensionSettingsStore.getLevelOfControl(extension, STORE_TYPE, name);
   },
 };
--- a/toolkit/components/extensions/ExtensionSettingsStore.jsm
+++ b/toolkit/components/extensions/ExtensionSettingsStore.jsm
@@ -51,25 +51,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/AddonManager.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "JSONFile",
                                   "resource://gre/modules/JSONFile.jsm");
 
 const JSON_FILE_NAME = "extension-settings.json";
 const JSON_FILE_VERSION = 2;
 const STORE_PATH = OS.Path.join(Services.dirsvc.get("ProfD", Ci.nsIFile).path, JSON_FILE_NAME);
 
-let _store;
-
-// Test-only method to force reloading of the JSON file, by removing the
-// cached file from memory.
-function clearFileFromCache() {
-  let finalizePromise = _store.finalize();
-  _store = null;
-  return finalizePromise;
-}
+let _initializePromise;
+let _store = {};
 
 // Processes the JSON data when read from disk to convert string dates into numbers.
 function dataPostProcessor(json) {
   if (json.version !== JSON_FILE_VERSION) {
     for (let storeType in json) {
       for (let setting in json[storeType]) {
         for (let extData of json[storeType][setting].precedenceList) {
           if (typeof extData.installDate != "number") {
@@ -78,41 +71,54 @@ function dataPostProcessor(json) {
         }
       }
     }
     json.version = JSON_FILE_VERSION;
   }
   return json;
 }
 
-// Get the internal settings store, which is persisted in a JSON file.
-function getStore(type) {
-  if (!_store) {
-    let initStore = new JSONFile({
+// Loads the data from the JSON file into memory.
+function initialize() {
+  if (!_initializePromise) {
+    _store = new JSONFile({
       path: STORE_PATH,
       dataPostProcessor,
     });
-    initStore.ensureDataReady();
-    _store = initStore;
+    _initializePromise = _store.load();
+  }
+  return _initializePromise;
+}
+
+// Test-only method to force reloading of the JSON file.
+async function reloadFile() {
+  await _store.finalize();
+  _initializePromise = null;
+  return initialize();
+}
+
+// Checks that the store is ready and that the requested type exists.
+function ensureType(type) {
+  if (!_store.dataReady) {
+    throw new Error(
+      "The ExtensionSettingsStore was accessed before the initialize promise resolved.");
   }
 
   // Ensure a property exists for the given type.
   if (!_store.data[type]) {
     _store.data[type] = {};
   }
-
-  return _store;
 }
 
 // Return an object with properties for key and value|initialValue, or null
 // if no setting has been stored for that key.
 function getTopItem(type, key) {
-  let store = getStore(type);
+  ensureType(type);
 
-  let keyInfo = store.data[type][key];
+  let keyInfo = _store.data[type][key];
   if (!keyInfo) {
     return null;
   }
 
   // Find the highest precedence, enabled setting.
   for (let item of keyInfo.precedenceList) {
     if (item.enabled) {
       return {key, value: item.value};
@@ -150,19 +156,19 @@ function precedenceComparator(a, b) {
  *
  * @returns {object | null}
  *          Either an object with properties for key and value, which
  *          corresponds to the current top precedent setting, or null if
  *          the current top precedent setting has not changed.
  */
 function alterSetting(extension, type, key, action) {
   let returnItem;
-  let store = getStore(type);
+  ensureType(type);
 
-  let keyInfo = store.data[type][key];
+  let keyInfo = _store.data[type][key];
   if (!keyInfo) {
     if (action === "remove") {
       return null;
     }
     throw new Error(
       `Cannot alter the setting for ${type}:${key} as it does not exist.`);
   }
 
@@ -197,26 +203,38 @@ function alterSetting(extension, type, k
       throw new Error(`${action} is not a valid action for alterSetting.`);
   }
 
   if (foundIndex === 0) {
     returnItem = getTopItem(type, key);
   }
 
   if (action === "remove" && keyInfo.precedenceList.length === 0) {
-    delete store.data[type][key];
+    delete _store.data[type][key];
   }
 
-  store.saveSoon();
+  _store.saveSoon();
 
   return returnItem;
 }
 
 this.ExtensionSettingsStore = {
   /**
+   * Loads the JSON file for the SettingsStore into memory.
+   * The promise this returns must be resolved before asking the SettingsStore
+   * to perform any other operations.
+   *
+   * @returns {Promise}
+   *          A promise that resolves when the Store is ready to be accessed.
+   */
+  initialize() {
+    return initialize();
+  },
+
+  /**
    * Adds a setting to the store, possibly returning the current top precedent
    * setting.
    *
    * @param {Extension} extension
    *        The extension for which a setting is being added.
    * @param {string} type
    *        The type of setting to be stored.
    * @param {string} key
@@ -238,43 +256,43 @@ this.ExtensionSettingsStore = {
    *                          at the top of the precedence list.
    */
   async addSetting(extension, type, key, value, initialValueCallback, callbackArgument = key) {
     if (typeof initialValueCallback != "function") {
       throw new Error("initialValueCallback must be a function.");
     }
 
     let id = extension.id;
-    let store = getStore(type);
+    ensureType(type);
 
-    if (!store.data[type][key]) {
+    if (!_store.data[type][key]) {
       // The setting for this key does not exist. Set the initial value.
       let initialValue = await initialValueCallback(callbackArgument);
-      store.data[type][key] = {
+      _store.data[type][key] = {
         initialValue,
         precedenceList: [],
       };
     }
-    let keyInfo = store.data[type][key];
+    let keyInfo = _store.data[type][key];
     // Check for this item in the precedenceList.
     let foundIndex = keyInfo.precedenceList.findIndex(item => item.id == id);
     if (foundIndex === -1) {
       // No item for this extension, so add a new one.
       let addon = await AddonManager.getAddonByID(id);
       keyInfo.precedenceList.push(
         {id, installDate: addon.installDate.valueOf(), value, enabled: true});
     } else {
       // Item already exists or this extension, so update it.
       keyInfo.precedenceList[foundIndex].value = value;
     }
 
     // Sort the list.
     keyInfo.precedenceList.sort(precedenceComparator);
 
-    store.saveSoon();
+    _store.saveSoon();
 
     // Check whether this is currently the top item.
     if (keyInfo.precedenceList[0].id == id) {
       return {key, value};
     }
     return null;
   },
 
@@ -342,19 +360,19 @@ this.ExtensionSettingsStore = {
    * Retrieves all settings from the store for a given extension.
    *
    * @param {Extension} extension The extension for which a settings are being retrieved.
    * @param {string} type The type of setting to be returned.
    *
    * @returns {array} A list of settings which have been stored for the extension.
    */
   getAllForExtension(extension, type) {
-    let store = getStore(type);
+    ensureType(type);
 
-    let keysObj = store.data[type];
+    let keysObj = _store.data[type];
     let items = [];
     for (let key in keysObj) {
       if (keysObj[key].precedenceList.find(item => item.id == extension.id)) {
         items.push(key);
       }
     }
     return items;
   },
@@ -405,19 +423,19 @@ this.ExtensionSettingsStore = {
    * @param {string} key
    *        A string that uniquely identifies the setting, for example, a
    *        preference name.
    *
    * @returns {string}
    *          The level of control of the extension over the key.
    */
   async getLevelOfControl(extension, type, key) {
-    let store = getStore(type);
+    ensureType(type);
 
-    let keyInfo = store.data[type][key];
+    let keyInfo = _store.data[type][key];
     if (!keyInfo || !keyInfo.precedenceList.length) {
       return "controllable_by_this_extension";
     }
 
     let id = extension.id;
     let enabledItems = keyInfo.precedenceList.filter(item => item.enabled);
     if (!enabledItems.length) {
       return "controllable_by_this_extension";
@@ -439,11 +457,11 @@ this.ExtensionSettingsStore = {
    *
    * Note that this method simply clears the local variable that stores the
    * file, so the next time the file is accessed it will be reloaded.
    *
    * @returns {Promise}
    *          A promise that resolves once the settings store has been cleared.
    */
   _reloadFile() {
-    return clearFileFromCache();
+    return reloadFile();
   },
 };
--- a/toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js
@@ -66,16 +66,25 @@ add_task(async function test_settings_st
   }
 
   // Create an array actual Extension objects which correspond to the
   // test framework extension wrappers.
   let extensions = testExtensions.map(extension => extension.extension);
 
   let expectedCallbackCount = 0;
 
+  await Assert.rejects(
+  ExtensionSettingsStore.getLevelOfControl(
+    1, TEST_TYPE, "key"),
+  /The ExtensionSettingsStore was accessed before the initialize promise resolved/,
+  "Accessing the SettingsStore before it is initialized throws an error.");
+
+  // Initialize the SettingsStore.
+  await ExtensionSettingsStore.initialize();
+
   // Add a setting for the second oldest extension, where it is the only setting for a key.
   for (let key of KEY_LIST) {
     let extensionIndex = 1;
     let itemToAdd = ITEMS[key][extensionIndex];
     let levelOfControl = await ExtensionSettingsStore.getLevelOfControl(extensions[extensionIndex], TEST_TYPE, key);
     equal(
       levelOfControl,
       "controllable_by_this_extension",
@@ -414,14 +423,16 @@ add_task(async function test_settings_st
   for (let extension of testExtensions) {
     await extension.unload();
   }
 
   await promiseShutdownManager();
 });
 
 add_task(async function test_exceptions() {
+  await ExtensionSettingsStore.initialize();
+
   await Assert.rejects(
     ExtensionSettingsStore.addSetting(
       1, TEST_TYPE, "key_not_a_function", "val1", "not a function"),
     /initialValueCallback must be a function/,
     "addSetting rejects with a callback that is not a function.");
 });