Structured clone rather than sanitizing storage values

NEW
Assigned to

Status

()

Toolkit
WebExtensions: General
P1
normal
3 months ago
a month ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 3 bugs, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 months 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

3 months ago
Blocks: 1368119

Updated

3 months ago
See Also: → bug 1371253

Updated

3 months ago
Blocks: 1226547, 1363905

Updated

3 months ago
See Also: → bug 1371255
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1371253
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 months 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 months 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 months 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 months 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 months 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 months 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 months ago
Keywords: leave-open
(Assignee)

Comment 11

2 months 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 months ago
Priority: -- → P1
Whiteboard: triaged
(Assignee)

Comment 12

2 months 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 13

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b483c5bb3d4
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8876350 - Attachment is obsolete: true

Comment 16

2 months 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

2 months 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 month 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
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
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

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

Comment 20

a month 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

Comment 21

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6232176ba57
https://hg.mozilla.org/mozilla-central/rev/317331a50bde
You need to log in before you can comment on or make changes to this bug.