992958 - Give [Object] and [Function] a ClassSpec

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

We need the standard class information here to be declarative so that we can hook into it for Xrays.

Comment 1

[Bobby Holley (:bholley)]

(Object and Function are pretty inextricable)

Comment 2

[Bobby Holley (:bholley)]

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

This is green modulo a couple of failures that I've fixed. Uploading patches and flagging for review

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix test that depends on the order in which properties are enumerated. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop going through the __proto__ getter in getPrototypeOf. v1

The need for this is long-gone, I believe.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Streamline __proto__ getter and setter definitions into a JSPropertySpec. v1

We don't need the nativeCall bits anymore.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add support for prototype JSPropertySpecs on ClassSpec. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Rejigger the ordering of resolveConstructor to make it work for Object/Function. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 6 - Invoke set{Constructor,Prototype} as soon as possible, and remove setDetails helpers. v1

When we decouple function and object, we'll need to rely on these slots being
set up as soon as the relevant objects are created.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 7 - Hoist Object/Function prototype/constructor creation into ClassObjectCreationOps. v1

We try to keep the diff small for now, and reformat in the next patch.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 8 - Hoist extra logic in initFunctionAndObjectClasses into FinishClassInitOps. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 9 - Reindenting and trivial cleanup. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 10 - Switch Function and Object to ClassSpec. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 11 - Move ClassObjectCreationOps and FinishClassInitOps closer to their respective JSClasses. v1

This is a pure move, aside from adding a js:: namespace to a few NullPtrs.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8423556 [details] [diff] [review]
Part 2 - Stop going through the __proto__ getter in getPrototypeOf. v1

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

::: js/src/vm/GlobalObject.cpp
@@ -365,5 @@
>      {
>          return nullptr;
>      }
>  #endif /* JS_HAS_OBJ_PROTO_PROP */
> -    self->setProtoGetter(getter);

If you care about that, the definition of getter can now be inside the JS_HAS_OBJ_PROTO_PROP block.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add support for prototype JSPropertySpecs on ClassSpec. v2

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8423556 [details] [diff] [review]
Part 2 - Stop going through the __proto__ getter in getPrototypeOf. v1

Reassigning to Waldo; otherwise I'd just be rubberstamping.

Comment 17

[Luke Wagner [:luke]]

Comment on attachment 8423559 [details] [diff] [review]
Part 5 - Rejigger the ordering of resolveConstructor to make it work for Object/Function. v1

Yeah... actually all of these patches exceed my understanding.  Waldo did the last major refactoring of this area.

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8423563 [details] [diff] [review]
Part 9 - Reindenting and trivial cleanup. v1

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

I got this one.

::: js/src/vm/GlobalObject.cpp
@@ +141,5 @@
> +    JSObject *functionProto_ = NewObjectWithGivenProto(cx, &JSFunction::class_,
> +                                                       objectProto, self, SingletonObject);
> +    if (!functionProto_)
> +        return nullptr;
> +    RootedFunction functionProto(cx, &functionProto_->as<JSFunction>());

Preexisting in this area of the code: can you add \n after branches before the next line of code?  That's the usual SM style.

@@ +218,5 @@
> +    if (!ctor)
> +        return nullptr;
> +    RootedAtom objectAtom(cx, cx->names().Object);
> +    return NewFunction(cx, ctor, obj_construct, 1, JSFunction::NATIVE_CTOR, self,
> +                       objectAtom);

Preexisting, but I think you can pass cx->names().Object directly to NewFunction w/o the Rooted.  Same thing for functionAtom below.

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 8423559 [details] [diff] [review]
Part 5 - Rejigger the ordering of resolveConstructor to make it work for Object/Function. v1

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

::: js/src/vm/GlobalObject.cpp
@@ +391,5 @@
> +
> +    // Create the constructor.
> +    RootedObject ctor(cx, clasp->spec.createConstructor(cx, key));
> +    if (!ctor)
> +        return false;

Can you add \n after the conditions?

@@ +422,5 @@
>  
> +    // Set the property slot and stash type information, so that what we do
> +    // here is equivalent to initBuiltinConstructor.
> +    global->setConstructorPropertySlot(key, ObjectValue(*ctor));
> +    types::AddTypePropertyId(cx, global, id, ObjectValue(*ctor));

I'd rather this relationship was made explicit via a shared method called by both, unless there is something going to happen here in a later patch.

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 8423564 [details] [diff] [review]
Part 10 - Switch Function and Object to ClassSpec. v1

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

Bam

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 8423565 [details] [diff] [review]
Part 11 - Move ClassObjectCreationOps and FinishClassInitOps closer to their respective JSClasses. v1

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

You're right, that was all pretty direct slicing and dicing; sorry I missed it on the first look.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #18)
> Preexisting, but I think you can pass cx->names().Object directly to
> NewFunction w/o the Rooted.  Same thing for functionAtom below.

I get:

 0:02.20 In file included from /files/mozilla/repos/b/obj-x86_64-apple-darwin13.2.0/js/src/Unified_cpp_js_src7.cpp:15:
 0:02.20 /files/mozilla/repos/b/js/src/jsfun.cpp:618:35: error: no matching function for call to 'NewFunction'
 0:02.20     RootedObject functionCtor(cx, NewFunction(cx, ctor, Function, 1, JSFunction::NATIVE_CTOR, self,
 0:02.20                                   ^~~~~~~~~~~
 0:02.20 /files/mozilla/repos/b/js/src/jsfun.h:511:1: note: candidate function not viable: no known conversion from 'js::ImmutablePropertyNamePtr' (aka 'ImmutableTenuredPtr<js::PropertyName *>') to 'HandleAtom' (aka 'Handle<JSAtom *>') for 7th argument
 0:02.20 NewFunction(ExclusiveContext *cx, HandleObject funobj, JSNative native, unsigned nargs,
 0:02.20 ^
 0:02.47 In file included from /files/mozilla/repos/b/obj-x86_64-apple-darwin13.2.0/js/src/Unified_cpp_js_src7.cpp:93:
 0:02.47 /files/mozilla/repos/b/js/src/jsobj.cpp:90:12: error: no matching function for call to 'NewFunction'
 0:02.47     return NewFunction(cx, ctor, obj_construct, 1, JSFunction::NATIVE_CTOR, self,
 0:02.47            ^~~~~~~~~~~
 0:02.47 /files/mozilla/repos/b/js/src/jsfun.cpp:1767:5: note: candidate function not viable: no known conversion from 'js::ImmutablePropertyNamePtr' (aka 'ImmutableTenuredPtr<js::PropertyName *>') to 'HandleAtom' (aka 'Handle<JSAtom *>') for 7th argument
 0:02.47 js::NewFunction(ExclusiveContext *cx, HandleObject funobjArg, Native native, unsigned nargs,
 0:02.47     ^

Comment 23

[Bobby Holley (:bholley)]

Thanks for the reviews luke! Final all-platform try push:

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

Comment 24

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #22)
I think that is a compile error b/c C++ would need to do two conversions in a row:
  ImmutableTenuredPtr<PropertyName*> -> Handle<PropertyName*> -> Handle<JSAtom*>
so I think you need to help it along by writing:
  Handle<JSAtom*> objectAtom = cx->names().Object;
which at least avoids the Rooted.

Comment 25

[Bobby Holley (:bholley)]

Still doesn't compile. I'm going to drop it, and let you pick it up if you're inspired.

Comment 26

[Luke Wagner [:luke]]

Oops, I should have written HandlePropertyName.  Anyhow, you can just write:

     NewFunction(...,
                 HandlePropertyName(cx->names().Object));

I actually tried compiling this time.

Comment 27

[Bobby Holley (:bholley)]

Try push isn't 100% done, but it looks green in the places I would be concerned about.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=eaa9169a7756

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d628d406270c
https://hg.mozilla.org/mozilla-central/rev/e9ebab0be6f6
https://hg.mozilla.org/mozilla-central/rev/2105c5982f0f
https://hg.mozilla.org/mozilla-central/rev/f8170b77ee5f
https://hg.mozilla.org/mozilla-central/rev/5d613a24ecec
https://hg.mozilla.org/mozilla-central/rev/d2c49820085c
https://hg.mozilla.org/mozilla-central/rev/73289f388c61
https://hg.mozilla.org/mozilla-central/rev/ac46016a6791
https://hg.mozilla.org/mozilla-central/rev/246b22be3881
https://hg.mozilla.org/mozilla-central/rev/6464056158d3
https://hg.mozilla.org/mozilla-central/rev/8d960134fa7f
https://hg.mozilla.org/mozilla-central/rev/eaa9169a7756

Comment 29

[[e-mail]]

Comment on attachment 8423557 [details] [diff] [review]
Part 3 - Streamline __proto__ getter and setter definitions into a JSPropertySpec. v1

>+static bool
>+ProtoSetter(JSContext *cx, unsigned argc, Value *vp)
...
>-static bool
>-ProtoSetterImpl(JSContext *cx, CallArgs args)
...
>-static bool
>-ProtoSetter(JSContext *cx, unsigned argc, Value *vp)
>-{
>-    CallArgs args = CallArgsFromVp(argc, vp);
>-
>-    // Do this here, rather than in |ProtoSetterImpl|, so even likely-buggy
>-    // use of the __proto__ setter on unacceptable values, where no subsequent
>-    // use occurs on an acceptable value, will trigger a warning.
>-    RootedObject callee(cx, &args.callee());
>-    if (!GlobalObject::warnOnceAboutPrototypeMutation(cx, callee))
>-        return false;
>-
>-    return CallNonGenericMethod(cx, TestProtoThis, ProtoSetterImpl, args);
>-}
Does this not regress bug 948227?

Comment 30

[Bobby Holley (:bholley)]

(In reply to [e-mail] from comment #29)
> Does this not regress bug 948227?

Good catch! Filed bug 1021258.