1469217 - Clean up DefineProperty APIs for accessors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

anba's fix for bug 1105518 made me realize we no longer have JSGetterOps/JSSetterOps outside the JS engine (since bug 1445551 landed), so we should remove JSPROP_PROPOP_ACCESSORS before new uses show up.

While we're at it, we should remove most of the JSObject* <-> JSNative <-> JSGetterOp/JSSetterOp casts in this code.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove JSPROP_PROPOP_ACCESSORS

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add APIs taking getter/setter objects instead of JSNatives

Now we can pass getter/setter objects directly instead of having to do the ugly/error-prone JSNative casting.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Clean up TryResolvePropertyFromSpecs; remove a bunch of macros

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Simplify DefineAccessorPropertyById a bit more

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Simplify DefineAccessorPropertyById a bit more

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Make js::DefineAccessorProperty take HandleObjects instead of JSGetterOp/JSSetterOp

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Clean up TryResolvePropertyFromSpecs; remove a bunch of macros

Comment 8

[André Bargull [:anba]]

Comment on attachment 8985858 [details] [diff] [review]
Part 1 - Remove JSPROP_PROPOP_ACCESSORS

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

Follow-up bug: Maybe we can even now remove JSGetterOp/JSSetterOp from the public API?

Comment 9

[André Bargull [:anba]]

Comment on attachment 8985859 [details] [diff] [review]
Part 2 - Add APIs taking getter/setter objects instead of JSNatives

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

::: js/src/jsapi.cpp
@@ +2472,5 @@
> +                 HandleObject setter, unsigned attrs)
> +{
> +    assertSameCompartment(cx, obj, getter, setter);
> +    AssertHeapIsIdle();
> +    CHECK_REQUEST(cx);

Do we still need these checks here, given that |DefineAccessorPropertyById| will also perform them?

::: js/src/jsapi.h
@@ +2496,5 @@
>                      JS::HandleValue value, unsigned attrs);
>  
>  extern JS_PUBLIC_API(bool)
>  JS_DefineUCProperty(JSContext* cx, JS::HandleObject obj, const char16_t* name, size_t namelen,
> +                    JS::HandleObject getter, JS::HandleObject setter, unsigned attrs);

Let's hope nobody will be sad that the |const char16_t| and |uint32_t| JS_Define APIs no longer accept JSNative.

Comment 10

[André Bargull [:anba]]

Comment on attachment 8985865 [details] [diff] [review]
Part 3 - Clean up TryResolvePropertyFromSpecs; remove a bunch of macros

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +421,5 @@
>          desc.value().setUndefined();
>          unsigned flags = psMatch->flags;
>          if (psMatch->isAccessor()) {
> +            RootedObject getterObj(cx);
> +            RootedObject setterObj(cx);

Nit: Move into the if-statement, because they're not used in the else branch.

@@ +431,3 @@
>                  if (psMatch->accessors.setter.selfHosted.funname) {
>                      MOZ_ASSERT(flags & JSPROP_SETTER);
> +                    JSFunction* setterFun = JS::GetSelfHostedFunction(cx, psMatch->accessors.setter.selfHosted.funname, id, 0);

Pre-existing: I wonder if passing 0 for |nargs| is really correct here, given that setters normally accept exactly one argument.

Comment 11

[André Bargull [:anba]]

Comment on attachment 8985862 [details] [diff] [review]
Part 4 - Simplify DefineAccessorPropertyById a bit more

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

Follow-up bug:
This DefineAccessorProperty function [1] could be cleaned up, too:
- AutoRooterGetterSetter doesn't seem necessary here.
- It could probably inlined into its sole caller.

[1] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/js/src/jsapi.cpp#2253-2267

Comment 12

[André Bargull [:anba]]

Comment on attachment 8985863 [details] [diff] [review]
Part 5 - Make js::DefineAccessorProperty take HandleObjects instead of JSGetterOp/JSSetterOp

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

Can you create a follow-up bug to move (or even just inline and then remove) CastAsGetterOp and CastAsSetterOp now that they're only used in ArgumentsObject.cpp?

::: js/src/vm/JSObject.cpp
@@ +2809,5 @@
>      Rooted<PropertyDescriptor> desc(cx);
> +
> +    {
> +        JSGetterOp getterOp = JS_DATA_TO_FUNC_PTR(GetterOp, getter.get());
> +        JSSetterOp setterOp = JS_DATA_TO_FUNC_PTR(SetterOp, setter.get());

Nit: Maybe use either JSGetterOp or GetterOp, but not both?

::: js/src/vm/JSObject.h
@@ -722,5 @@
>  
>  extern bool
> -DefineAccessorProperty(JSContext* cx, HandleObject obj, PropertyName* name,
> -                       JSGetterOp getter, JSSetterOp setter, unsigned attrs,
> -                       ObjectOpResult& result);

Removing some of the overloads mentioned in bug 1394835 comment #12. :-)

Comment 13

[André Bargull [:anba]]

Great changes, good to see more legacy functionality gone! :-)

Comment 14

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #8)
> Follow-up bug: Maybe we can even now remove JSGetterOp/JSSetterOp from the
> public API?

PropertyDescriptor still uses it :/ See also bug 1404885, I'm not sure how we should represent the array.length/ArgumentsObject properties internally without PropertyOps, maybe a JSObject* tagged pointer (similar to TaggedProto) with magic values for these few internal properties. Doing that is a bigger rewrite though.

Thanks for the reviews, good suggestions. Maybe we should also add an assert to the Shape code to check we only define PropertyOp accessors on ArrayObject and ArgumentsObject, another way to avoid regressing this. I'll post a follow-up patch :)

Comment 15

[Jan de Mooij [:jandem]]

JSPROP_SHADOWABLE and this code can probably be removed as well, because all property ops are shadowable now:

https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/js/src/vm/NativeObject.cpp#2700-2707

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Remove JSPROP_SHADOWABLE, address review comments

Note:

* JSPROP_INTERNAL_USE_BIT is still used by JSPropertySpec to distinguish value/accessor properties:

https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/js/src/jsapi.h#1521

This could/should probably become more of a JSPropertySpec implementation detail at some point, but for now I kept JSPROP_INTERNAL_USE_BIT.

* There is a little duplication in the two NativeDefineAccessorProperty variants (the result.reportError path), but I didn't see a nice way to eliminate this while still keeping the GetterOp/SetterOp scope as small as possible.

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Assert PropertyOps are only defined on ArrayObject/ArgumentsObject

Also removes the AutoRooterGetterSetter constructor that has JSNative arguments, it's no longer used.

Comment 18

[André Bargull [:anba]]

Comment on attachment 8986533 [details] [diff] [review]
Part 6 - Remove JSPROP_SHADOWABLE, address review comments

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

Yay, JSPROP_SHADOWABLE can be removed!

Comment 19

[Tom S [:evilpie]]

(In reply to Jan de Mooij [:jandem] from comment #15)
> JSPROP_SHADOWABLE and this code can probably be removed as well, because all
> property ops are shadowable now:
> 
> https://searchfox.org/mozilla-central/rev/
> d0a41d2e7770fc00df7844d5f840067cc35ba26f/js/src/vm/NativeObject.cpp#2700-2707

\o/ Nice!

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Remove unnecessary array.length code in SetExistingProperty

Just realized we can remove more code from SetExistingProperty... The array.length code [0] there is unnecessary because NativeSetExistingDataProperty handles this case just fine by calling [1] array_length_setter.

With these changes, SetExistingProperty is now quite simple.

[0] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/js/src/vm/NativeObject.cpp#2690-2693

[1] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/js/src/vm/NativeObject.cpp#2492

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca87fb0412c2
part 1 - Remove JSPROP_PROPOP_ACCESSORS. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/66fd5497203f
part 2 - Add APIs taking getter/setter objects instead of JSNatives. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a1c5b1acb9
part 3 - Clean up TryResolvePropertyFromSpecs; remove a bunch of macros. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/dce07077e253
part 4 - Simplify DefineAccessorPropertyById a bit more. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/4db8094fadc1
part 5 - Make js::DefineAccessorProperty take HandleObjects instead of JSGetterOp/JSSetterOp. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/422829b56d7f
part 6 - Remove JSPROP_SHADOWABLE, address review comments. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/98710e23e09b
part 7 - Assert PropertyOps are only defined on ArrayObject/ArgumentsObject. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac80efa3b134
part 8 - Remove unnecessary array.length code in SetExistingProperty. r=anba

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ca87fb0412c2
https://hg.mozilla.org/mozilla-central/rev/66fd5497203f
https://hg.mozilla.org/mozilla-central/rev/72a1c5b1acb9
https://hg.mozilla.org/mozilla-central/rev/dce07077e253
https://hg.mozilla.org/mozilla-central/rev/4db8094fadc1
https://hg.mozilla.org/mozilla-central/rev/422829b56d7f
https://hg.mozilla.org/mozilla-central/rev/98710e23e09b
https://hg.mozilla.org/mozilla-central/rev/ac80efa3b134