Bug 1381605 - Do not load ExtensionSettingsStore's JSON file synchronously, r=aswan
authorBob Silverberg <bsilverberg@mozilla.com>
Mon, 24 Jul 2017 15:49:30 -0400
changeset 420707 5ffc57ebe8e97d09b8d2dc4390a629963805c233
parent 420706 bfd30365fa0bb0aa0ece6c52e574dc861d674f16
child 420708 a2717f579f07821503c56657fc676b4487baa629
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaswan
bugs1381605
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 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.");
 });