Closed
Bug 927765
Opened 12 years ago
Closed 12 years ago
Need a way to export objects from sandboxes to content
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: gkrizsanits, Assigned: gkrizsanits)
Details
Attachments
(2 files, 1 obsolete file)
7.84 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
3.50 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•12 years ago
|
Assignee: nobody → gkrizsanits
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 1•12 years ago
|
||
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 2•12 years ago
|
||
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+
Assignee | ||
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
(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.
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #818318 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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•12 years ago
|
||
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());
Assignee | ||
Comment 9•12 years ago
|
||
(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•12 years ago
|
||
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)
Assignee | ||
Comment 11•12 years ago
|
||
(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•12 years ago
|
||
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 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/595c7dc7a02a
https://hg.mozilla.org/mozilla-central/rev/9916cc731d15
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•