976148 - Implement Xrays to Function objects

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Part of Xray-to-JS. This may get pretty involved.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add a mechanism to identify a standard constructor. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add jsfriendapi accessors for function name and length. v1

Attaching the patches that need to be reviewed by luke. The rest are coming
in a couple of minutes.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement Xrays to Function objects. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Support the 'prototype' property for standard constructors. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Support the .name and .length properties of Function instances. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 6 - Tests. v1

Comment 8

[Bobby Holley (:bholley)]

This is the last part of our Q2 goal. \o/

https://tbpl.mozilla.org/?tree=Try&rev=673316e5f6ae

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8441577 [details] [diff] [review]
Part 1 - Add a mechanism to identify a standard constructor. v1

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

::: js/src/jsobj.cpp
@@ +3451,5 @@
> +
> +    GlobalObject &global = obj->global();
> +    for (size_t k = 0; k < JSProto_LIMIT; ++k) {
> +        JSProtoKey key = static_cast<JSProtoKey>(k);
> +        if (global.getConstructor(key) == ObjectValue(*obj))

Could you use JSCLASS_CACHED_PROTO_KEY?  You'd still need to test the identity of the constructor function against global.getConstructor(key), but at least that way you know the key and don't need an O(n) search.  I'm not sure if every standard constructor's class JSCLASS_HAS_CACHED_PROTO, but it seems like you could assert this in initBuiltinConstructor() and fix the stragglers.

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8441578 [details] [diff] [review]
Part 2 - Add jsfriendapi accessors for function name and length. v1

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

::: js/src/jsfriendapi.cpp
@@ +553,5 @@
>  
> +JS_FRIEND_API(size_t)
> +js::FunctionLength(JSObject *fun)
> +{
> +    return fun->as<JSFunction>().nargs();

I think you can use:
  JS_GetFunctionArity(JS_GetObjectFunction(fun))
instead of adding this.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8441577 [details] [diff] [review]
Part 1 - Add a mechanism to identify a standard constructor. v1

Ah, I forgot that these Class here is just the Function Class, not the Class of the constructed objects, so there is no CACHED_PROTO.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 5 - Support the .name and .length properties of Function instances. v2

Comment 13

[Bobby Holley (:bholley)]

Note - part 2 is now obsolete thanks to JS_GetFunctionArity and JS_GetFunctionId.

Comment 14

[Bobby Holley (:bholley)]

Final all-platform try push: https://tbpl.mozilla.org/?tree=Try&rev=728f17338811

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #14)
> Final all-platform try push:
> https://tbpl.mozilla.org/?tree=Try&rev=728f17338811

Something odd happened with Windows Opt xpcshell, but thankfully it went away with a fresh push: https://tbpl.mozilla.org/?tree=Try&rev=d00179b9b23c

This is now green. Once it's reviewed, we can land it, and mark that Q2 goal as done.

Comment 16

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8441603 [details] [diff] [review]
Part 3 - Implement Xrays to Function objects. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +347,5 @@
>  
>      static bool call(JSContext *cx, HandleObject wrapper,
>                       const JS::CallArgs &args, js::Wrapper& baseInstance)
>      {
> +        auto self = JSXrayTraits::singleton;

https://i.imgflip.com/9not0.jpg
Seriously though for this single ensureHolder call I don't think you should use anything fancy. Do we have an actual policy for the usage of all the new C++ features? If I missed out something on this topic feel free to ignore my opinion here.

Comment 17

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8441665 [details] [diff] [review]
Part 5 - Support the .name and .length properties of Function instances. v2

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +846,5 @@
>                  props.infallibleAppend(INT_TO_JSID(i));
>          } else if (key == JSProto_Function) {
> +            if (!props.append(GetRTIdByIndex(cx, XPCJSRuntime::IDX_LENGTH)))
> +                return false;
> +            if (!props.append(GetRTIdByIndex(cx, XPCJSRuntime::IDX_NAME)))

NIT: you could merge these two ifs with ||

Comment 18

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17)
> >          } else if (key == JSProto_Function) {
> > +            if (!props.append(GetRTIdByIndex(cx, XPCJSRuntime::IDX_LENGTH)))
> > +                return false;
> > +            if (!props.append(GetRTIdByIndex(cx, XPCJSRuntime::IDX_NAME)))
> 
> NIT: you could merge these two ifs with ||

Yeah, but with the required bracing style for multi-line conditionals it ends up longer that way.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/51ab4d67ca8a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fa086a70cfbf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a471415834ae
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/206d7f502e14
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/871d570bc7c9

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #18)
> Yeah, but with the required bracing style for multi-line conditionals it
> ends up longer that way.

Ah, OK then.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51ab4d67ca8a
https://hg.mozilla.org/mozilla-central/rev/fa086a70cfbf
https://hg.mozilla.org/mozilla-central/rev/a471415834ae
https://hg.mozilla.org/mozilla-central/rev/206d7f502e14
https://hg.mozilla.org/mozilla-central/rev/871d570bc7c9