1440372 - StructuredClone comments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Jason Orendorff [:jorendorff]]

After paging all this one once again (and discovering that the spec has changed to match our implementation more closely) I decided to document what I found.

I hope we can simplify StructuredCloneScope, but that's another bug.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: StructuredClone comments

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8953116 [details] [diff] [review]
StructuredClone comments

Review of attachment 8953116 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful, thank you! I will be very grateful the next time I need to page this all back in. (And hopefully you'll do your magic and clean things up in other bugs so that there's less weirdness to page in, too.)

::: js/public/StructuredClone.h
@@ +70,5 @@
> + * However, HTML also supports `Transferable` objects, which, when cloned, can
> + * be moved from the source object into the clone, like when you take a
> + * photograph of someone and it steals their soul.
> + * See <https://developer.mozilla.org/en-US/docs/Web/API/Transferable>.
> + * We support cloning and transferring objects of many types.

This makes it sound like the only advantage of structured cloning over JSON is Transferables. I think of cyclic data and additional data types (typed arrays, Map, Set, ...) as other important advantages.

@@ +76,5 @@
> + * For example, when we transfer an ArrayBuffer (within a process), we "detach"
> + * the ArrayBuffer, embed the raw buffer pointer in the serialized data, and
> + * later install it in a new ArrayBuffer in the destination realm. Ownership
> + * of that buffer memory is transfered from the original ArrayBuffer to the
> + * serialized data and then to the clone.

*transferred

(but "transferable" is ok. English be consistent.)

@@ +80,5 @@
> + * serialized data and then to the clone.
> + *
> + * This only makes sense within a single process. When we transfer an
> + * ArrayBuffer to another process, the contents of the buffer must be copied
> + * into the serialized data.

The origin still gets detached, though.

@@ +84,5 @@
> + * into the serialized data.
> + *
> + * ArrayBuffers are actually a lucky case; some objects (like MessagePorts)
> + * can't reasonably be transferred by value into serialized data -- it's
> + * pointers or nothing.

Hmm... would this work better either as "stored by value" or "transferred into"? "Transferred by value" sounds confusing to me.

@@ +104,5 @@
> +     *
> +     * When reading, this means: Accept transferred objects and buffers
> +     * (pointers). The caller promises that the serialized data was written
> +     * using this API (otherwise, the serialized data may contain bogus
> +     * pointers, leading to undefined behavior).

Hm... reading this makes me wonder why we don't store JSAtom pointers in SameThread clones. I guess we'd have to trace them. (Not a problem with ArrayBuffer pointers because they aren't pointers to GC things.)

@@ +131,5 @@
> +     * versions of Firefox years from now. Transferable objects are limited to
> +     * ArrayBuffers, whose contents are copied into the serialized data (rather
> +     * than just writing a pointer).
> +     *
> +     * When writing, this means: Do not accept pointers.

s/writing/reading/

::: js/src/vm/StructuredClone.cpp
@@ +12,3 @@
>   *
> + * -   StructuredSerialize examines a JS value and produces a graph of Records.
> + * -   StructuredDeserialize walks the Records and produces a new JS value.

Record! That's the term I was happy about the spec introducing. Not sure why I couldn't find it the other day.

@@ +17,2 @@
>   *
> + * -   We call the two phases "write" and "read".

I wonder if we'd be better off using the spec terms. JSAutoStructuredCloneBuffer's read*/write* methods are confusing, because I'm never sure who the subject of the sentence is. Or at least, "serialize" and "deserialize".

@@ +428,5 @@
>  
>      // Stack of objects with properties remaining to be read.
>      AutoValueVector objs;
>  
> +    // Array of all objects read during this deserialization, for reading

s/reading/resolving/

@@ +438,5 @@
> +    // is usually no problem, since both algorithms do a single linear pass over
> +    // the serialized data. There is one hitch; see readTypedArray.
> +    //
> +    // The values in this vector are objects, except it can temporarily have
> +    // one `undefined` placeholder value (the readTypedArray hack).x

s/x$//

Why "one"? If you have multiple typed arrays, you'd get a placeholder for each. "except for temporary 'undefined' placeholder values (the readTypedArray hack)."?

Comment 3

[Jason Orendorff [:jorendorff]]

> I wonder if we'd be better off using the spec terms. JSAutoStructuredCloneBuffer's read*/write* methods are confusing, because I'm never sure who the subject of the sentence is. Or at least, "serialize" and "deserialize".

Yeah. I think we should switch, but haven't actually written a patch for it yet. We probably need to do more significant surgery on the API and implementation.

Comment 4

[Jason Orendorff [:jorendorff]]

> Why "one"? If you have multiple typed arrays, you'd get a placeholder for each. "except for temporary 'undefined' placeholder values (the readTypedArray hack)."?

I think you can only get one at a time -- with well-formed input. You can get more than one with bogus input, but then it'll end in an error.

readTypedArray() does `allObjs.append(dummy)` at the top and `allObjs[placeholderIndex].set(vp);` at the bottom. In between, it can read at most one ArrayBuffer or SharedArrayBuffer. Fully general recursion isn't happening here (and generally we avoid it).

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/523ecd9f034b0fc9ba6e24d87cfba72518c05abe
Bug 1440372 - StructuredClone comments. r=sfink.

Comment 6

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/523ecd9f034b