Clean up DefineProperty APIs for accessors

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 2 bugs)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(8 attachments, 2 obsolete attachments)

10.53 KB, patch
anba
: review+
Details | Diff | Splinter Review
24.93 KB, patch
anba
: review+
Details | Diff | Splinter Review
2.02 KB, patch
anba
: review+
Details | Diff | Splinter Review
10.81 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.85 KB, patch
anba
: review+
Details | Diff | Splinter Review
23.31 KB, patch
anba
: review+
Details | Diff | Splinter Review
4.65 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.26 KB, patch
anba
: review+
Details | Diff | Splinter Review
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.
Attachment #8985858 - Flags: review?(andrebargull)
Now we can pass getter/setter objects directly instead of having to do the ugly/error-prone JSNative casting.
Attachment #8985859 - Flags: review?(andrebargull)
Attachment #8985861 - Flags: review?(andrebargull)
Attachment #8985861 - Attachment is obsolete: true
Attachment #8985861 - Flags: review?(andrebargull)
Attachment #8985862 - Flags: review?(andrebargull)
Attachment #8985860 - Attachment is obsolete: true
Attachment #8985860 - Flags: review?(andrebargull)
Attachment #8985865 - Flags: review?(andrebargull)
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?
Attachment #8985858 - Flags: review?(andrebargull) → review+
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.
Attachment #8985859 - Flags: review?(andrebargull) → review+
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.
Attachment #8985865 - Flags: review?(andrebargull) → review+
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
Attachment #8985862 - Flags: review?(andrebargull) → review+
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. :-)
Attachment #8985863 - Flags: review?(andrebargull) → review+
Great changes, good to see more legacy functionality gone! :-)
(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 :)
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
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.
Attachment #8986533 - Flags: review?(andrebargull)
Also removes the AutoRooterGetterSetter constructor that has JSNative arguments, it's no longer used.
Attachment #8986542 - Flags: review?(andrebargull)
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!
Attachment #8986533 - Flags: review?(andrebargull) → review+
Attachment #8986542 - Flags: review?(andrebargull) → review+
(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!
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
Attachment #8986560 - Flags: review?(andrebargull)
Attachment #8986560 - Flags: review?(andrebargull) → review+
Blocks: 1470081
Pushed by jandemooij@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.