Implement Xray support for selfHosted functions

RESOLVED FIXED in mozilla32

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug)

unspecified
mozilla32
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

5 years ago
Blocks: 914970
(Assignee)

Comment 1

4 years ago
Created attachment 8432124 [details] [diff] [review]
Implement Xrays to self-hosted methods and properties. v1
Attachment #8432124 - Flags: review?(till)
Attachment #8432124 - Flags: review?(gkrizsanits)
(Assignee)

Comment 2

4 years ago
Lots of array functions are self-hosted, so let's make sure to support them before landing Xrays to Arrays.
Blocks: 987163
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.
Attachment #8432124 - Flags: review?(till) → review+
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.
Attachment #8432124 - Flags: review?(gkrizsanits) → review+
(Assignee)

Comment 5

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/39034630898e
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(Assignee)

Updated

4 years ago
No longer blocks: 987163
(Assignee)

Updated

4 years ago
Blocks: 1020609
You need to log in before you can comment on or make changes to this bug.