Closed Bug 595297 Opened 10 years ago Closed 10 years ago

Fix CreateStructuredClone to copy across compartments


(Core :: JavaScript Engine, defect)

Other Branch
Not set





(Reporter: jorendorff, Assigned: jorendorff)



(Whiteboard: [fixed-in-tracemonkey])


(1 file, 5 obsolete files)

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.
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;

  JS_WriteStructured(JSContext *cx, jsval v, JSBytes *bytesp);

  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.
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.
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.
Attached patch WIP 1 (obsolete) — Splinter Review
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
Attached patch WIP 1, really (obsolete) — Splinter Review
Attachment #479165 - Attachment is obsolete: true
There is a lot of unrelated changes in this patch.
Actually no. bugzilla was lying to me. Ignore #7.
Attached patch WIP 2 (obsolete) — Splinter Review
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.
Attached patch WIP 3 (obsolete) — Splinter Review
- 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
Attached patch WIP 4 (obsolete) — Splinter Review
- TypedArray support.
- More tests.
Attachment #479503 - Attachment is obsolete: true
You don't seem to be throwing DATA_CLONE_ERRs per spec?
Attached patch v5Splinter Review
- 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)
For your entertainment:

var x = [];
for (var i = 0; i < 99; i++)
    x = [x, x];
serialize(x);  // clone awhile. clone forever
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.
ES5 does not require it but practical implementations may have to use quotas to mitigate DoS attacks and accidents.

Comment on attachment 479888 [details] [diff] [review]

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+
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".

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.
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 *****
(that is to say: jsclone.cpp & jsclone.h need the "/" at the start of line 2 removed.)
(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.
Sure, either way. (I suggested the other to better match a file in the same directory (picked at random), jscntxt.cpp)
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.