Structured clone rather than sanitizing storage values

RESOLVED FIXED in mozilla56

Status

P1
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1368119

Updated

2 years ago
See Also: → bug 1371253

Updated

2 years ago
Blocks: 1226547, 1363905

Updated

2 years ago
See Also: → bug 1371255
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1371253
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
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*
Attachment #8876352 - Flags: review?(aswan) → review+

Comment 6

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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
Attachment #8876352 - Flags: review+ → review?(aswan)

Comment 9

2 years ago
mozreview-review
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
Attachment #8876350 - Flags: review?(aswan) → review+

Comment 10

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

https://reviewboard.mozilla.org/r/147748/#review152126
Attachment #8876351 - Flags: review?(aswan) → review+
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 11

2 years ago
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

Updated

2 years ago
Priority: -- → P1
Whiteboard: triaged
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8876350 - Attachment is obsolete: true

Comment 16

a year ago
mozreview-review
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"?
Attachment #8876352 - Flags: review?(aswan) → review+
(Assignee)

Comment 17

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 18

a year ago
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
(Assignee)

Updated

a year ago
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 20

a year ago
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
(Assignee)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 22

a year ago
correct Milestone.
Target Milestone: mozilla55 → mozilla56

Updated

a year ago
Blocks: 1371255

Updated

5 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.