Bug 1406181 - Run more storage.local.set code in the child process when IDB backend is enabled. draft
authorLuca Greco <lgreco@mozilla.com>
Fri, 22 Dec 2017 17:52:23 +0100
changeset 714409 d62228330009a75a98349900c9b10524ed8a1b9d
parent 714402 855986dc90db7d22545f607d0233d1b975868488
child 714410 41549723b3f399b8986a9f2200ccf7bb225b18cc
push id93914
push userluca.greco@alcacoop.it
push dateFri, 22 Dec 2017 19:06:36 +0000
bugs1406181
milestone59.0a1
Bug 1406181 - Run more storage.local.set code in the child process when IDB backend is enabled. MozReview-Commit-ID: 5tAkQRvv4j2
toolkit/components/extensions/ExtensionStorage.jsm
toolkit/components/extensions/ExtensionStorageIDB.jsm
toolkit/components/extensions/ext-c-storage.js
toolkit/components/extensions/ext-storage.js
--- a/toolkit/components/extensions/ExtensionStorage.jsm
+++ b/toolkit/components/extensions/ExtensionStorage.jsm
@@ -346,16 +346,19 @@ this.ExtensionStorage = {
     } else if (topic == "extension-invalidate-storage-cache") {
       for (let promise of this.jsonFilePromises.values()) {
         promise.then(jsonFile => { jsonFile.finalize(); });
       }
       this.jsonFilePromises.clear();
     }
   },
 
+  // Serializes an arbitrary value into a StructuredCloneHolder, if appropriate.
+  serialize,
+
   /**
    * Serializes the given storage items for transporting between processes.
    *
    * @param {BaseContext} context
    *        The context to use for the created StructuredCloneHolder
    *        objects.
    * @param {Array<string>|object} items
    *        The items to serialize. If an object is provided, its
--- a/toolkit/components/extensions/ExtensionStorageIDB.jsm
+++ b/toolkit/components/extensions/ExtensionStorageIDB.jsm
@@ -79,52 +79,83 @@ this.ExtensionStorageIDB = {
    *        The storage items to set. For each property in the object,
    *        the storage value for that property is set to its value in
    *        said object. Any values which are StructuredCloneHolder
    *        instances are deserialized before being stored.
    * @param {object}  options
    * @param {boolean} options.skipNotify
    *        Set to true to skip the onChanged event notification
    *        (and use a lower amount of memory).
-   * @returns {Promise<void>}
+   * @param {boolean} options.returnChanges
+   *        Set to true to return the computed "changes" object as the
+   *        return value (used to be able to run most of the storage.local.set
+   *        code in the child process and only sent the serialized changes
+   *        object across the processes to be notified to the storage.onChanged
+   *        listeners).
+   * @param {function} options.serialize
+   *        Set to a function which will be used to serialize the values into
+   *        a StructuredCloneHolder object (if appropriate) and being sent
+   *        across the processes (it is also used to detect data cloning errors
+   *        and raise an appropriate error to the caller).
+   *
+   * @returns {Promise<void|object>}
+   *        Return a promise which resolved to the computed "changes" object
+   *        when "returnChages" is true, otherwise it just resolves to undefined.
    */
-  async set(extensionId, items, {skipNotify} = {}) {
+  async set(extensionId, items, {skipNotify, returnChanges, serialize} = {}) {
     const db = await this._open(extensionId);
     let data;
 
-    if (!skipNotify) {
+    // There no need to load the data from the storage if we don't need to
+    // compute the changes object.
+    if (!skipNotify || returnChanges) {
       data = await this._loadData(db, Object.keys(items));
     }
 
     const changes = {};
     const savedPromises = [];
 
     const store = db.objectStore(IDB_DATA_STORENAME, "readwrite");
 
     for (let key in items) {
-      let value = items[key];
+      // By serializing the value here we also ensure that if the value cannot be cloned
+      // (e.g. because of a DataCloneError / cyclic object issues), the expected
+      // exception is going to be raised to the caller, and it will be already
+      // serialized to be sent across the processing in a message manager message.
+      let value = serialize ? serialize(items[key]) : items[key];
 
-      if (!skipNotify) {
+      // We only need to create the changes object if we are going to notify
+      // it to the listeners or if we are going to return it to the caller.
+      if (!skipNotify || returnChanges) {
         changes[key] = {
-          oldValue: data[key],
           newValue: value,
+          oldValue: data && serialize ? serialize(data[key]) : data[key],
         };
       }
 
-      savedPromises.push(store.put(value, key));
+      // Using the value deserialized from the StructuredCloneHolder will make
+      // the IndexedDB put operation faster (because it will not spent that much time
+      // on the Xrays wrappers).
+      const valueToStore = value && value.deserialize ? value.deserialize(this) : value;
+
+      savedPromises.push(store.put(valueToStore, key));
     }
 
     await Promise.all(savedPromises);
 
     await db.close();
 
     if (!skipNotify) {
       this.notifyListeners(extensionId, changes);
     }
 
+    if (returnChanges) {
+      return changes;
+    }
+
     return null;
   },
 
   /**
    * Asynchronously retrieves the values for the given storage items for
    * the given extension ID.
    *
    * @param {string} extensionId
--- a/toolkit/components/extensions/ext-c-storage.js
+++ b/toolkit/components/extensions/ext-c-storage.js
@@ -53,42 +53,41 @@ this.storage = class extends ExtensionAP
         TelemetryStopwatch.start(storageGetHistogram, stopwatchKey);
         try {
           let result = await ExtensionStorageIDB.get(context.extension.id, keys);
 
           TelemetryStopwatch.finish(storageGetHistogram, stopwatchKey);
           return result;
         } catch (e) {
           TelemetryStopwatch.cancel(storageGetHistogram, stopwatchKey);
-          throw e;
+          throw new ExtensionUtils.ExtensionError(String(e));
         }
       },
       set: async function(items) {
         const stopwatchKey = {};
         TelemetryStopwatch.start(storageSetHistogram, stopwatchKey);
         try {
-          const hasListeners = await hasParentListeners();
+          const changes = await ExtensionStorageIDB.set(context.extension.id, items, {
+            skipNotify: true,
+            returnChanges: true,
+            serialize: ExtensionStorage.serialize,
+          });
 
-          let result;
+          const hasListeners = await hasParentListeners();
 
           if (hasListeners) {
-            result = await context.childManager.callParentAsyncFunction("storage.local.set", [
-              serialize(items),
+            await context.childManager.callParentAsyncFunction("storage.fireStorageLocalOnChanged", [
+              changes,
             ]);
-          } else {
-            result = await ExtensionStorageIDB.set(context.extension.id, items, {
-              skipNotify: true,
-            });
           }
 
           TelemetryStopwatch.finish(storageSetHistogram, stopwatchKey);
-          return result;
         } catch (e) {
           TelemetryStopwatch.cancel(storageSetHistogram, stopwatchKey);
-          throw e;
+          throw new ExtensionUtils.ExtensionError(String(e));
         }
       },
       remove: async function(keys) {
         // TODO: we should also collect telemetry on storage.local.remove.
         const hasListeners = await hasParentListeners();
 
         let result;
 
--- a/toolkit/components/extensions/ext-storage.js
+++ b/toolkit/components/extensions/ext-storage.js
@@ -159,16 +159,25 @@ this.storage = class extends ExtensionAP
             let data = await lookup;
             if (!data) {
               return Promise.reject({message: "Managed storage manifest not found"});
             }
             return ExtensionStorage._filterProperties(data, keys);
           },
         },
 
+        fireStorageLocalOnChanged(changes) {
+          // This is only used by the storage.local IDB backend which can
+          // run most of the storage.local.set code in the child process and
+          // then ask the main process to just route the computed "changes"
+          // object to the listeners subscribed in the other processes
+          // (e.g. a storage.onChanged listener subscribed in the content child process).
+          ExtensionStorageIDB.notifyListeners(context.extension.id, changes);
+        },
+
         onChanged: new EventManager(context, "storage.onChanged", fire => {
           let listenerLocal = changes => {
             fire.raw(changes, "local");
           };
           let listenerSync = changes => {
             fire.async(changes, "sync");
           };