Closed Bug 854955 Opened 9 years ago Closed 9 years ago

GC: Rooting in XrayWrapper.cpp

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file, 2 obsolete files)

Fix some rooting hazards in XrayWrapper.cpp.
Attached patch Proposed changes (obsolete) — Splinter Review
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.
Attachment #729670 - Flags: review?(terrence)
bholley should probably review this too.
Component: JavaScript Engine → XPConnect
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.
Attachment #729670 - Flags: review?(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. ;)
> > 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! :-)
Attached patch Proposed changes (obsolete) — Splinter Review
Fix for rooting issues in XrayWrapper.cpp
Attachment #729670 - Attachment is obsolete: true
Attachment #730130 - Flags: review?(bobbyholley+bmo)
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. ;-)
Attachment #730130 - Flags: review?(bobbyholley+bmo) → review-
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.
(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. :(
> (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.
(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. :-)
Attached patch Proposed changesSplinter Review
Ok, here's an updated patch using the typedefs and the JS namespace.
Attachment #730130 - Attachment is obsolete: true
Attachment #730627 - Flags: review?(bobbyholley+bmo)
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.
Attachment #730627 - Flags: review?(bobbyholley+bmo) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/b4f217369d8a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.