Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values. r=aswan
authorKris Maglione <maglione.k@gmail.com>
Mon, 10 Jul 2017 18:24:11 -0700
changeset 368206 317331a50bde2f2e59bcd6074078c1d6ec2bd20c
parent 368205 f6232176ba57ffdd3c06b201e0e7562256936c76
child 368207 aea5888a82edce883da06a66f69521ceac196dc8
push id32159
push usercbook@mozilla.com
push dateTue, 11 Jul 2017 10:52:11 +0000
treeherdermozilla-central@b07db5d650b7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaswan
bugs1370752
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 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values. r=aswan This gives us performance wins in sevaral areas: - Creating a structured clone blob of storage data directly from the source compartment allows us to avoid X-ray and JSON serialization overhead when storing new values. - Storing the intermediate StructuredCloneBlob, rather than JSON values, in-memory saves us additional JSON and structured clone overhead when passing the values to listeners and API callers, and saves us a fair amount of memory to boot. - Serializing storage values before sending them over a message manager allows us to deserialize them directly into an extension scope on the other side, saving us a lot of additional structured clone overhead and intermediate garbage generation. - Using JSONFile.jsm for storage lets us consolidate multiple storage file write operations, rather than performing a separate JSON serialization for each individual storage write. - Additionally, this paves the way for us to transition to IndexedDB as a storage backend, with full support for arbitrary structured-clone-compatible data structures. MozReview-Commit-ID: JiRE7EFMYxn
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionChild.jsm
toolkit/components/extensions/ExtensionCommon.jsm
toolkit/components/extensions/ExtensionParent.jsm
toolkit/components/extensions/ExtensionStorage.jsm
toolkit/components/extensions/ext-c-storage.js
toolkit/components/extensions/ext-storage.js
toolkit/components/extensions/test/mochitest/test_ext_storage_content.html
toolkit/components/extensions/test/xpcshell/test_ext_storage.js
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -266,17 +266,19 @@ var UninstallObserver = {
   onUninstalled(addon) {
     let uuid = UUIDMap.get(addon.id, false);
     if (!uuid) {
       return;
     }
 
     if (!this.leaveStorage) {
       // Clear browser.local.storage
-      ExtensionStorage.clear(addon.id);
+      AsyncShutdown.profileChangeTeardown.addBlocker(
+        `Clear Extension Storage ${addon.id}`,
+        ExtensionStorage.clear(addon.id));
 
       // Clear any IndexedDB storage created by the extension
       let baseURI = NetUtil.newURI(`moz-extension://${uuid}/`);
       let principal = Services.scriptSecurityManager.createCodebasePrincipal(
         baseURI, {});
       Services.qms.clearStoragesForPrincipal(principal);
 
       // Clear localStorage created by the extension
--- a/toolkit/components/extensions/ExtensionChild.jsm
+++ b/toolkit/components/extensions/ExtensionChild.jsm
@@ -801,29 +801,34 @@ class ChildAPIManager {
   /**
    * Calls a function in the parent process and returns its result
    * asynchronously.
    *
    * @param {string} path The full name of the method, e.g. "tabs.create".
    * @param {Array} args The parameters for the function.
    * @param {function(*)} [callback] The callback to be called when the function
    *     completes.
+   * @param {object} [options] Extra options.
+   * @param {boolean} [options.noClone = false] If true, do not clone
+   *     the arguments into an extension sandbox before calling the API
+   *     method.
    * @returns {Promise|undefined} Must be void if `callback` is set, and a
    *     promise otherwise. The promise is resolved when the function completes.
    */
-  callParentAsyncFunction(path, args, callback) {
+  callParentAsyncFunction(path, args, callback, options = {}) {
     let callId = getUniqueId();
     let deferred = PromiseUtils.defer();
     this.callPromises.set(callId, deferred);
 
     this.messageManager.sendAsyncMessage("API:Call", {
       childId: this.id,
       callId,
       path,
       args,
+      noClone: options.noClone || false,
     });
 
     return this.context.wrapPromise(deferred.promise, callback);
   }
 
   /**
    * Create a proxy for an event in the parent process. The returned event
    * object shares its internal state with other instances. For instance, if
--- a/toolkit/components/extensions/ExtensionCommon.jsm
+++ b/toolkit/components/extensions/ExtensionCommon.jsm
@@ -1096,17 +1096,17 @@ class SchemaAPIManager extends EventEmit
    * @returns {object} A sandbox that is used as the global by `loadScript`.
    */
   _createExtGlobal() {
     let global = Cu.Sandbox(Services.scriptSecurityManager.getSystemPrincipal(), {
       wantXrays: false,
       sandboxName: `Namespace of ext-*.js scripts for ${this.processType}`,
     });
 
-    Object.assign(global, {global, Cc, Ci, Cu, Cr, XPCOMUtils, ChromeWorker, ExtensionCommon, MatchPattern, MatchPatternSet, extensions: this});
+    Object.assign(global, {global, Cc, Ci, Cu, Cr, XPCOMUtils, ChromeWorker, ExtensionCommon, MatchPattern, MatchPatternSet, StructuredCloneHolder, extensions: this});
 
     Cu.import("resource://gre/modules/AppConstants.jsm", global);
     Cu.import("resource://gre/modules/ExtensionAPI.jsm", global);
 
     XPCOMUtils.defineLazyGetter(global, "console", getConsole);
 
     XPCOMUtils.defineLazyModuleGetter(global, "ExtensionUtils",
                                       "resource://gre/modules/ExtensionUtils.jsm");
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -662,17 +662,17 @@ ParentAPIManager = {
         "API:CallResult",
         Object.assign({
           childId: data.childId,
           callId: data.callId,
         }, result));
     };
 
     try {
-      let args = Cu.cloneInto(data.args, context.sandbox);
+      let args = data.noClone ? data.args : Cu.cloneInto(data.args, context.sandbox);
       let pendingBrowser = context.pendingEventBrowser;
       let fun = await context.apiCan.asyncFindAPIPath(data.path);
       let result = context.withPendingBrowser(pendingBrowser,
                                               () => fun(...args));
       if (data.callId) {
         result = result || Promise.resolve();
 
         result.then(result => {
--- a/toolkit/components/extensions/ExtensionStorage.jsm
+++ b/toolkit/components/extensions/ExtensionStorage.jsm
@@ -8,211 +8,306 @@ this.EXPORTED_SYMBOLS = ["ExtensionStora
 
 const Ci = Components.interfaces;
 const Cc = Components.classes;
 const Cu = Components.utils;
 const Cr = Components.results;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
+
+XPCOMUtils.defineLazyModuleGetter(this, "ExtensionUtils",
+                                  "resource://gre/modules/ExtensionUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "JSONFile",
+                                  "resource://gre/modules/JSONFile.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "OS",
                                   "resource://gre/modules/osfile.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown",
-                                  "resource://gre/modules/AsyncShutdown.jsm");
+
+const global = this;
+
+function isStructuredCloneHolder(value) {
+  return (value && typeof value === "object" &&
+          Cu.getClassName(value, true) === "StructuredCloneHolder");
+}
+
+class SerializeableMap extends Map {
+  toJSON() {
+    let result = {};
+    for (let [key, value] of this) {
+      if (isStructuredCloneHolder(value)) {
+        value = value.deserialize(global);
+        this.set(key, value);
+      }
+
+      result[key] = value;
+    }
+    return result;
+  }
+
+  /**
+   * Like toJSON, but attempts to serialize every value separately, and
+   * elides any which fail to serialize. Should only be used if initial
+   * JSON serialization fails.
+   *
+   * @returns {object}
+   */
+  toJSONSafe() {
+    let result = {};
+    for (let [key, value] of this) {
+      try {
+        void JSON.serialize(value);
+
+        result[key] = value;
+      } catch (e) {
+        Cu.reportError(new Error(`Failed to serialize browser.storage key "${key}": ${e}`));
+      }
+    }
+    return result;
+  }
+}
 
 /**
- * Helper function used to sanitize the objects that have to be saved in the ExtensionStorage.
+ * Serializes an arbitrary value into a StructuredCloneHolder, if
+ * appropriate. Existing StructuredCloneHolders are returned unchanged.
+ * Non-object values are also returned unchanged. Anything else is
+ * serialized, and a new StructuredCloneHolder returned.
  *
- * @param {BaseContext} context
- *   The current extension context.
- * @param {string} key
- *   The key of the current JSON property.
- * @param {any} value
- *   The value of the current JSON property.
+ * This allows us to avoid a second structured clone operation after
+ * sending a storage value across a message manager, before cloning it
+ * into an extension scope.
  *
- * @returns {any}
- *   The sanitized value of the property.
+ * @param {StructuredCloneHolder|*} value
+ *        A value to serialize.
+ * @returns {*}
  */
-function jsonReplacer(context, key, value) {
-  switch (typeof(value)) {
-    // Serialize primitive types as-is.
-    case "string":
-    case "number":
-    case "boolean":
-      return value;
-
-    case "object":
-      if (value === null) {
-        return value;
-      }
-
-      switch (Cu.getClassName(value, true)) {
-        // Serialize arrays and ordinary objects as-is.
-        case "Array":
-        case "Object":
-          return value;
-
-        // Serialize Date objects and regular expressions as their
-        // string representations.
-        case "Date":
-        case "RegExp":
-          return String(value);
-      }
-      break;
+function serialize(value) {
+  if (value && typeof value === "object" && !isStructuredCloneHolder(value)) {
+    return new StructuredCloneHolder(value);
   }
-
-  if (!key) {
-    // If this is the root object, and we can't serialize it, serialize
-    // the value to an empty object.
-    return new context.cloneScope.Object();
-  }
-
-  // Everything else, omit entirely.
-  return undefined;
+  return value;
 }
 
 this.ExtensionStorage = {
-  cache: new Map(),
+  // Map<extension-id, Promise<JSONFile>>
+  jsonFilePromises: new Map(),
+
   listeners: new Map(),
 
   /**
+   * Asynchronously reads the storage file for the given extension ID
+   * and returns a Promise for its initialized JSONFile object.
+   *
+   * @param {string} extensionId
+   *        The ID of the extension for which to return a file.
+   * @returns {Promise<JSONFile>}
+   */
+  async _readFile(extensionId) {
+    OS.File.makeDir(this.getExtensionDir(extensionId), {
+      ignoreExisting: true,
+      from: OS.Constants.Path.profileDir,
+    });
+
+    let jsonFile = new JSONFile({path: this.getStorageFile(extensionId)});
+    await jsonFile.load();
+
+    jsonFile.data = new SerializeableMap(Object.entries(jsonFile.data));
+
+    return jsonFile;
+  },
+
+  /**
+   * Returns a Promise for initialized JSONFile instance for the
+   * extension's storage file.
+   *
+   * @param {string} extensionId
+   *        The ID of the extension for which to return a file.
+   * @returns {Promise<JSONFile>}
+   */
+  getFile(extensionId) {
+    let promise = this.jsonFilePromises.get(extensionId);
+    if (!promise) {
+      promise = this._readFile(extensionId);
+      this.jsonFilePromises.set(extensionId, promise);
+    }
+    return promise;
+  },
+
+  /**
    * Sanitizes the given value, and returns a JSON-compatible
    * representation of it, based on the privileges of the given global.
    *
    * @param {value} value
    *        The value to sanitize.
    * @param {Context} context
    *        The extension context in which to sanitize the value
    * @returns {value}
    *        The sanitized value.
    */
   sanitize(value, context) {
-    let json = context.jsonStringify(value, jsonReplacer.bind(null, context));
+    let json = context.jsonStringify(value === undefined ? null : value);
+    if (json == undefined) {
+      throw new ExtensionUtils.ExtensionError("DataCloneError: The object could not be cloned.");
+    }
     return JSON.parse(json);
   },
 
+
+  /**
+   * Returns the path to the storage directory within the profile for
+   * the given extension ID.
+   *
+   * @param {string} extensionId
+   *        The ID of the extension for which to return a directory path.
+   * @returns {string}
+   */
   getExtensionDir(extensionId) {
     return OS.Path.join(this.extensionDir, extensionId);
   },
 
+  /**
+   * Returns the path to the JSON storage file for the given extension
+   * ID.
+   *
+   * @param {string} extensionId
+   *        The ID of the extension for which to return a file path.
+   * @returns {string}
+   */
   getStorageFile(extensionId) {
     return OS.Path.join(this.extensionDir, extensionId, "storage.js");
   },
 
-  read(extensionId) {
-    if (this.cache.has(extensionId)) {
-      return this.cache.get(extensionId);
+  /**
+   * Asynchronously sets the values of the given storage items for the
+   * given extension.
+   *
+   * @param {string} extensionId
+   *        The ID of the extension for which to set storage values.
+   * @param {object} items
+   *        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.
+   * @returns {Promise<void>}
+   */
+  async set(extensionId, items) {
+    let jsonFile = await this.getFile(extensionId);
+
+    let changes = {};
+    for (let prop in items) {
+      let item = items[prop];
+      changes[prop] = {oldValue: serialize(jsonFile.data.get(prop)), newValue: serialize(item)};
+      jsonFile.data.set(prop, item);
     }
 
-    let path = this.getStorageFile(extensionId);
-    let decoder = new TextDecoder();
-    let promise = OS.File.read(path);
-    promise = promise.then(array => {
-      return JSON.parse(decoder.decode(array));
-    }).catch((error) => {
-      if (!error.becauseNoSuchFile) {
-        Cu.reportError("Unable to parse JSON data for extension storage.");
-      }
-      return {};
-    });
-    this.cache.set(extensionId, promise);
-    return promise;
+    this.notifyListeners(extensionId, changes);
+
+    jsonFile.saveSoon();
+    return null;
   },
 
-  write(extensionId) {
-    let promise = this.read(extensionId).then(extData => {
-      let encoder = new TextEncoder();
-      let array = encoder.encode(JSON.stringify(extData));
-      let path = this.getStorageFile(extensionId);
-      OS.File.makeDir(this.getExtensionDir(extensionId), {
-        ignoreExisting: true,
-        from: OS.Constants.Path.profileDir,
-      });
-      let promise = OS.File.writeAtomic(path, array);
-      return promise;
-    }).catch(() => {
-      // Make sure this promise is never rejected.
-      Cu.reportError("Unable to write JSON data for extension storage.");
-    });
+  /**
+   * Asynchronously removes the given storage items for the given
+   * extension ID.
+   *
+   * @param {string} extensionId
+   *        The ID of the extension for which to remove storage values.
+   * @param {Array<string>} items
+   *        A list of storage items to remove.
+   * @returns {Promise<void>}
+   */
+  async remove(extensionId, items) {
+    let jsonFile = await this.getFile(extensionId);
 
-    AsyncShutdown.profileBeforeChange.addBlocker(
-      "ExtensionStorage: Finish writing extension data",
-      promise);
+    let changed = false;
+    let changes = {};
 
-    return promise.then(() => {
-      AsyncShutdown.profileBeforeChange.removeBlocker(promise);
-    });
+    for (let prop of [].concat(items)) {
+      if (jsonFile.data.has(prop)) {
+        changes[prop] = {oldValue: serialize(jsonFile.data.get(prop))};
+        jsonFile.data.delete(prop);
+        changed = true;
+      }
+    }
+
+    if (changed) {
+      this.notifyListeners(extensionId, changes);
+      jsonFile.saveSoon();
+    }
+    return null;
   },
 
-  set(extensionId, items) {
-    return this.read(extensionId).then(extData => {
-      let changes = {};
-      for (let prop in items) {
-        let item = items[prop];
-        changes[prop] = {oldValue: extData[prop], newValue: item};
-        extData[prop] = item;
-      }
-
-      this.notifyListeners(extensionId, changes);
+  /**
+   * Asynchronously clears all storage entries for the given extension
+   * ID.
+   *
+   * @param {string} extensionId
+   *        The ID of the extension for which to clear storage.
+   * @returns {Promise<void>}
+   */
+  async clear(extensionId) {
+    let jsonFile = await this.getFile(extensionId);
 
-      return this.write(extensionId);
-    });
-  },
+    let changed = false;
+    let changes = {};
 
-  remove(extensionId, items) {
-    return this.read(extensionId).then(extData => {
-      let changes = {};
-      for (let prop of [].concat(items)) {
-        changes[prop] = {oldValue: extData[prop]};
-        delete extData[prop];
-      }
+    for (let [prop, oldValue] of jsonFile.data.entries()) {
+      changes[prop] = {oldValue: serialize(oldValue)};
+      jsonFile.data.delete(prop);
+      changed = true;
+    }
 
+    if (changed) {
       this.notifyListeners(extensionId, changes);
-
-      return this.write(extensionId);
-    });
+      jsonFile.saveSoon();
+    }
+    return null;
   },
 
-  clear(extensionId) {
-    return this.read(extensionId).then(extData => {
-      let changes = {};
-      for (let prop of Object.keys(extData)) {
-        changes[prop] = {oldValue: extData[prop]};
-        delete extData[prop];
-      }
-
-      this.notifyListeners(extensionId, changes);
-
-      return this.write(extensionId);
-    });
-  },
+  /**
+   * Asynchronously retrieves the values for the given storage items for
+   * the given extension ID.
+   *
+   * @param {string} extensionId
+   *        The ID of the extension for which to get storage values.
+   * @param {Array<string>|object|null} [keys]
+   *        The storage items to get. If an array, the value of each key
+   *        in the array is returned. If null, the values of all items
+   *        are returned. If an object, the value for each key in the
+   *        object is returned, or that key's value if the item is not
+   *        set.
+   * @returns {Promise<object>}
+   *        An object which a property for each requested key,
+   *        containing that key's storage value. Values are
+   *        StructuredCloneHolder objects which can be deserialized to
+   *        the original storage value.
+   */
+  async get(extensionId, keys) {
+    let jsonFile = await this.getFile(extensionId);
+    let {data} = jsonFile;
 
-  get(extensionId, keys) {
-    return this.read(extensionId).then(extData => {
-      let result = {};
-      if (keys === null) {
-        Object.assign(result, extData);
-      } else if (typeof(keys) == "object" && !Array.isArray(keys)) {
-        for (let prop in keys) {
-          if (prop in extData) {
-            result[prop] = extData[prop];
-          } else {
-            result[prop] = keys[prop];
-          }
-        }
-      } else {
-        for (let prop of [].concat(keys)) {
-          if (prop in extData) {
-            result[prop] = extData[prop];
-          }
+    let result = {};
+    if (keys === null) {
+      Object.assign(result, data.toJSON());
+    } else if (typeof(keys) == "object" && !Array.isArray(keys)) {
+      for (let prop in keys) {
+        if (data.has(prop)) {
+          result[prop] = serialize(data.get(prop));
+        } else {
+          result[prop] = keys[prop];
         }
       }
+    } else {
+      for (let prop of [].concat(keys)) {
+        if (data.has(prop)) {
+          result[prop] = serialize(data.get(prop));
+        }
+      }
+    }
 
-      return result;
-    });
+    return result;
   },
 
   addOnChangedListener(extensionId, listener) {
     let listeners = this.listeners.get(extensionId) || new Set();
     listeners.add(listener);
     this.listeners.set(extensionId, listeners);
   },
 
@@ -238,17 +333,20 @@ this.ExtensionStorage = {
     Services.obs.addObserver(this, "xpcom-shutdown");
   },
 
   observe(subject, topic, data) {
     if (topic == "xpcom-shutdown") {
       Services.obs.removeObserver(this, "extension-invalidate-storage-cache");
       Services.obs.removeObserver(this, "xpcom-shutdown");
     } else if (topic == "extension-invalidate-storage-cache") {
-      this.cache.clear();
+      for (let promise of this.jsonFilePromises.values()) {
+        promise.then(jsonFile => { jsonFile.finalize(); });
+      }
+      this.jsonFilePromises.clear();
     }
   },
 };
 
 XPCOMUtils.defineLazyGetter(ExtensionStorage, "extensionDir",
   () => OS.Path.join(OS.Constants.Path.profileDir, "browser-extension-data"));
 
 ExtensionStorage.init();
--- a/toolkit/components/extensions/ext-c-storage.js
+++ b/toolkit/components/extensions/ext-c-storage.js
@@ -1,21 +1,76 @@
 "use strict";
 
+/* import-globals-from ext-c-toolkit.js */
+
 XPCOMUtils.defineLazyModuleGetter(this, "ExtensionStorage",
                                   "resource://gre/modules/ExtensionStorage.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryStopwatch",
                                   "resource://gre/modules/TelemetryStopwatch.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 
+var {
+  ExtensionError,
+} = ExtensionUtils;
+
 const storageGetHistogram = "WEBEXT_STORAGE_LOCAL_GET_MS";
 const storageSetHistogram = "WEBEXT_STORAGE_LOCAL_SET_MS";
 
 this.storage = class extends ExtensionAPI {
   getAPI(context) {
+    /**
+     * Serializes the given storage items for transporting to the parent
+     * process.
+     *
+     * @param {Array<string>|object} items
+     *        The items to serialize. If an object is provided, its
+     *        values are serialized to StructuredCloneHolder objects.
+     *        Otherwise, it is returned as-is.
+     * @returns {Array<string>|object}
+     */
+    function serialize(items) {
+      if (items && typeof items === "object" && !Array.isArray(items)) {
+        let result = {};
+        for (let [key, value] of Object.entries(items)) {
+          try {
+            result[key] = new StructuredCloneHolder(value, context.cloneScope);
+          } catch (e) {
+            throw new ExtensionError(String(e));
+          }
+        }
+        return result;
+      }
+      return items;
+    }
+
+    /**
+     * Deserializes the given storage items from the parent process into
+     * the extension context.
+     *
+     * @param {object} items
+     *        The items to deserialize. Any property of the object which
+     *        is a StructuredCloneHolder instance is deserialized into
+     *        the extension scope. Any other object is cloned into the
+     *        extension scope directly.
+     * @returns {object}
+     */
+    function deserialize(items) {
+      let result = new context.cloneScope.Object();
+      for (let [key, value] of Object.entries(items)) {
+        if (value && typeof value === "object" && Cu.getClassName(value, true) === "StructuredCloneHolder") {
+          value = value.deserialize(context.cloneScope);
+        } else {
+          value = Cu.cloneInto(value, context.cloneScope);
+        }
+        result[key] = value;
+      }
+      return result;
+    }
+
     function sanitize(items) {
       // The schema validator already takes care of arrays (which are only allowed
       // to contain strings). Strings and null are safe values.
       if (typeof items != "object" || items === null || Array.isArray(items)) {
         return items;
       }
       // If we got here, then `items` is an object generated by `ObjectType`'s
       // `normalize` method from Schemas.jsm. The object returned by `normalize`
@@ -25,42 +80,41 @@ this.storage = class extends ExtensionAP
       // it is not allowed to access properties of `items`.
       // So we enumerate all properties and sanitize each value individually.
       let sanitized = {};
       for (let [key, value] of Object.entries(items)) {
         sanitized[key] = ExtensionStorage.sanitize(value, context);
       }
       return sanitized;
     }
+
     return {
       storage: {
         local: {
           get: async function(keys) {
             const stopwatchKey = {};
             TelemetryStopwatch.start(storageGetHistogram, stopwatchKey);
             try {
-              keys = sanitize(keys);
               let result = await context.childManager.callParentAsyncFunction("storage.local.get", [
-                keys,
-              ]);
+                serialize(keys),
+              ], null, {noClone: true}).then(deserialize);
               TelemetryStopwatch.finish(storageGetHistogram, stopwatchKey);
               return result;
             } catch (e) {
               TelemetryStopwatch.cancel(storageGetHistogram, stopwatchKey);
               throw e;
             }
           },
           set: async function(items) {
             const stopwatchKey = {};
             TelemetryStopwatch.start(storageSetHistogram, stopwatchKey);
             try {
-              items = sanitize(items);
               let result = await context.childManager.callParentAsyncFunction("storage.local.set", [
-                items,
-              ]);
+                serialize(items),
+              ], null, {noClone: true});
               TelemetryStopwatch.finish(storageSetHistogram, stopwatchKey);
               return result;
             } catch (e) {
               TelemetryStopwatch.cancel(storageSetHistogram, stopwatchKey);
               throw e;
             }
           },
         },
@@ -74,12 +128,28 @@ this.storage = class extends ExtensionAP
           },
           set: function(items) {
             items = sanitize(items);
             return context.childManager.callParentAsyncFunction("storage.sync.set", [
               items,
             ]);
           },
         },
+
+        onChanged: new EventManager(context, "storage.onChanged", fire => {
+          let onChanged = (data, area) => {
+            let changes = new context.cloneScope.Object();
+            for (let [key, value] of Object.entries(data)) {
+              changes[key] = deserialize(value);
+            }
+            fire.raw(changes, area);
+          };
+
+          let parent = context.childManager.getParentEvent("storage.onChanged");
+          parent.addListener(onChanged);
+          return () => {
+            parent.removeListener(onChanged);
+          };
+        }).api(),
       },
     };
   }
 };
--- a/toolkit/components/extensions/ext-storage.js
+++ b/toolkit/components/extensions/ext-storage.js
@@ -60,17 +60,17 @@ this.storage = class extends ExtensionAP
           clear: function() {
             enforceNoTemporaryAddon(extension.id);
             return extensionStorageSync.clear(extension, context);
           },
         },
 
         onChanged: new EventManager(context, "storage.onChanged", fire => {
           let listenerLocal = changes => {
-            fire.async(changes, "local");
+            fire.raw(changes, "local");
           };
           let listenerSync = changes => {
             fire.async(changes, "sync");
           };
 
           ExtensionStorage.addOnChangedListener(extension.id, listenerLocal);
           extensionStorageSync.addOnChangedListener(extension, listenerSync, context);
           return () => {
--- a/toolkit/components/extensions/test/mochitest/test_ext_storage_content.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_storage_content.html
@@ -144,60 +144,71 @@ async function contentScript(checkGet) {
 
       // Make sure we can store complex JSON data.
       // known previous values
       await storage.set({"test-prop1": "value1", "test-prop2": "value2"});
 
       // Make sure the set() handler landed.
       await globalChanges;
 
+      let date = new Date(0);
+
       clearGlobalChanges();
       await storage.set({
         "test-prop1": {
           str: "hello",
           bool: true,
           null: null,
           undef: undefined,
           obj: {},
           arr: [1, 2],
           date: new Date(0),
           regexp: /regexp/,
-          func: function func() {},
-          window,
         },
       });
 
-      await storage.set({"test-prop2": function func() {}});
+      await browser.test.assertRejects(
+        storage.set({
+          window,
+        }),
+        /DataCloneError|cyclic object value/);
+
+      await browser.test.assertRejects(
+        storage.set({"test-prop2": function func() {}}),
+        /DataCloneError/);
+
       const recentChanges = await globalChanges;
 
       browser.test.assertEq("value1", recentChanges["test-prop1"].oldValue, "oldValue correct");
       browser.test.assertEq("object", typeof(recentChanges["test-prop1"].newValue), "newValue is obj");
       clearGlobalChanges();
 
       data = await storage.get({"test-prop1": undefined, "test-prop2": undefined});
       let obj = data["test-prop1"];
 
+      if (areaName === "local") {
+        browser.test.assertEq(String(date), String(obj.date), "date part correct");
+        browser.test.assertEq("/regexp/", obj.regexp.toSource(), "regexp part correct");
+      } else {
+        browser.test.assertEq("1970-01-01T00:00:00.000Z", String(obj.date), "date part correct");
+
+        browser.test.assertEq("object", typeof obj.regexp, "regexp part is an object");
+        browser.test.assertEq(0, Object.keys(obj.regexp).length, "regexp part is an empty object");
+      }
+
       browser.test.assertEq("hello", obj.str, "string part correct");
       browser.test.assertEq(true, obj.bool, "bool part correct");
       browser.test.assertEq(null, obj.null, "null part correct");
       browser.test.assertEq(undefined, obj.undef, "undefined part correct");
-      browser.test.assertEq(undefined, obj.func, "function part correct");
       browser.test.assertEq(undefined, obj.window, "window part correct");
-      browser.test.assertEq("1970-01-01T00:00:00.000Z", obj.date, "date part correct");
-      browser.test.assertEq("/regexp/", obj.regexp, "regexp part correct");
       browser.test.assertEq("object", typeof(obj.obj), "object part correct");
       browser.test.assertTrue(Array.isArray(obj.arr), "array part present");
       browser.test.assertEq(1, obj.arr[0], "arr[0] part correct");
       browser.test.assertEq(2, obj.arr[1], "arr[1] part correct");
       browser.test.assertEq(2, obj.arr.length, "arr.length part correct");
-
-      obj = data["test-prop2"];
-
-      browser.test.assertEq("[object Object]", {}.toString.call(obj), "function serialized as a plain object");
-      browser.test.assertEq(0, Object.keys(obj).length, "function serialized as an empty object");
     } catch (e) {
       browser.test.fail(`Error: ${e} :: ${e.stack}`);
       browser.test.notifyFail("storage");
     }
   }
 
   browser.test.onMessage.addListener(msg => {
     let promise;
--- a/toolkit/components/extensions/test/xpcshell/test_ext_storage.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_storage.js
@@ -248,60 +248,71 @@ add_task(async function test_backgroundS
 
         // Make sure we can store complex JSON data.
         // known previous values
         await storage.set({"test-prop1": "value1", "test-prop2": "value2"});
 
         // Make sure the set() handler landed.
         await globalChanges;
 
+        let date = new Date(0);
+
         clearGlobalChanges();
         await storage.set({
           "test-prop1": {
             str: "hello",
             bool: true,
             null: null,
             undef: undefined,
             obj: {},
             arr: [1, 2],
-            date: new Date(0),
+            date,
             regexp: /regexp/,
-            func: function func() {},
-            window,
           },
         });
 
-        await storage.set({"test-prop2": function func() {}});
+        await browser.test.assertRejects(
+          storage.set({
+            window,
+          }),
+          /DataCloneError|cyclic object value/);
+
+        await browser.test.assertRejects(
+          storage.set({"test-prop2": function func() {}}),
+          /DataCloneError/);
+
         const recentChanges = await globalChanges;
 
         browser.test.assertEq("value1", recentChanges["test-prop1"].oldValue, "oldValue correct");
         browser.test.assertEq("object", typeof(recentChanges["test-prop1"].newValue), "newValue is obj");
         clearGlobalChanges();
 
         data = await storage.get({"test-prop1": undefined, "test-prop2": undefined});
         let obj = data["test-prop1"];
 
+        if (areaName === "local") {
+          browser.test.assertEq(String(date), String(obj.date), "date part correct");
+          browser.test.assertEq("/regexp/", obj.regexp.toSource(), "regexp part correct");
+        } else {
+          browser.test.assertEq("1970-01-01T00:00:00.000Z", String(obj.date), "date part correct");
+
+          browser.test.assertEq("object", typeof obj.regexp, "regexp part is an object");
+          browser.test.assertEq(0, Object.keys(obj.regexp).length, "regexp part is an empty object");
+        }
+
         browser.test.assertEq("hello", obj.str, "string part correct");
         browser.test.assertEq(true, obj.bool, "bool part correct");
         browser.test.assertEq(null, obj.null, "null part correct");
         browser.test.assertEq(undefined, obj.undef, "undefined part correct");
-        browser.test.assertEq(undefined, obj.func, "function part correct");
         browser.test.assertEq(undefined, obj.window, "window part correct");
-        browser.test.assertEq("1970-01-01T00:00:00.000Z", obj.date, "date part correct");
-        browser.test.assertEq("/regexp/", obj.regexp, "regexp part correct");
         browser.test.assertEq("object", typeof(obj.obj), "object part correct");
         browser.test.assertTrue(Array.isArray(obj.arr), "array part present");
         browser.test.assertEq(1, obj.arr[0], "arr[0] part correct");
         browser.test.assertEq(2, obj.arr[1], "arr[1] part correct");
         browser.test.assertEq(2, obj.arr.length, "arr.length part correct");
-
-        obj = data["test-prop2"];
-
-        browser.test.assertEq("[object Object]", {}.toString.call(obj), "function serialized as a plain object");
-        browser.test.assertEq(0, Object.keys(obj).length, "function serialized as an empty object");
       } catch (e) {
         browser.test.fail(`Error: ${e} :: ${e.stack}`);
         browser.test.notifyFail("storage");
       }
     }
 
     browser.test.onMessage.addListener(msg => {
       let promise;