861887 - GC: Rooting in XrayWrapper.cpp

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Current work

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Part 1 - addtions to jsapi.h

So to fix the reported hazards in XrayWrapper.cpp I had to add a few things to the rooting base clases:

 - uncheckedGetter() and uncheckedSetter(), because the getter and setter are passed to JS_DefinePropertyById as JSPropertyOp/JSStrictPropertyOp whether they are those types or really objects.

 - I added value() and obj() methods to the handle base class that return Handles rather than MutableHandles, otherwise calling these on a non-const Handle attempts to generate MutableHandles which doesn't work.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Part 2 - rooting in XrayWrapper.cpp

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 739994 [details] [diff] [review]
Part 1 - addtions to jsapi.h

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

Thanks!

::: js/src/jsapi.h
@@ +3371,5 @@
>          MOZ_ASSERT(hasSetterObject());
>          return JS::HandleObject::fromMarkedLocation(reinterpret_cast<JSObject *const *>(&desc()->setter));
>      }
> +    JSPropertyOp uncheckedGetter() const { return desc()->getter; }
> +    JSStrictPropertyOp uncheckedSetter() const { return desc()->setter; }

I hit another place inside the engine that wanted these as well, but none that needed the other. Just strip the assertions from getter() and setter().

@@ +3415,5 @@
>  template <>
>  class HandleBase<JSPropertyDescriptor>
>    : public JS::PropertyDescriptorOperations<JS::Handle<JSPropertyDescriptor> >
>  {
>      friend class JS::PropertyDescriptorOperations<JS::Rooted<JSPropertyDescriptor> >;

I made a copy-paste-o when creating these: we should be friending Handle, not Rooted here. Also in the MutableHandle variant. Please fix these inline.

@@ +3421,5 @@
>          return static_cast<const JS::Handle<JSPropertyDescriptor>*>(this)->address();
>      }
> +  public:
> +    JS::HandleValue value() const { return JS::HandleValue::fromMarkedLocation(&extract()->value); }
> +    JS::HandleObject obj() const { return JS::HandleObject::fromMarkedLocation(&extract()->obj); }

Ah, this is a good idea. When I hit this problem I added:
    
const JS::HandleObject object() const { return JS::HandleObject::fromMarkedLocation(&desc()->obj); }

to PropertyDescriptorOperations. This, however, required make all of the Handles const, which was awkward. I think I like this solution better.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Part 2 - rooting in XrayWrapper.cpp

Address rooting hazards in XrayWrapper.cpp

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 740809 [details] [diff] [review]
Part 2 - rooting in XrayWrapper.cpp

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

This may need to be rebased slightly on some Xray changes I just pushed.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +186,5 @@
>                                      HandleObject holder, HandleId id,
>                                      JSPropertyDescriptor *desc, unsigned flags);
>      static bool defineProperty(JSContext *cx, HandleObject wrapper, HandleId id,
> +                               PropertyDescriptor *desc,
> +                               Handle<PropertyDescriptor> existingDesc, bool *defined);

Don't we also need some sort of Handle to |desc| here?

@@ +1203,3 @@
>          return true;
>  
> +    JSObject *obj = getTargetObject(wrapper);

Why isn't this rooted? Even if the static analysis decides it's not a GC hazard, I don't really want to have that kind of arbitrary stuff in the code. It seems like we should just always used Handle/Rooted.

@@ +1631,3 @@
>          return false;
>  
> +    if (existing_desc.object() && (existing_desc.get().attrs & JSPROP_PERMANENT))

don't we have a convenience method for attrs?

::: js/xpconnect/wrappers/XrayWrapper.h
@@ +179,5 @@
>      friend class AutoSetWrapperNotShadowing;
>      friend class XPCWrappedNativeXrayTraits;
>  
> +    JS::RootedId mId;
> +    JS::RootedObject mHolder;

Why can't we just use Handles here?

Comment 7

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e96a99f7c5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/da6a56be2e05

Comment 8

[Jon Coppeard (:jonco)]

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

Ah, I missed your comments when landing this.  I will followup with another push.

> >      static bool defineProperty(JSContext *cx, HandleObject wrapper, HandleId id,
> > +                               PropertyDescriptor *desc,
> > +                               Handle<PropertyDescriptor> existingDesc, bool *defined);
> 
> Don't we also need some sort of Handle to |desc| here?

This is the PropertyDescriptor that is passed through the proxy API from the JS engine, and it is rooted internally.  It would be nice to change this to a handle, but I think it's worth waiting until we root the proxy API so we can do the whole thing in one go.

> > +    JSObject *obj = getTargetObject(wrapper);
> 
> Why isn't this rooted? Even if the static analysis decides it's not a GC
> hazard, I don't really want to have that kind of arbitrary stuff in the
> code. It seems like we should just always used Handle/Rooted.

Good idea, I've rooted a bunch of these.

> > +    if (existing_desc.object() && (existing_desc.get().attrs & JSPROP_PERMANENT))
> 
> don't we have a convenience method for attrs?

We do now, updated.

> > +    JS::RootedId mId;
> > +    JS::RootedObject mHolder;
> 
> Why can't we just use Handles here?

Updated mId to be a handle.  mHolder is not passed as a handle but comes from getHolderObject(), so needs a root.

Comment 9

[Jon Coppeard (:jonco)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e96a99f7c5d
https://hg.mozilla.org/mozilla-central/rev/da6a56be2e05
https://hg.mozilla.org/mozilla-central/rev/fb4ab26a49de