Implement Xrays to Function objects

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Part of Xray-to-JS. This may get pretty involved.
(Assignee)

Updated

5 years ago
Depends on: 976151
(Assignee)

Updated

5 years ago
Blocks: 987678
(Assignee)

Updated

5 years ago
Assignee: nobody → bobbyholley
(Assignee)

Comment 2

5 years ago
Created attachment 8441577 [details] [diff] [review]
Part 1 - Add a mechanism to identify a standard constructor. v1
Attachment #8441577 - Flags: review?(luke)
(Assignee)

Comment 3

5 years ago
Created attachment 8441578 [details] [diff] [review]
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.
Attachment #8441578 - Flags: review?(luke)
(Assignee)

Comment 4

5 years ago
Created attachment 8441603 [details] [diff] [review]
Part 3 - Implement Xrays to Function objects. v1
Attachment #8441603 - Flags: review?(gkrizsanits)
(Assignee)

Comment 5

5 years ago
Created attachment 8441604 [details] [diff] [review]
Part 4 - Support the 'prototype' property for standard constructors. v1
Attachment #8441604 - Flags: review?(gkrizsanits)
(Assignee)

Comment 6

5 years ago
Created attachment 8441605 [details] [diff] [review]
Part 5 - Support the .name and .length properties of Function instances. v1
Attachment #8441605 - Flags: review?(gkrizsanits)
(Assignee)

Comment 7

5 years ago
Created attachment 8441607 [details] [diff] [review]
Part 6 - Tests. v1
Attachment #8441607 - Flags: review?(gkrizsanits)
(Assignee)

Comment 8

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

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

Comment 9

5 years ago
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

5 years ago
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 11

5 years ago
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+
(Assignee)

Comment 12

5 years ago
Created attachment 8441665 [details] [diff] [review]
Part 5 - Support the .name and .length properties of Function instances. v2
Attachment #8441578 - Attachment is obsolete: true
Attachment #8441665 - Flags: review?(gkrizsanits)
(Assignee)

Comment 13

5 years ago
Note - part 2 is now obsolete thanks to JS_GetFunctionArity and JS_GetFunctionId.
(Assignee)

Comment 14

5 years ago
Final all-platform try push: https://tbpl.mozilla.org/?tree=Try&rev=728f17338811
Attachment #8441605 - Attachment is obsolete: true
Attachment #8441605 - Flags: review?(gkrizsanits)
(Assignee)

Comment 15

5 years ago
(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+
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Updated

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