Fix CreateStructuredClone to copy across compartments

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

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

7 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

7 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

7 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

7 years ago
Created attachment 479165 [details] [diff] [review]
WIP 1

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

7 years ago
Created attachment 479175 [details] [diff] [review]
WIP 1, really
Attachment #479165 - Attachment is obsolete: true

Comment 7

7 years ago
There is a lot of unrelated changes in this patch.

Comment 8

7 years ago
Actually no. bugzilla was lying to me. Ignore #7.
(Assignee)

Comment 9

7 years ago
Created attachment 479428 [details] [diff] [review]
WIP 2

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

7 years ago
Created attachment 479503 [details] [diff] [review]
WIP 3

- 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

7 years ago
Created attachment 479579 [details] [diff] [review]
WIP 4

- TypedArray support.
- More tests.
Attachment #479503 - Attachment is obsolete: true
Blocks: 600725
You don't seem to be throwing DATA_CLONE_ERRs per spec?
(Assignee)

Comment 13

7 years ago
Created attachment 479888 [details] [diff] [review]
v5

- 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

7 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

7 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
ES5 does not require it but practical implementations may have to use quotas to mitigate DoS attacks and accidents.

/be

Comment 18

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

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

Comment 26

7 years ago
http://hg.mozilla.org/mozilla-central/rev/288c6ac359c3
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.