856477 - Root XPComponents

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Tom S [:evilpie]]

Attachment: wip

Just so we don't duplicate work here. WIP patch, will clean up and ask for review when I am back at the end of the week.

Comment 1

[Tom S [:evilpie]]

Attachment: v1

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 734647 [details] [diff] [review]
v1

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

r=bholley with comments addressed.

::: js/xpconnect/src/XPCComponents.cpp
@@ -62,5 @@
> -    if (!JSVAL_IS_PRIMITIVE(v) &&
> -        nullptr != (xpc = nsXPConnect::GetXPConnect()) &&
> -        NS_SUCCEEDED(xpc->GetWrappedNativeOfJSObject(cx, JSVAL_TO_OBJECT(v),
> -                                                     getter_AddRefs(wn))) && wn &&
> -        NS_SUCCEEDED(wn->Native()->QueryInterface(iid, (void**)&iface)) && iface) {

Holy hell this is a terribly-written function! Your version is much better. :-)

@@ +74,1 @@
>          NS_RELEASE(iface);

I'd love to fix this up using nsCOMPtr and do_QueryWrappedNative in a brief followup patch, but you don't have to if you want want to.

@@ +352,5 @@
>                  if (NS_SUCCEEDED(xpc->WrapNative(cx, obj,
>                                                   static_cast<nsIJSIID*>(nsid),
>                                                   NS_GET_IID(nsIJSIID),
>                                                   getter_AddRefs(holder)))) {
> +                    JS::RootedObject idobj(cx);

Can you just do |using namespace JS;| at the top of the file and kill the namespacing on all these?

@@ +2287,4 @@
>          return ThrowAndFail(NS_ERROR_XPC_CANT_CREATE_WN, cx, _retval);
>      }
>  
> +    jsval ctorArgs[1] = {OBJECT_TO_JSVAL(iidObj)}; // XXX

What's this about. Doesn't this line need rooting changes?

@@ +2933,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (args.thisv().isPrimitive()) {
> +        XPCThrower::Throw(NS_ERROR_INVALID_ARG, cx); // XX

Why the XX?

@@ +3981,5 @@
>      }
>  
>      // We create a separate cx to do the sandbox evaluation. Scope it.
> +    RootedValue v(cx);
> +    RootedValue exn(cx);

Hm, I still think we should initialize these with UndefinedValue for good measure.

@@ +4327,3 @@
>      NS_ASSERTION(v.isObject(), "weird function");
>  
> +    JSObject *obj = JS_THIS_OBJECT(cx, vp); // XXX

Hm, don't you mean to get this from |args|? I'm assuming that's what the XX is about

@@ +4365,5 @@
>      JS::AutoIdArray ida(cx, JS_Enumerate(cx, obj));
>      if (!ida)
>          return NS_ERROR_FAILURE;
>  
> +    RootedId id(cx);

Hm, is this even necessary? Can't we just make sure that |ida| is rooted and then access it that way? It doesn't seem like |id| is ever mutated.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +1670,5 @@
> +                                    "x-bogus://XPConnect/Sandbox", 1, JSVERSION_DEFAULT,
> +                                    returnStringOnly, &rval);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    *rvalArg = rval;
> +    return rv;

return NS_OK

Comment 3

[Tom S [:evilpie]]

(In reply to Bobby Holley (:bholley) from comment #2)
> Comment on attachment 734647 [details] [diff] [review]
> v1
> 
> Review of attachment 734647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=bholley with comments addressed.
> 
> ::: js/xpconnect/src/XPCComponents.cpp
> @@ -62,5 @@
> > -    if (!JSVAL_IS_PRIMITIVE(v) &&
> > -        nullptr != (xpc = nsXPConnect::GetXPConnect()) &&
> > -        NS_SUCCEEDED(xpc->GetWrappedNativeOfJSObject(cx, JSVAL_TO_OBJECT(v),
> > -                                                     getter_AddRefs(wn))) && wn &&
> > -        NS_SUCCEEDED(wn->Native()->QueryInterface(iid, (void**)&iface)) && iface) {
> 
> Holy hell this is a terribly-written function! Your version is much better.
> :-)
> 
> @@ +74,1 @@
> >          NS_RELEASE(iface);
> 
> I'd love to fix this up using nsCOMPtr and do_QueryWrappedNative in a brief
> followup patch, but you don't have to if you want want to.
> 
I don't know how this stuff works.
> @@ +352,5 @@
> >                  if (NS_SUCCEEDED(xpc->WrapNative(cx, obj,
> >                                                   static_cast<nsIJSIID*>(nsid),
> >                                                   NS_GET_IID(nsIJSIID),
> >                                                   getter_AddRefs(holder)))) {
> > +                    JS::RootedObject idobj(cx);
> 
> Can you just do |using namespace JS;| at the top of the file and kill the
> namespacing on all these?
> 
We already do "use namespace js;", so we should never use JS::. We should fix the complete file, the next patch just removes the newly introduced ones.
> @@ +2287,4 @@
> >          return ThrowAndFail(NS_ERROR_XPC_CANT_CREATE_WN, cx, _retval);
> >      }
> >  
> > +    jsval ctorArgs[1] = {OBJECT_TO_JSVAL(iidObj)}; // XXX
> 
> What's this about. Doesn't this line need rooting changes?
> 
I am not sure if we want to root this. Terrence?
> @@ +2933,5 @@
> > +{
> > +    CallArgs args = CallArgsFromVp(argc, vp);
> > +
> > +    if (args.thisv().isPrimitive()) {
> > +        XPCThrower::Throw(NS_ERROR_INVALID_ARG, cx); // XX
> 
> Why the XX?
> 
Changed that to NS_ERROR_UNEXPECTED, because this is not an argument.
> @@ +3981,5 @@
> >      }
> >  
> >      // We create a separate cx to do the sandbox evaluation. Scope it.
> > +    RootedValue v(cx);
> > +    RootedValue exn(cx);
> 
> Hm, I still think we should initialize these with UndefinedValue for good
> measure.
> 
Okay, didn't look at this too much.
> @@ +4327,3 @@
> >      NS_ASSERTION(v.isObject(), "weird function");
> >  
> > +    JSObject *obj = JS_THIS_OBJECT(cx, vp); // XXX
> 
> Hm, don't you mean to get this from |args|? I'm assuming that's what the XX
> is about
> 
Yeah, I will fill a bug to fix that globally.
> @@ +4365,5 @@
> >      JS::AutoIdArray ida(cx, JS_Enumerate(cx, obj));
> >      if (!ida)
> >          return NS_ERROR_FAILURE;
> >  
> > +    RootedId id(cx);
> 
> Hm, is this even necessary? Can't we just make sure that |ida| is rooted and
> then access it that way? It doesn't seem like |id| is ever mutated.
> 
The rooting analysis says it is. I think the id might be moved even inside the loop before the second call.
> ::: js/xpconnect/src/nsXPConnect.cpp
> @@ +1670,5 @@
> > +                                    "x-bogus://XPConnect/Sandbox", 1, JSVERSION_DEFAULT,
> > +                                    returnStringOnly, &rval);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    *rvalArg = rval;
> > +    return rv;
> 
> return NS_OK

Comment 4

[Tom S [:evilpie]]

Attachment: v2

Terrence it would be cool if you could take a short look at this and the previous comments by Bobby. I however doubt I made anything to terrible to warrant a full review.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 735131 [details] [diff] [review]
v2

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +337,1 @@
>          name.ptr()[0] != '{') { // we only allow interfaces by name here

{ on new line.

@@ +1609,5 @@
>  
>  // static
>  nsresult
>  nsXPCComponents_ID::CallOrConstruct(nsIXPConnectWrappedNative *wrapper,
> +                                    JSContext * cx, HandleObject obj,

*cx

@@ +1610,5 @@
>  // static
>  nsresult
>  nsXPCComponents_ID::CallOrConstruct(nsIXPConnectWrappedNative *wrapper,
> +                                    JSContext * cx, HandleObject obj,
> +                                    uint32_t argc, Value * argv,

*argv

@@ +1611,5 @@
>  nsresult
>  nsXPCComponents_ID::CallOrConstruct(nsIXPConnectWrappedNative *wrapper,
> +                                    JSContext * cx, HandleObject obj,
> +                                    uint32_t argc, Value * argv,
> +                                    Value * vp, bool *_retval)

*vp

@@ +2076,5 @@
>      virtual ~nsXPCConstructor();
>  
>  private:
>      nsresult CallOrConstruct(nsIXPConnectWrappedNative *wrapper,
> +                             JSContext * cx, HandleObject obj,

*cx

@@ +2287,4 @@
>          return ThrowAndFail(NS_ERROR_XPC_CANT_CREATE_WN, cx, _retval);
>      }
>  
> +    Value args[1] = {ObjectValue(*iidObj)};

Oh, that's just awkward; if we had C++11's initializers we could do something nice here. I think JS_CallFunction* is going to root these immediately, so there isn't a hazard here, as such. However, in the long term, JS_CallFunction* should probably take a MutableHandleValue[] for clarity.

@@ +2674,5 @@
>  private:
>      static nsresult CallOrConstruct(nsIXPConnectWrappedNative *wrapper,
> +                                    JSContext *cx, HandleObject obj,
> +                                    uint32_t argc, Value *argv,
> +                                    Value * vp, bool *_retval);

*vp

@@ +3173,5 @@
>                                sandboxProtoProxy, js::ProxyIsCallable);
>  }
>  
>  template<typename Op>
> +bool BindPropertyOp(JSContext *cx, Op& op, PropertyDescriptor *desc, HandleId id,

&op

@@ +3602,5 @@
>    result.forget(out);
>    return NS_OK;
>  }
>  
>  // helper that tries to get a property form the options object

*twitch*  I was going to ask you to fix the spelling of "from" here until I realized that most of them are misspelled. We wouldn't want to go against the prevailing style after all.

::: js/xpconnect/src/xpcprivate.h
@@ +4195,5 @@
>  // will be coerced into strings. If an exception is thrown converting
>  // an exception to a string, evalInSandbox will return an NS_ERROR_*
>  // result, and cx->exception will be empty.
>  nsresult
> +xpc_EvalInSandbox(JSContext *cx, JSHandleObject sandbox, const nsAString& source,

The JSHandleFoo variants are defined for C compatibility. I think we should probably remove all of these now: I'll open a bug. For this patch, please use JS::HandleObject.

@@ +4200,3 @@
>                    const char *filename, int32_t lineNo,
> +                  JSVersion jsVersion, bool returnStringOnly,
> +                  JSMutableHandleValue rval);

...and JS::MutableHandleValue.

Comment 6

[Tom S [:evilpie]]

The previous commit
https://hg.mozilla.org/integration/mozilla-inbound/rev/b018c2f116e4
wasn't quite enough. We still have about 20 to go. New patch coming.

Comment 7

[Tom S [:evilpie]]

Attachment: second patch

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b018c2f116e4

Comment 9

[Tom S [:evilpie]]

Attachment: root left overs

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 737288 [details] [diff] [review]
root left overs

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +4307,5 @@
>      if (!cx)
>          return NS_ERROR_FAILURE;
>  
>      // first argument must be an object
> +    if (!vobj.isObject())

It looks like an existing bug, but I think this should probably be |!vobj.isObjectOrNull()|, since js::UncheckedUnwrap will happily dereference a nullptr if you pass one.

@@ +4315,2 @@
>      {
> +        JSObject *scope = js::UncheckedUnwrap(JSVAL_TO_OBJECT(vobj));

Then you can safely use |&vobj.toObject()| here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> but I think this should probably be |!vobj.isObjectOrNull()|

Why, exactly?  Seems like the isObject() check is in fact the right one, esp if the plan is to call toObject()!

Comment 12

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> > but I think this should probably be |!vobj.isObjectOrNull()|
> 
> Why, exactly?  Seems like the isObject() check is in fact the right one, esp
> if the plan is to call toObject()!

/me goes to scrutinize Value.h in more detail

Ah, you are quite correct. I had assumed that NULL was stored as isObject() && !toObject() because of the combined presence of isObjectOrNull() and the semantics of isGCThing(). It seems that Value's encoding is cleverer than I gave it credit for.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 737288 [details] [diff] [review]
root left overs

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

r=bholley contingent on removing the rooting stuff from the exception arg parsing.

::: js/xpconnect/src/XPCComponents.cpp
@@ +1899,5 @@
> +        if (argc > 3) {
> +            RootedValue data(cx, argv[3]);
> +            if (!parseData(data))
> +                return false;
> +        }

This is really gross. Can't we use CallArgs here and get automatic rooting?

Comment 14

[Tom S [:evilpie]]

So did somebody have a good idea how to go about comment 13? CallArgs is not an option, because the function calling it is not a real JSNative.

Comment 15

[Bobby Holley (:bholley)]

Do we have some way to have handle/rooted versions of an array of jsvals?

Comment 16

[Terrence Cole [:terrence]]

(In reply to Bobby Holley (:bholley) from comment #15)
> Do we have some way to have handle/rooted versions of an array of jsvals?

Not at the moment. We've been advising people to use AutoValueArray in these cases.

In this case, we should probably do just that: root the value buffer with an AutoValueArray and then wrap a CallArgs around it.

Comment 17

[Tom S [:evilpie]]

For now I just punted on the rooting thing. Going to file a follow up to fix this proper.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0ce90f9f19

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf0ce90f9f19

Comment 19

[Tom S [:evilpie]]

Jon is going to fix all remaining problems. Filed a follow up as well.