Closed Bug 909672 Opened 6 years ago Closed 6 years ago

Make cross-compartment cloning possible with JS_StructuredClone

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: mhordecki, Assigned: mhordecki)

References

Details

Attachments

(1 file, 2 obsolete files)

As it stands now, JS_StructuredClone explicitly disallows cross-compartment cloning. It would be very useful to have it, though.
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.
Attachment #795942 - Flags: review?(jorendorff) → review+
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..
Attachment #795942 - Attachment is obsolete: true
Attachment #797782 - Flags: review?(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.
Attachment #797782 - Flags: review?(jorendorff) → review+
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.
Attachment #797782 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/86114140b797
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.