1370752 - Structured clone rather than sanitizing storage values

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P1)

Description

[Kris Maglione [:kmag]]

In the near future, we'd like to move the storage API backend to IndexedDB, which will allow storing arbitrary structured-clone-compatible data. In the mean time, the sanitization step for storing data is quite expensive, especially due to the use of X-rays. It would be much more efficient to serialize the storage data into a StructuredCloneHolder directly out of the add-on compartment, and only perform JSON down-conversion when we write it to disk.

This means that some values which were previously silently converted to JSON-compatible values (like functions) will cause errors. But it also means that when we switch to IndexedDB, we'll be able to store much richer data structures.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1370752: Part 1 - Enter the correct target compartment when creating structured clone holder.

Review commit: https://reviewboard.mozilla.org/r/147746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147746/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1370752: Part 2 - Allow fallback serializer when JSON.serialize fails.

Currently, we need to be able to handle serializing non-JSON-compatible
objects without catastrophically failing to save the storage file. Ideally, we
would ensure this in the ordinary toJSON method. However, that would require
a unnecessary extra calls to JSON.serialize for each object that needs to be
sanitized before returning a JSON-safe value, which is more expensive than we
can afford.

The fallback toJSONSafe method allows us to do this only when necessary, due
to an initial failed JSON serialization.

Review commit: https://reviewboard.mozilla.org/r/147748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147748/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.

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 arbitrary structured-clone-compatible
  data structures.

Review commit: https://reviewboard.mozilla.org/r/147750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147750/

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8876352 [details]
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.

https://reviewboard.mozilla.org/r/147750/#review152104

::: commit-message-d8161:24
(Diff revision 1)
> +- 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 arbitrary structured-clone-compatible

nit: full support *for*

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8876352 [details]
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.

https://reviewboard.mozilla.org/r/147750/#review152110

::: toolkit/components/extensions/ExtensionStorage.jsm:109
(Diff revision 1)
> +    let promise = this.jsonFilePromises.get(extensionId);
> +    if (!promise) {
> +      promise = this._readFile(extensionId);
> +      this.jsonFilePromises.set(extensionId, promise);
> +    }
> +    return promise;

can't you just use DefaultMap?

::: toolkit/components/extensions/ExtensionStorage.jsm:150
(Diff revision 1)
> -  set(extensionId, items) {
> -    return this.read(extensionId).then(extData => {
> -      let changes = {};
> +    let changes = {};
> -      for (let prop in items) {
> +    for (let prop in items) {
> -        let item = items[prop];
> +      let item = items[prop];
> -        changes[prop] = {oldValue: extData[prop], newValue: item};
> +      changes[prop] = {oldValue: serialize(jsonFile.data.get(prop)), newValue: serialize(item)};

hasn't this already been serialized in the child process?

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8876352 [details]
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.

https://reviewboard.mozilla.org/r/147750/#review152118

I keep getting confused reading this, ExtensionStorage.jsm would benefit from comments on the individual methods documenting the types of arguments and return values.

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8876352 [details]
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.

https://reviewboard.mozilla.org/r/147750/#review152122

correcting the flags, i goofed when marking this one r+ before

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8876350 [details]
Bug 1370752: Part 1 - Enter the correct target compartment when creating structured clone holder.

https://reviewboard.mozilla.org/r/147746/#review152124

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8876351 [details]
Bug 1370752: Part 2 - Allow fallback serializer when JSON.serialize fails.

https://reviewboard.mozilla.org/r/147748/#review152126

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b483c5bb3d4384ec0d6efb365d9c4aa8967a3bd
Bug 1370752: Part 1 - Enter the correct target compartment when creating structured clone holder. r=aswan

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8876352 [details]
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.

https://reviewboard.mozilla.org/r/147750/#review152110

> hasn't this already been serialized in the child process?

Only if it's been set during this session, in which case yes, and the existing StructuredCloneBlob is used. If it was read from a JSON file during this session, though, then no.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b483c5bb3d4

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8876351 [details]
Bug 1370752: Part 2 - Allow fallback serializer when JSON.serialize fails.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147748/diff/1-2/

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8876352 [details]
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147750/diff/1-2/

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8876352 [details]
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.

https://reviewboard.mozilla.org/r/147750/#review159008

This all seems okay.  Mostly for my own understanding: why can't we use STructuredCloneHolder for primitive (non-object) types?

::: toolkit/components/extensions/test/xpcshell/test_ext_storage.js:277
(Diff revision 2)
>  
> -        await storage.set({"test-prop2": function func() {}});
> +        await browser.test.assertRejects(
> +          storage.set({
> +            window,
> +          }),
> +          /DataCloneError|cyclic object value/);

why "cyclic object value"?

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8876352 [details]
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.

https://reviewboard.mozilla.org/r/147750/#review159008

We can, and do. But we only want to structured clone serialize storage values, not arrays of storage keys, or `items` objects themselves, which contain multiple keys with separate values.

> why "cyclic object value"?

Because storage.sync still uses JSON serialization.

Comment 18

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c4bf59ab966a8ec17181d85cc1fc4be7450cca3
Bug 1370752: Part 2 - Allow fallback serializer when JSON.serialize fails. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/42d3c1599af53b047d7ccd6b1c92ab08975284d7
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values. r=aswan

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out parts 2 and 3 for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=113126664&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/43fca39fe75c9af03828c315c6cd8108d3ec597f

Comment 20

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6232176ba57ffdd3c06b201e0e7562256936c76
Bug 1370752: Part 2 - Allow fallback serializer when JSON.serialize fails. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/317331a50bde2f2e59bcd6074078c1d6ec2bd20c
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values. r=aswan

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f6232176ba57
https://hg.mozilla.org/mozilla-central/rev/317331a50bde

Comment 22

[YF (Yang)]

correct Milestone.