972987 - Implement Xray support for selfHosted functions

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I'm just punting on self-hosted functions in my current patches, but we'll need to support them at some point.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Implement Xrays to self-hosted methods and properties. v1

Comment 2

[Bobby Holley (:bholley)]

Lots of array functions are self-hosted, so let's make sure to support them before landing Xrays to Arrays.

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8432124 [details] [diff] [review]
Implement Xrays to self-hosted methods and properties. v1

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

Nice. r=me with the flags situation either cleaned up or an explanation of why you have to set the flags the way you do.

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +198,5 @@
>  
>    function testDate() {
>      // toGMTString is handled oddly in the engine. We don't bother to support
>      // it over Xrays.
> +    let propsToSkip = ['toGMTString'];

Hmm, what's the remaining issue with `toGMTString`? AFAICT, it's a straight-forward alias for `toUTCString`, which really isn't all that weird.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +587,5 @@
>          }
>      }
>      if (fsMatch) {
>          // Generate an Xrayed version of the method.
> +        Rooted<JSFunction*> fun(cx);

Nit: RootedFunction (or Rooted<JSFunction*> below to be consistent, your call.)

@@ +600,5 @@
>  
>          // The generic Xray machinery only defines non-own properties on the holder.
>          // This is broken, and will be fixed at some point, but for now we need to
>          // cache the value explicitly. See the corresponding call to
>          // JS_GetPropertyById at the top of this function.

Do we have a bug for this on file that could be mentioned here?

@@ +617,5 @@
>      }
>      if (psMatch) {
> +        desc.value().setUndefined();
> +        // Note that this is also kind of an abuse of JSPROP_NATIVE_ACCESSORS.
> +        // See bug 992977.

We should probably clean that up, but at least it'll assert and not survive even the most superficial testing.

@@ +625,5 @@
> +        if (flags & JSPROP_NATIVE_ACCESSORS) {
> +            desc.setGetter(psMatch->getter.propertyOp.op);
> +            desc.setSetter(psMatch->setter.propertyOp.op);
> +        } else {
> +            flags = (flags & ~JSPROP_NATIVE_ACCESSORS) | JSPROP_GETTER;

Flags can't have JSPROP_NATIVE_ACCESSORS set if we're in the else branch. And, see bug 958262 comment 8, must already have JSPROP_GETTER set. Can you also assert that here?

@@ +631,5 @@
> +            if (!getterObj)
> +                return false;
> +            desc.setGetterObject(JS_GetFunctionObject(getterObj));
> +            if (psMatch->setter.selfHosted.funname) {
> +                flags |= JSPROP_SETTER;

Same here, shouldn't this flag be set iff psMatch->setter.selfHosted.funname is non-NULL?

@@ +794,5 @@
>      for (const JSPropertySpec *ps = clasp->spec.prototypeProperties; ps && ps->name; ++ps) {
> +        // We have code to Xray self-hosted accessors. But at present, there don't appear
> +        // to be any self-hosted accessors anywhere in SpiderMonkey, let alone in on an
> +        // Xrayable class, so we can't test it. Assert against it to make sure that we get
> +        // test coverage in test_XrayToJS.xul when the time comes.

Yeah, they were introduced for SIMD, but that got refactored and doesn't use them anymore.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8432124 [details] [diff] [review]
Implement Xrays to self-hosted methods and properties. v1

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

Looks good.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Till Schneidereit [:till] from comment #3)
> ::: js/xpconnect/tests/chrome/test_xrayToJS.xul
> @@ +198,5 @@
> >  
> >    function testDate() {
> >      // toGMTString is handled oddly in the engine. We don't bother to support
> >      // it over Xrays.
> > +    let propsToSkip = ['toGMTString'];
> 
> Hmm, what's the remaining issue with `toGMTString`? AFAICT, it's a
> straight-forward alias for `toUTCString`, which really isn't all that weird.

Yes, but the alias is done manually inside SM as a property lookup + definition. We'd need to mirror that on the Xray side, which seemed like more trouble than it was worth at the time. If anybody complains about this I can fix it.

> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> Nit: RootedFunction (or Rooted<JSFunction*> below to be consistent, your
> call.)

Fixed.
 
> @@ +600,5 @@
> >  
> >          // The generic Xray machinery only defines non-own properties on the holder.
> >          // This is broken, and will be fixed at some point, but for now we need to
> >          // cache the value explicitly. See the corresponding call to
> >          // JS_GetPropertyById at the top of this function.
> 
> Do we have a bug for this on file that could be mentioned here?

It's blocked on various pieces of cleanup that's about to happen now that WebIDL window is done. It's all orthogonal to this patch, so I'm just going to leave it for now.

> @@ +617,5 @@
> >      }
> >      if (psMatch) {
> > +        desc.value().setUndefined();
> > +        // Note that this is also kind of an abuse of JSPROP_NATIVE_ACCESSORS.
> > +        // See bug 992977.
> 
> We should probably clean that up, but at least it'll assert and not survive
> even the most superficial testing.

Agreed. Any interest in taking bug 992977? ;-)
 
> @@ +625,5 @@
> > +        if (flags & JSPROP_NATIVE_ACCESSORS) {
> > +            desc.setGetter(psMatch->getter.propertyOp.op);
> > +            desc.setSetter(psMatch->setter.propertyOp.op);
> > +        } else {
> > +            flags = (flags & ~JSPROP_NATIVE_ACCESSORS) | JSPROP_GETTER;
> 
> Flags can't have JSPROP_NATIVE_ACCESSORS set if we're in the else branch.
> And, see bug 958262 comment 8, must already have JSPROP_GETTER set. Can you
> also assert that here?
...
> 
> Same here, shouldn't this flag be set iff psMatch->setter.selfHosted.funname
> is non-NULL?

Sounds good - I'll assert it.

Comment 6

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=0164cc646d5f

Comment 7

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39034630898e

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39034630898e