1243824 - Support static functions/attributes on JSXrays (Xrays to JS objects)

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We need this for Promise.  Otherwise rivers of flame will flow down the tree.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Add support for static functions and attributes on JSXrays

Note that there are some XXX comments that I would love answers to!

Comment 2

[Bobby Holley (:bholley)]

Sorry I haven't gotten to this. I'll try to tomorrow morning.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I just realized I can use js::ProtoKeyToClass here to make things a bit simpler.  Updated patch coming up.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Add support for static functions and attributes on JSXrays

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8715880 [details] [diff] [review]
Add support for static functions and attributes on JSXrays

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

r=me with comments addressed.

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +294,5 @@
> +  function testCtorCallables(ctorCallables, xrayCtor, localCtor) {
> +    for (let name of ctorCallables) {
> +      // Don't try to test Function.prototype, since that is in fact a callable
> +      // but doesn't really do the things we expect callables to do here
> +      // (e.g. it's in the wrong global, since it gets Xrayed itself.

Nit: Missing ).

@@ +327,5 @@
> +      // Also don't try to do the return-value check on Regexp.lastMatch and
> +      // Regexp["$&"] (which are aliases), because they get state off the global
> +      // they live in, as far as I can tell, so testing them over Xrays will be
> +      // wrong: on the Xray they will actaully get the lastMatch of _our_
> +      // global, not the Xrayed one.

If the semantics of what we expose are wrong, I'd much rather just not expose any constructor properties on RegExp. Can we just do that in the Xray code, with a comment indicating that someone should sort out the semantics if they actually need it someday (which is very doubtful)?

@@ +337,5 @@
> +
> +        // If invoking this method returns something non-Xrayable, the
> +        // stringification is going to return [object Opaque]. Just
> +        // check for that case.
> +        if (!/Opaque/.test(method.call(xrayCtor))) {

Hm. What are the cases where we return something on-Xrayable? Seems like we just shouldn't expose the function in those cases.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +373,5 @@
>      return true;
>  }
>  
> +// Returns true on success (in the JSAPI sense), false on failure.  If true is
> +// returned, the "resolve" outparam will indicate whether we actually resolved

s/resolve/resolved/

@@ +383,5 @@
> +// fs is the JSFunctionSpec* from the relevant ClassSpec.
> +// ps is the JSPropertySpec* from the relevant ClassSpec.
> +// desc is the descriptor we're resolving into.
> +static bool
> +TryResolvePropertyFromClassSpec(JSContext* cx, HandleId id, HandleObject holder,

I would call this "TryResolvePropertyFromSpecs", since ClassSpecs have two different sets of function Specs, and that's exactly what we're passing here.

That also would clear up the somewhat-misleading comment of "fs is _the_ JSFunctionSpec* from the relevant ClassSpec", since there are two.

@@ +387,5 @@
> +TryResolvePropertyFromClassSpec(JSContext* cx, HandleId id, HandleObject holder,
> +                                const JSFunctionSpec* fs,
> +                                const JSPropertySpec* ps,
> +                                MutableHandle<JSPropertyDescriptor> desc,
> +                                bool* resolved)

I assume there were no subtle changes in this hoisting?

@@ +562,5 @@
> +                    // we've already returned.  But why is this bit not just at
> +                    // the top of this function, before we start doing all this
> +                    // stuff?  Is it just because this function only used to put
> +                    // things on the holder further down, in the prototype case,
> +                    // or is there an actual reason?

The main reason is that returning a property would be wrong in most of the non-prototype cases, since with Object, for example, it would obstruct our view into the dynamically-changing properties underneath.

In practice, it's ok because we never cache anything on the holder to begin with in those cases. So I'd be for moving the holder check to the top of the function, since that will ideally allow us to hoist the holder check into the superclass once we eliminate XPCWN Xrays.

@@ +582,5 @@
> +                        return false;
> +                    }
> +
> +                    // XXXbz Do we not need to change desc.object() from holder
> +                    // to wrapper?  Why not?

We could. In practice it probably doesn't matter, and this will hopefully go away. I just poked efaust about bug 1025338 a few weeks ago, and he promises that it's really coming this time. :-)

But I wouldn't object to being more correct in the mean time. Up to you.

@@ +673,5 @@
>  
> +    // Indexed array properties are handled above, so we can just work with the
> +    // class spec here.
> +    // XXXbz Do we not need to change desc.object() from holder
> +    // to wrapper if we define?  Why not?

See above.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Can we just do that in the Xray code

So just explicitly skip exposing statics for the regex jsproto value?

> Hm. What are the cases where we return something on-Xrayable?

No idea.  This code was already here in the prototype case and I just copied it.  I just checked, and I can remove this test for Opaque and the test still passes, so I guess I'll do that.  Good call.

> s/resolve/resolved/

OK.

> I would call this "TryResolvePropertyFromSpecs"

OK, done.  And adjusted comment a bit:

  // fs is the relevant JSFunctionSpec*.
  // ps is the relevant JSPropertySpec*.

> I assume there were no subtle changes in this hoisting?

Not intentional ones, certainly.  Except for not setting fs/ps in the loop headers, of course, since they're already set.

> So I'd be for moving the holder check to the top of the function

Yay, sounds good.  ;)

> But I wouldn't object to being more correct in the mean time. Up to you.

OK, done.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for the C++ changes

Comment 8

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> > Can we just do that in the Xray code
> 
> So just explicitly skip exposing statics for the regex jsproto value?

Yes.

> 
> > Hm. What are the cases where we return something on-Xrayable?
> 
> No idea.  This code was already here in the prototype case and I just copied
> it.  I just checked, and I can remove this test for Opaque and the test
> still passes, so I guess I'll do that.  Good call.

Great!

> 
> > s/resolve/resolved/
> 
> OK.
> 
> > I would call this "TryResolvePropertyFromSpecs"
> 
> OK, done.  And adjusted comment a bit:
> 
>   // fs is the relevant JSFunctionSpec*.
>   // ps is the relevant JSPropertySpec*.
> 
> > I assume there were no subtle changes in this hoisting?
> 
> Not intentional ones, certainly.  Except for not setting fs/ps in the loop
> headers, of course, since they're already set.
> 
> > So I'd be for moving the holder check to the top of the function
> 
> Yay, sounds good.  ;)
> 
> > But I wouldn't object to being more correct in the mean time. Up to you.
> 
> OK, done.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Yes.

OK.  Just going to wrap a standardConstructor != JSProto_RegExp around the two places I added stuff for constructors based on JSClass.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> > Yes.
> 
> OK.  Just going to wrap a standardConstructor != JSProto_RegExp around the
> two places I added stuff for constructors based on JSClass.

If you haven't landed already, please add a commented constexpr function like ShouldResolveStaticMethods rather than hard-coding it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Still doing the try run.  Will add the function.  Just a static inside XrayWrapper.cpp?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> Still doing the try run.  Will add the function.  Just a static inside
> XrayWrapper.cpp?

Yep, SGTM.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/984b9d0ea04d

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/984b9d0ea04d