Closed
Bug 595297
Opened 14 years ago
Closed 14 years ago
Fix CreateStructuredClone to copy across compartments
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
(Whiteboard: [fixed-in-tracemonkey])
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
Currently, nsContentUtils::CreateStructuredClone creates a copy in the same compartment. DOM Workers and IndexedDB use it.
For Workers, at least, we are cloning in order to ship a message off to another compartment. We have to create the clone in the target worker's compartment. However, since the source compartment and the target compartment are both single-threaded, it's not immediately obvious how to do this without fancy thread-synchronization footwork.
Assignee | ||
Comment 1•14 years ago
|
||
A nice non-fancy approach:
I think what IndexedDB actually wants is two separate functions,
serialize :: jsval -> ByteArray
deserialize :: ByteArray -> jsval
such that the composition (deserialize . serialize) is an implementation of the structured cloning algorithm. I think this is also what is needed for Workers.
So, suppose we add an API for these two functions:
struct JSBytes {
void *data;
size_t length;
};
JSBool
JS_WriteStructured(JSContext *cx, jsval v, JSBytes *bytesp);
JSBool
JS_ReadStructured(JSContext *cx, const JSBytes *bytes, jsval *vp);
DOM Workers will call JS_WriteStructured in the sending thread, then pass the data to the receiving thread which will do JS_ReadStructured (followed by JS_free).
IndexedDB will use the two functions separately for saving and loading data.
Neither one will call nsContentUtils::CreateStructuredClone anymore.
Assignee | ||
Comment 2•14 years ago
|
||
Hacking on this now.
Gregor, does this block compartmental GC? I (tentatively) claim it does not, because we're just allocating stuff in the wrong compartment. We're not allocating stuff on the wrong thread.
Assignee | ||
Comment 3•14 years ago
|
||
bent, suppose we implement this and start using it for IndexedDB. We need to choose a serialization format that will be portable across platforms, right? And we can never change it incompatibly, once we settle on one, because the data will be stored in everyone's profiles?
Well, we can change the format in the future if we need to, but it would mean we would bump the schema version of the databases and force everyone to lose their existing data. We could do some migration step too if we didn't like that.
And yes, we'd want it to be portable.
Assignee | ||
Comment 5•14 years ago
|
||
Don't expect much. This might not even compile.
Still TODO:
- finish hooking up error messages
- add shell builtins to test this
- write tests
- make jsworkers.cpp use these
- make Gecko Web workers use these
- make IndexedDB use these
- fix bugs
Assignee: general → jorendorff
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #479165 -
Attachment is obsolete: true
Comment 7•14 years ago
|
||
There is a lot of unrelated changes in this patch.
Comment 8•14 years ago
|
||
Actually no. bugzilla was lying to me. Ignore #7.
Assignee | ||
Comment 9•14 years ago
|
||
This fixes bugs, contains some tests and passes them locally. I don't know of any bugs in it except that the errors need some thought.
Assignee | ||
Comment 10•14 years ago
|
||
- Rename jsstructured.{h,cpp} -> jsclone.{h,cpp}.
- Rename "Structured" to "StructuredClone" and "SD" to "SC" everywhere.
- More tests.
- Don't expose all the SCTAG constants, just JS_SCTAG_USER_{MIN,MAX}
to tell the application what the tag values it can use
- Add a constant in jsapi.h saying that this is serialization format #1
(Gecko can stamp this value into some database header and check it on load.)
- In jsapi.h, add a little RAII sugar for JS_WriteStructuredClone.
- Add JS_StructuredClone function.
Attachment #479175 -
Attachment is obsolete: true
Attachment #479428 -
Attachment is obsolete: true
Assignee | ||
Comment 11•14 years ago
|
||
- TypedArray support.
- More tests.
Attachment #479503 -
Attachment is obsolete: true
Comment 12•14 years ago
|
||
You don't seem to be throwing DATA_CLONE_ERRs per spec?
Assignee | ||
Comment 13•14 years ago
|
||
- never get properties from a prototype, even if the object changes during serialization (this required a format change)
- ignore XMLNamed properties
- more tests
- new reportError hook so gecko can throw a DOMException as required by HTML5
There's just one more exciting problem with this, which is that you can easily create a graph of, say, 50 objects that would cause serialize() to spit out many terabytes of data.
Attachment #479579 -
Attachment is obsolete: true
Attachment #479888 -
Flags: review?(gal)
Assignee | ||
Comment 14•14 years ago
|
||
For your entertainment:
var x = [];
for (var i = 0; i < 99; i++)
x = [x, x];
serialize(x); // clone awhile. clone forever
Assignee | ||
Comment 15•14 years ago
|
||
Waldo points out that JSON.stringify already has the same issue. So maybe we don't care.
Note that the structured clone algorithm might be changing soon so that it'll behave differently for cyclic and directed acyclic graphs.
http://www.w3.org/Bugs/Public/show_bug.cgi?id=10878
Comment 17•14 years ago
|
||
ES5 does not require it but practical implementations may have to use quotas to mitigate DoS attacks and accidents.
/be
Comment 18•14 years ago
|
||
Comment on attachment 479888 [details] [diff] [review]
v5
Awesome. After landing want to show jesse how to fuzz this with binary input (deserialize) and random data input (serialize)? Doesn't look like we will ever feed untrusted data into this, but still can help identifying problems.
Attachment #479888 -
Flags: review?(gal) → review+
Comment 19•14 years ago
|
||
Andreas asked me to comment on API names -- they all look good, although I would burn the extra chars on JS_{Read,Write}Uint32Pair to be precise about "pair of what type".
/be
Assignee | ||
Comment 20•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/19add492f64d
And a follow-up for comment 19:
http://hg.mozilla.org/tracemonkey/rev/288c6ac359c3
Whiteboard: [fixed-in-tracemonkey]
For what it's worth, the spec has now changed. It now allows both cycles as well as objects appearing more than once in the graph.
Comment 22•14 years ago
|
||
This introduced build waarning:
../../../mozilla/js/src/jsclone.cpp:2:1: warning: "/*" within comment
In file included from ../../../mozilla/js/src/jsclone.cpp:39:
../../../mozilla/js/src/jsclone.h:2:1: warning: "/*" within comment
since line 2 opens a new comment without closing the one from line 1:
1 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
2 /* ***** BEGIN LICENSE BLOCK *****
Comment 23•14 years ago
|
||
(that is to say: jsclone.cpp & jsclone.h need the "/" at the start of line 2 removed.)
Comment 24•14 years ago
|
||
(In reply to comment #23)
> (that is to say: jsclone.cpp & jsclone.h need the "/" at the start of line 2
> removed.)
Actually, they need "*/" added at the end of line 1.
Comment 25•14 years ago
|
||
Sure, either way. (I suggested the other to better match a file in the same directory (picked at random), jscntxt.cpp)
Comment 26•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•