854955 - GC: Rooting in XrayWrapper.cpp

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jon Coppeard (:jonco)]

Fix some rooting hazards in XrayWrapper.cpp.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Proposed changes

This does some of the easier rooting.  XPCCallContext needs rooting and doesn't always have a JSContext, so that's one I'm currently thinking about.  Also it looks like we need to expose the PropertyDescriptor auto rooter outside SM.

Comment 2

[Andrew McCreight [:mccr8]]

bholley should probably review this too.

Comment 3

[Terrence Cole [:terrence]]

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

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

For now we are going to try to not use the Handle and Rooted typedefs outside SpiderMonkey proper. For internal use it makes sense to teach everyone new vocabulary to save a few characters. For outside users, however, we'd like to make it clear that Rooted<> and Handle<> modify an existing type and keep these classes easy to search for. Sorry I was not clear enough about that decision.

Otherwise it looks fine to me, but I'd like us to try to get browser people to review these as much as possible. Andrew (:mccr8) said he'd be willing to direct patches to where they need to go, so flag him for review on the next pass and he will direct you to the best reviewer.

Comment 4

[Andrew McCreight [:mccr8]]

> so flag him for review on the next pass and he will direct you to the best reviewer
For XrayWrapper, bholley, as I said in comment 2. ;)

Comment 5

[Terrence Cole [:terrence]]

> > so flag him for review on the next pass and he will direct you to the best reviewer
> For XrayWrapper, bholley, as I said in comment 2. ;)
We mid-aired. You're fast on the draw! :-)

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Proposed changes

Fix for rooting issues in XrayWrapper.cpp

Comment 7

[Bobby Holley (:bholley)]

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

(In reply to Terrence Cole [:terrence] from comment #3)
> For now we are going to try to not use the Handle and Rooted typedefs
> outside SpiderMonkey proper. For internal use it makes sense to teach
> everyone new vocabulary to save a few characters. For outside users,
> however, we'd like to make it clear that Rooted<> and Handle<> modify an
> existing type and keep these classes easy to search for. Sorry I was not
> clear enough about that decision.

I don't agree here. Objects, ids, and values are all over XPConnect. Clearly the JS folks recognize the value of syntactic sugar here, and I don't buy the argument that JSAPI consumers need to type more characters to avoid getting confused. Just looking at the patch here, all the extra <>'s in the argument declarations make me cry.

More bluntly, I don't think you want to type Handle<JSObject *> everywhere in your module, and I don't want to in mine either. ;-)

Comment 8

[Jeff Walden [:Waldo]]

Note that there is dissension in the ranks about using the hiding-information typedefs.  I think we shouldn't have the typedefs and should only use Handle<*> and Rooted<*>, and I think it's totally reasonable to have to type a few extra characters to do so.

Comment 9

[Jim Blandy :jimb]

(In reply to Bobby Holley (:bholley) from comment #7)
> I don't agree here. Objects, ids, and values are all over XPConnect. Clearly
> the JS folks recognize the value of syntactic sugar here, and I don't buy
> the argument that JSAPI consumers need to type more characters to avoid
> getting confused. Just looking at the patch here, all the extra <>'s in the
> argument declarations make me cry.
> 
> More bluntly, I don't think you want to type Handle<JSObject *> everywhere
> in your module, and I don't want to in mine either. ;-)

First off, I don't believe we really understand what makes things legible and illegible. So while I disagree with you, my justifications feel more to me like post-facto excuses than solid reasons. And, we have a module owner system to address exactly these sorts of disagreements.

That said:

When I see HandleObject, I have to wonder whether it's a typedef for what I'm expecting, or just something related, with special changes and exceptions. When I see Handle<JSObject *> and Handle<jsval>, I know exactly what those two have in common, and I know that Handle must be something with interfaces that aren't specific to the referent.

In other words, when I see a name, I need to look up its meaning, which is an additional step over simply writing out its meaning.

I really don't think non-letters vs. letters has a lot of influence on legibility.

So, yes, we do want to type Handle<JSObject *> everywhere in our module. Or, at least, I do, and I think the JS team settled on that as preferable, in their own code as well as outside.

Oh dear. I've been tempted by the bikeshed discussion. :(

Comment 10

[Terrence Cole [:terrence]]

> (In reply to Bobby Holley (:bholley) from comment #7)
> More bluntly, I don't think you want to type Handle<JSObject *> everywhere
> in your module, and I don't want to in mine either. ;-)

Good point! :-)

This also follows the XPConnect uses SpiderMonkey's style guideline. Would it be okay to also |using namespace JS;| and kill the JS:: everywhere.

(In reply to Jim Blandy :jimb from comment #9)
> (In reply to Bobby Holley (:bholley) from comment #7)
> > More bluntly, I don't think you want to type Handle<JSObject *> everywhere
> > in your module, and I don't want to in mine either. ;-)
> ... snip ...
> 
> So, yes, we do want to type Handle<JSObject *> everywhere in our module. Or,
> at least, I do, and I think the JS team settled on that as preferable, in
> their own code as well as outside.

/me sighs
 
> Oh dear. I've been tempted by the bikeshed discussion. :(

"Tempted" huh? I've been /dreading/ this inevitable discussion for most of a year now.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Terrence Cole [:terrence] from comment #10)

> This also follows the XPConnect uses SpiderMonkey's style guideline. Would
> it be okay to also |using namespace JS;| and kill the JS:: everywhere.

That's fine by me.


> > So, yes, we do want to type Handle<JSObject *> everywhere in our module. Or,
> > at least, I do, and I think the JS team settled on that as preferable, in
> > their own code as well as outside.
> 
> /me sighs

If the JS team decides to eliminate the typedefs within js/src, I'll defer to that. I mostly want to ensure that we're eating the same syntactic dogfood for this stuff, especially since, as you note, we use js-style in XPConnect. :-)

Comment 12

[Jon Coppeard (:jonco)]

Attachment: Proposed changes

Ok, here's an updated patch using the typedefs and the JS namespace.

Comment 13

[Bobby Holley (:bholley)]

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

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

r=bholley with comments addressed.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +28,5 @@
>  using namespace mozilla::dom;
>  
>  namespace xpc {
>  
> +using namespace JS;

This should probably go at the top with |using namespace mozilla::dom;|

@@ +184,5 @@
> +    virtual bool resolveOwnProperty(JSContext *cx, js::Wrapper &jsWrapper, HandleObject wrapper,
> +                                    HandleObject holder, HandleId id,
> +                                    JSPropertyDescriptor *desc, unsigned flags);
> +    static bool defineProperty(JSContext *cx, HandleObject wrapper,
> +                               HandleId,

This should probably have a name.

@@ +185,5 @@
> +                                    HandleObject holder, HandleId id,
> +                                    JSPropertyDescriptor *desc, unsigned flags);
> +    static bool defineProperty(JSContext *cx, HandleObject wrapper,
> +                               HandleId,
> +                               js::PropertyDescriptor *desc,

I'd prefer to add |using js::PropertyDescriptor| (and js::Wrapper) at the top of the file.

@@ +370,4 @@
>  {
>      // The expando object lives in the compartment of the target, so all our
>      // work needs to happen there.
> +    RootedObject exclusiveGlobal(cx, exclusiveGlobal_);

It seems like this should be rooted at the callsite, no?

@@ +402,2 @@
>  {
> +    RootedObject target(cx, target_);

same as above

@@ +449,5 @@
>          // anyone else, so we tag it with the sandbox global.
>          //
>          // NB: We first need to check the class, _then_ wrap for the target's
>          // compartment.
> +        RootedObject consumerGlobal(cx, js::GetGlobalForObjectCrossCompartment(wrapper));

It's not a problem to do |RootedObject r(cx, obj);| when |cx->compartment != obj->compartment()|, right?

@@ +456,3 @@
>              return NULL;
>          expandoObject = attachExpandoObject(cx, target, ObjectPrincipal(wrapper),
> +                                            isSandbox ? (HandleObject)consumerGlobal : NullPtr());

Can you explain why this cast is necessary?

@@ +465,3 @@
>  {
> +    RootedObject dst(cx, dst_);
> +    RootedObject src(cx, src_);

Shouldn't these be rooted at the callsite? I understand that you might not want to dig all the way into the browser, but it seems like XrayUtils::CloneExpandoChain would be a good choke point.

@@ +510,2 @@
>  {
> +    RootedObject wrapper(cx, wrapper_);

Root at callsite?

@@ +558,5 @@
>  // Some DOM objects have shared properties that don't have an explicit
>  // getter/setter and rely on the class getter/setter. We install a
>  // class getter/setter on the holder object to trigger them.
>  JSBool
> +holder_get(JSContext *cx, HandleObject wrapper_, HandleId id, MutableHandleValue vp)

The underscore can die here.

@@ +582,5 @@
>      return true;
>  }
>  
>  JSBool
> +holder_set(JSContext *cx, HandleObject wrapper_, HandleId id, JSBool strict, MutableHandleValue vp)

same

@@ +1547,5 @@
>      if (!desc->obj &&
>          EnsureCompartmentPrivate(wrapper)->scope->IsXBLScope() &&
>          (content = do_QueryInterfaceNative(cx, wrapper)))
>      {
> +        RootedId id_(cx, id);

This is unnecessary now, no?

::: js/xpconnect/wrappers/XrayWrapper.h
@@ +32,5 @@
>  
>  bool CloneExpandoChain(JSContext *cx, JSObject *src, JSObject *dst);
>  
>  bool
> +IsTransparent(JSContext *cx, JS::Handle<JSObject *> wrapper, JS::Handle<jsid> id);

Use the typedefs here,

@@ +38,5 @@
>  JSObject *
>  GetNativePropertiesObject(JSContext *cx, JSObject *wrapper);
>  
>  bool
> +IsXrayResolving(JSContext *cx, JS::Handle<JSObject *> wrapper, JS::Handle<jsid> id);

and here.

Comment 14

[Jon Coppeard (:jonco)]

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

Thanks for the detailed review!

@@ +370,4 @@
>> +    RootedObject exclusiveGlobal(cx, exclusiveGlobal_);
>
> It seems like this should be rooted at the callsite, no?

We still need a root here because it's modified by calling JS_WrapObject().

> It's not a problem to do |RootedObject r(cx, obj);| when |cx->compartment != obj->compartment()|, right?

As I understand it, the context is just used as a place to store a pointer to the top of a stack of roots.  When GC happens all roots are marked, regardless of compartment.

@@ +456,3 @@
>>          expandoObject = attachExpandoObject(cx, target, ObjectPrincipal(wrapper),
>> +                                            isSandbox ? (HandleObject)consumerGlobal : NullPtr());
>
> Can you explain why this cast is necessary?

The types of the second and third operands to the conditional operator need to be the same or one needs to be convertible to the other.  Casting the RootedObject to a HandleObject achieves this, as HandleObject is constructible from NullPtr.

@@ +510,2 @@
>>  {
> +    RootedObject wrapper(cx, wrapper_);
>
> Root at callsite?

The thing that |wrapper| points to is updated in the loop so there needs to be a root here.

Other comments addressed.

Comment 15

[Jon Coppeard (:jonco)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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