927765 - Need a way to export objects from sandboxes to content

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

As we talked about this with Bobby, content-script sandboxes need to have access to a createObjectIn like API, until we have some meaningful xrays for vanilla JS Objects.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: options for createObjectIn. v1

The next patch will simply extend the exportHelpers functions with a helper that calls this generalized xpc::createObjectIn function.

I just wanted to get a quick feedback from you if you agree with this approach so far before I continue.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 818318 [details] [diff] [review]
part1: options for createObjectIn. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3016,5 @@
> +        return false;
> +    }
> +
> +    RootedObject scope(cx, options.checkedUnwrap ? js::CheckedUnwrap(&vobj.toObject())
> +                                                 : js::UncheckedUnwrap(&vobj.toObject()));

I don't think we need this option. We can just unconditionally us a CheckedUnwrap, because it will always succeed for the chrome caller of Cu.createObjectIn.

@@ +3018,5 @@
> +
> +    RootedObject scope(cx, options.checkedUnwrap ? js::CheckedUnwrap(&vobj.toObject())
> +                                                 : js::UncheckedUnwrap(&vobj.toObject()));
> +    if (!scope) {
> +        JS_ReportError(cx, "Permission denied");

More descriptive?

@@ +3027,5 @@
>          JSAutoCompartment ac(cx, scope);
>          obj = JS_NewObject(cx, nullptr, nullptr, scope);
>          if (!obj)
> +            return false;
> +        if (!options.defineAs.isUndefined()) {

Can we make the caller do the legwork of converting to a jsid, and use JSID_VOID as a sentinel?

Comment 3

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #2)
> > +    RootedObject scope(cx, options.checkedUnwrap ? js::CheckedUnwrap(&vobj.toObject())
> > +                                                 : js::UncheckedUnwrap(&vobj.toObject()));
> 
> I don't think we need this option. We can just unconditionally us a
> CheckedUnwrap, because it will always succeed for the chrome caller of
> Cu.createObjectIn.

I kind of hoped you're going to say that. I don't get why did we do uncheckedUnwrap and was a bit scared that we need it for some crazy testing purposes around SpecialPowers.

> >          obj = JS_NewObject(cx, nullptr, nullptr, scope);
> >          if (!obj)
> > +            return false;
> > +        if (!options.defineAs.isUndefined()) {
> 
> Can we make the caller do the legwork of converting to a jsid, and use
> JSID_VOID as a sentinel?

ParseJsid you mean? Sure.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> I kind of hoped you're going to say that. I don't get why did we do
> uncheckedUnwrap and was a bit scared that we need it for some crazy testing
> purposes around SpecialPowers.

Anything in SpecialPowers runs with System Principal (that's why using the SpecialPowers wrappers to proxy activity into the SpecialPowers scope gives you privileges). We have the invariant that the caller of Cu.anything() is always chrome.

Comment 5

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: options for createObjectIn. v2

Yes. So imo we should change all these UncheckedUnwrap in these Cu methods to CheckedUnwrap...

Anyway, I've just changed only in this one for now in this patch.

Comment 6

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: createObjectIn for exportHelpers. v1

You might wonder why did I put this local static CreateObjectIn in xpc namespace... it's about the stupid c++ namespace lookup machinery. So if it's not in the xpc namespace than the one that is in it shadows it even though the function signature is different. And using :: does not help it either. I could use another function name alternatively...

Comment 7

[Tom S. (please needinfo tschuster)]

Comment on attachment 818963 [details] [diff] [review]
part1: options for createObjectIn. v2

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3026,5 @@
>          JSAutoCompartment ac(cx, scope);
>          obj = JS_NewObject(cx, nullptr, nullptr, scope);
>          if (!obj)
> +            return false;
> +        if (options.defineAs != JSID_VOID) {

I would use JSID_IS_VOID.

Comment 8

[Tom S. (please needinfo tschuster)]

Comment on attachment 818967 [details] [diff] [review]
part2: createObjectIn for exportHelpers. v1

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

::: js/xpconnect/src/Sandbox.cpp
@@ +556,5 @@
> +namespace xpc {
> +static bool
> +CreateObjectIn(JSContext *cx, unsigned argc, jsval *vp)
> +{
> +    MOZ_ASSERT(cx);

This seems useless.

@@ +557,5 @@
> +static bool
> +CreateObjectIn(JSContext *cx, unsigned argc, jsval *vp)
> +{
> +    MOZ_ASSERT(cx);
> +    if (argc < 1) {

args.length()

@@ +562,5 @@
> +        JS_ReportError(cx, "Function requires at least 1 argument");
> +        return false;
> +    }
> +
> +    CallArgs args = CallArgsFromVp(argc, vp);

Move that to the top.

@@ +565,5 @@
> +
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    RootedObject optionsObj(cx);
> +    bool calledWithOptions = argc > 1;

args.length()

@@ +578,5 @@
> +    CreateObjectOptions options(cx, optionsObj);
> +    if (calledWithOptions && !options.Parse())
> +        return false;
> +
> +    RootedValue rv(cx);

this can be:
return xpc::CreateObjectIn(cx, args[0], args.rval());

@@ +583,5 @@
> +    if (!xpc::CreateObjectIn(cx, args.get(0), options, &rv))
> +        return false;
> +
> +    args.rval().set(rv);
> +    return true;

This can be:
return xpc::CreateObjectIn(cx, args[0], args.rval());

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Tom Schuster [:evilpie] from comment #8)
> > +    MOZ_ASSERT(cx);
> 
> This seems useless.

Whoops, indeed, thanks.

> > +    CallArgs args = CallArgsFromVp(argc, vp);
> 
> Move that to the top.

Not sure, I let Bobby decide that, I'm fine with moving it up too.

> @@ +578,5 @@
> > +    CreateObjectOptions options(cx, optionsObj);
> > +    if (calledWithOptions && !options.Parse())
> > +        return false;
> > +
> > +    RootedValue rv(cx);
> 
> this can be:
> return xpc::CreateObjectIn(cx, args[0], args.rval());

Wat?

> 
> @@ +583,5 @@
> > +    if (!xpc::CreateObjectIn(cx, args.get(0), options, &rv))
> > +        return false;
> > +
> > +    args.rval().set(rv);
> > +    return true;
> 
> This can be:
> return xpc::CreateObjectIn(cx, args[0], args.rval());

Not, this does not compile. Not the args[0] nor the args.rval() part.

Comment 10

[Tom S. (please needinfo tschuster)]

Comment on attachment 818963 [details] [diff] [review]
part1: options for createObjectIn. v2

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

I just compiled this with my changes. Your tree seems to be outdated.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3036,3 @@
>      }
>  
>      if (!JS_WrapObject(cx, obj.address()))

&obj)

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Tom Schuster [:evilpie] from comment #10)
> Comment on attachment 818963 [details] [diff] [review]
> part1: options for createObjectIn. v2
> 
> Review of attachment 818963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just compiled this with my changes. Your tree seems to be outdated.
> 
> ::: js/xpconnect/src/XPCComponents.cpp
> @@ +3036,3 @@
> >      }
> >  
> >      if (!JS_WrapObject(cx, obj.address()))
> 
> &obj)

Hmmm... I don't know what was with my tree but it does compile after update indeed. Thanks!!!

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 818963 [details] [diff] [review]
part1: options for createObjectIn. v2

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

r=bholley with my and Tom's comments addressed.

::: js/xpconnect/src/xpcprivate.h
@@ +3654,5 @@
>  protected:
>      bool ParseGlobalProperties();
>  };
>  
> +class MOZ_STACK_CLASS CreateObjectOptions : public OptionsBase {

I know it's ugly, but this should probably be CreateObjectInOptions. CreateObject is just too generic of a name.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 818967 [details] [diff] [review]
part2: createObjectIn for exportHelpers. v1

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

r=bholley with Tom's comments addressed.

::: js/xpconnect/src/Sandbox.cpp
@@ +562,5 @@
> +        JS_ReportError(cx, "Function requires at least 1 argument");
> +        return false;
> +    }
> +
> +    CallArgs args = CallArgsFromVp(argc, vp);

Yeah, move it up. The idea is that eventually we'll change the JSNative signature to a CallArgs, and so we want all the conversions to be super easy (i.e. no references to argc midway through the function).

Comment 14

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=4d7a25b8db75

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/595c7dc7a02a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9916cc731d15

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/595c7dc7a02a
https://hg.mozilla.org/mozilla-central/rev/9916cc731d15