Closed Bug 1370752 Opened 7 years ago Closed 7 years ago

Structured clone rather than sanitizing storage values

Categories

(WebExtensions :: General, enhancement, P1)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla56

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(2 files, 1 obsolete file)

Bug 1370752: Part 2 - Allow fallback serializer when JSON.serialize fails.
(deleted), text/x-review-board-request
aswan
: review+
Details
Bug 1370752: Part 3 - Use structured clone rather than JSON to sanitize storage values.
(deleted), text/x-review-board-request
aswan
: review+
Details
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.
Blocks: 1368119
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 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 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 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 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 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+
Keywords: leave-open
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
Priority: -- → P1
Whiteboard: triaged
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.
Attachment #8876350 - Attachment is obsolete: true
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+
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.
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
Flags: needinfo?(kmaglione+bmo)
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
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
correct Milestone.
Target Milestone: mozilla55 → mozilla56
Blocks: 1371255
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: