Closed Bug 976148 Opened 6 years ago Closed 5 years ago

Implement Xrays to Function objects

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

Part of Xray-to-JS. This may get pretty involved.
Depends on: 976151
Blocks: 987678
Assignee: nobody → bobbyholley
Attaching the patches that need to be reviewed by luke. The rest are coming
in a couple of minutes.
Attachment #8441578 - Flags: review?(luke)
Attachment #8441603 - Flags: review?(gkrizsanits)
Attachment #8441607 - Flags: review?(gkrizsanits)
This is the last part of our Q2 goal. \o/

https://tbpl.mozilla.org/?tree=Try&rev=673316e5f6ae
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 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.
Attachment #8441578 - Flags: review?(luke) → review+
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.
Attachment #8441577 - Flags: review?(luke) → review+
Attachment #8441578 - Attachment is obsolete: true
Attachment #8441665 - Flags: review?(gkrizsanits)
Note - part 2 is now obsolete thanks to JS_GetFunctionArity and JS_GetFunctionId.
Attachment #8441605 - Attachment is obsolete: true
Attachment #8441605 - Flags: review?(gkrizsanits)
(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 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.
Attachment #8441603 - Flags: review?(gkrizsanits) → review+
Attachment #8441604 - Flags: review?(gkrizsanits) → review+
Attachment #8441607 - Flags: review?(gkrizsanits) → review+
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 ||
Attachment #8441665 - Flags: review?(gkrizsanits) → review+
(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
(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.
Blocks: 856067
You need to log in before you can comment on or make changes to this bug.