Need a way to export objects from sandboxes to content

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Assignee: nobody → gkrizsanits
OS: Linux → All
Hardware: x86_64 → All
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.
Attachment #818318 - Flags: feedback?(bobbyholley+bmo)
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?
Attachment #818318 - Flags: feedback?(bobbyholley+bmo) → feedback+
(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.
(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.
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.
Attachment #818963 - Flags: review?(bobbyholley+bmo)
Attachment #818318 - Attachment is obsolete: true
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...
Attachment #818967 - Flags: review?(bobbyholley+bmo)
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 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());
(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 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)
(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 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.
Attachment #818963 - Flags: review?(bobbyholley+bmo) → review+
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).
Attachment #818967 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/595c7dc7a02a
https://hg.mozilla.org/mozilla-central/rev/9916cc731d15
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.