860311 - GC: More rooting in XPConnect

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jon Coppeard (:jonco)]

Address more rooting hazards in XPConnect.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Proposed changes

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Proposed changes

Here's a patch for some more rooting issues, mainly in XPCWrappedNative.cpp.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 736227 [details] [diff] [review]
Proposed changes

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

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +735,5 @@
>  
>  // static
>  nsresult
>  XPCWrappedNative::Morph(XPCCallContext& ccx,
> +                        JS::HandleObject existingJSObject,

using namespace JS, and kill the qualifiers.

@@ +1747,5 @@
>  // orphan.
>  nsresult
>  XPCWrappedNative::RescueOrphans(XPCCallContext& ccx)
>  {
> +    JS::RootedObject flatJSObject(ccx, mFlatJSObject);

mFlatJSObject is a heap pointer that should have its own rooted. Should we use fromMarkedLocation here or something? The performance of this function doesn't really matter, but I'm wondering what we're doing for stuff like mFlatJSObject in general.

@@ +2438,5 @@
>              continue;
>  
>          const nsXPTType& type = paramInfo.GetType();
>          nsXPTCVariant* dp = GetDispatchParam(i);
> +        JS::RootedValue v(mCallContext, JSVAL_NULL);

NullValue() here while you're at it.

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp
@@ +29,5 @@
>  }
>  
>  JSBool
>  XPCNativeMember::NewFunctionObject(XPCCallContext& ccx,
> +                                   XPCNativeInterface* iface, JS::HandleObject parent,

using namespace JS, and kill all the qualifications in this file.

@@ +288,5 @@
>              NS_ERROR("bad method name");
>              failed = true;
>              break;
>          }
> +        jsid name = INTERNED_STRING_TO_JSID(ccx, str);

So these don't need to be rooted because they're interned, right? Is there some better way to communicate that this has been audited, like RawId or something?

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +139,5 @@
>  
>  static JSObject*
>  GetDoubleWrappedJSObject(XPCCallContext& ccx, XPCWrappedNative* wrapper)
>  {
> +    JS::RootedObject obj(ccx);

using namespace JS and kill all the qualified names in this file

@@ +232,5 @@
>  
>  static JSBool
>  DefinePropertyIfFound(XPCCallContext& ccx,
> +                      JS::HandleObject obj,
> +                      JS::HandleId id_,

Is the a convention going forward about id_ vs idArg?

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Bobby Holley (:bholley) from comment #3)

Thanks for the review.

> > +    JS::RootedObject flatJSObject(ccx, mFlatJSObject);
> 
> mFlatJSObject is a heap pointer that should have its own rooted. Should we
> use fromMarkedLocation here or something? The performance of this function
> doesn't really matter, but I'm wondering what we're doing for stuff like
> mFlatJSObject in general.

You're right, I think we can use fromMarkedLocation() for things which are on the heap.  I haven't seen many of these so far though.

> @@ +288,5 @@
> >              NS_ERROR("bad method name");
> >              failed = true;
> >              break;
> >          }
> > +        jsid name = INTERNED_STRING_TO_JSID(ccx, str);
> 
> So these don't need to be rooted because they're interned, right? 

name here is now not live over a call that can GC.  That said, I don't think we're going to be moving strings when we GC anyway.

> Is there
> some better way to communicate that this has been audited, like RawId or
> something?

Yes, there are RawT typedefs for we can use for things that do not need to be rooted.  But marking things safe like this doesn't stop the code being changed to make them unsafe, in which case it can just lead to a false sense of security.  So I'm not sure it buys us that much.

> Is the a convention going forward about id_ vs idArg?

In the JS engine we're using arg_ for arguments which are immediately rooted.  I like this because it's ugly enough that seeing an arg_ in use in the body of the function immediately stands out as wrong.  Let me know what you think.

I'll add "using namespace JS" to the patches in bug 860777 as well.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Jon Coppeard (:jonco) from comment #4)
> In the JS engine we're using arg_ for arguments which are immediately
> rooted.  I like this because it's ugly enough that seeing an arg_ in use in
> the body of the function immediately stands out as wrong.  Let me know what
> you think.

I'm fine with either. Just coordinate with evilepies to ensure consistency, since he seems to be doing the fooArg approach.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #4)
> (In reply to Bobby Holley (:bholley) from comment #3)
> > > +    JS::RootedObject flatJSObject(ccx, mFlatJSObject);
> > 
> > mFlatJSObject is a heap pointer that should have its own rooted. Should we
> > use fromMarkedLocation here or something? The performance of this function
> > doesn't really matter, but I'm wondering what we're doing for stuff like
> > mFlatJSObject in general.
> 
> You're right, I think we can use fromMarkedLocation() for things which are
> on the heap.  I haven't seen many of these so far though.

We're going to need a JS:: HeapPtr equivalent for adding post-barriers to the browser. We could easily make such a class implicitly convertible to Handle, which would save on typing.
 
> > @@ +288,5 @@
> > >              NS_ERROR("bad method name");
> > >              failed = true;
> > >              break;
> > >          }
> > > +        jsid name = INTERNED_STRING_TO_JSID(ccx, str);
> > 
> > So these don't need to be rooted because they're interned, right? 
> 
> name here is now not live over a call that can GC.  That said, I don't think
> we're going to be moving strings when we GC anyway.

It will be awhile until we can move strings, but this is an eventual goal: manipulation of small strings is something that needs to be more efficient. I'm a bit dubious that we'll be able to move atoms, however, because of the "ptr equality" = "string equality" invariant that we have right now.
 
> > Is there
> > some better way to communicate that this has been audited, like RawId or
> > something?
> 
> Yes, there are RawT typedefs for we can use for things that do not need to
> be rooted.  But marking things safe like this doesn't stop the code being
> changed to make them unsafe, in which case it can just lead to a false sense
> of security.  So I'm not sure it buys us that much.

We've moved away from using Raw, because it introduces a 4th type that people have to know about. I think we should probably try to delete Raw eventually, but it's not a super-important goal.
 
> > Is the a convention going forward about id_ vs idArg?
> 
> In the JS engine we're using arg_ for arguments which are immediately
> rooted.  I like this because it's ugly enough that seeing an arg_ in use in
> the body of the function immediately stands out as wrong.  Let me know what
> you think.

Actually, a clear consensus of SpiderMonkey developers decided on idArg after debate. This was right after Brian landed the initial exact rooting patch with id_; some of those are still not fully cleaned up. For what it's worth, I was also on the side of id_, but that's life.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #6)
> Actually, a clear consensus of SpiderMonkey developers decided on idArg
> after debate. 

Ah right, I missed that.  idArg it is then!

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2344b2249db

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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