909672 - Make cross-compartment cloning possible with JS_StructuredClone

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Hordecki [:mhordecki] (UTC+2)]

As it stands now, JS_StructuredClone explicitly disallows cross-compartment cloning. It would be very useful to have it, though.

Comment 1

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: 0001-Bug-909672-Make-cross-compartment-cloning-possible-w.patch

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 795942 [details] [diff] [review]
0001-Bug-909672-Make-cross-compartment-cloning-possible-w.patch

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

Great work. Love the test.

::: js/src/jsapi-tests/testStructuredClone.cpp
@@ +24,5 @@
> +{
> +    JS::RootedObject g1(cx, JS_NewGlobalObject(cx, &global_class, NULL,
> +                        JS::FireOnNewGlobalHook));
> +    JS::RootedObject g2(cx, JS_NewGlobalObject(cx, &global_class, NULL,
> +                        JS::FireOnNewGlobalHook));

The jsapi-test framework creates one global for you; it's called `global` and you're already in the right compartment to use it (although feel free to keep g1 if you think the code is clearer that way).

And, you can also delete global_class and just call the createGlobal() method:

    JS::RootedObject g2(cx, createGlobal());
    CHECK(g2);

@@ +36,5 @@
> +        JS::RootedValue prop(cx, JS::Int32Value(1337));
> +
> +        v1 = JS::ObjectOrNullValue(JS_NewObject(cx, NULL, NULL, NULL));
> +        CHECK(v1.isObject());
> +        JS_SetProperty(cx, &v1.toObject(), "prop", prop);

If you decide to use the provided global rather than g1, you can just write:

    EVAL("({prop: 1337})", &v1);

@@ +47,5 @@
> +        JS_StructuredClone(cx, v1, v2.address(), NULL, NULL);
> +        CHECK(v2.isObject());
> +
> +        JS::RootedValue prop(cx);
> +        JS_GetProperty(cx, &v2.toObject(), "prop", &prop);

CHECK the return value of JS_StructuredClone and JS_GetProperty. (Also JS_SetProperty above.)

@@ +49,5 @@
> +
> +        JS::RootedValue prop(cx);
> +        JS_GetProperty(cx, &v2.toObject(), "prop", &prop);
> +        CHECK(prop.isInt32());
> +        CHECK(v1.address() != v2.address());

This checks that v1 and v2 are different variables (which they are). You want to check that v1.toObject() and v2.toObject() are different objects:

  CHECK(&v1.toObject() != &v2.toObject());

::: js/src/vm/StructuredClone.cpp
@@ +1493,3 @@
>      JSAutoStructuredCloneBuffer buf;
> +    {
> +        mozilla::Maybe<JSAutoCompartment> ac;

Use AutoCompartment instead of JSAutoCompartment here.

@@ +1493,4 @@
>      JSAutoStructuredCloneBuffer buf;
> +    {
> +        mozilla::Maybe<JSAutoCompartment> ac;
> +        if(value.isObject()) {

Style nit: space between "if(" please.

More importantly, I think this should also happen if value.isString(), like maybe

    if (value.isGCThing()) {
        if (value.isObject())
            ac.construct(cx, &value.toObject());
        else
            ac.construct(cx, value.toString()->compartment());
    }

and I hate to say it, but this case should have a test.

Comment 3

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: 0001-Bug-909672-Make-cross-compartment-cloning-possible-w.patch

AFAIK JSStrings do not belong to compartments, but zones instead. Seeing as I cannot enter a compartment to read the value in this case, I've done a workaround in which I create a cross-compartment wrapper (which is basically a straight-out copy) when I encounter a string as a parameter for JS_StructuredClone.

On second thought, maybe I should have made the copying explicit - who knows when the behavior of JSCompartment::wrap(JSString **) changes..

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 797782 [details] [diff] [review]
0001-Bug-909672-Make-cross-compartment-cloning-possible-w.patch

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

JSCompartment::wrap is precisely the right thing to use.

::: js/src/jsapi-tests/testStructuredClone.cpp
@@ +67,5 @@
> +
> +        CHECK(JS_StructuredClone(cx, v1, v2.address(), NULL, NULL));
> +        CHECK(v2.isString());
> +        CHECK(v2.toString());
> +        

Nit: please remove the whitespace on the blank line

::: js/src/vm/StructuredClone.cpp
@@ +1485,5 @@
>      AssertHeapIsIdle(cx);
>      CHECK_REQUEST(cx);
> +
> +    // Strings are associated with zones, not compartments,
> +    // so we copy the string by wrapping it.

// so we copy the string, if needed, using wrap().

@@ +1490,5 @@
> +    if (value.isString()) {
> +      RootedString strValue(cx, value.toString());
> +      if (!cx->compartment()->wrap(cx, strValue.address())) {
> +        return false;
> +      }

More nits: use 4-space indentation and lose the {} braces on this if-statement, since both the condition and the body are single lines.

Comment 5

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: 0001-Bug-909672-Make-cross-compartment-cloning-possible-w.patch

Thanks for the review!

Yeah, my vim was acting up; I've corrected my .vimrc so I hope those silly errors will no longer be a problem.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86114140b797

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86114140b797