1153592 - Remove slotful (not slot-less) getters/setters (without JSPROP_SHARED)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

JSPROP_SHARED means slot-less property as far as I understand. Normally getters/setters should never have slots to store values, but we allow it. If we remove those we can simplify some stuff and hopefully get rid of the weird JSPROP_SHARED flag.

Comment 1

[Tom S [:evilpie]]

So an example of this are XPC objects. They have a setProperty hook, so all property definitions with just a value automatically get that hook as a setter, but we don't use JSPROP_SHARED. In this case we definitely need the slot to store the value, but still probably want the setter. Maybe we should first think about removing the behavior where we automatically use JSClass.setProperty and JSClass.getProperty as {g,s}etters for their properties.

Comment 2

[Jan de Mooij [:jandem]]

I wrote some patches for this a while ago. I should probably just post them, so we can land this after the merge next week.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Replace checks with asserts in CheckCanChangeAttrs

CheckCanChangeAttrs is redundant nowadays: the callers are responsible for ensuring we don't redefine non-configurable properties etc.

This patch just uses asserts in CheckCanChangeAttrs.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove JSPROP_SHARED

This removes JSPROP_SHARED. A shape gets a slot iff it doesn't have a getter and setter (and is not the empty shape).

There's a lot more we can clean up and simplify in follow-ups.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Simplify GetExistingProperty

This removes some now-dead code in GetExistingProperty. Accessor props no longer have slots.

Comment 6

[Jan de Mooij [:jandem]]

This was green on Try a while ago, FWIW.

Comment 7

[Tom S [:evilpie]]

Comment on attachment 8908076 [details] [diff] [review]
Part 1 - Replace checks with asserts in CheckCanChangeAttrs

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

::: js/src/vm/Shape.cpp
@@ +671,3 @@
>   */
> +static void
> +CheckCanChangeAttrs(Shape* shape, unsigned attrs)

I think we would name this like AssertSomething usually.

Comment 8

[Tom S [:evilpie]]

Comment on attachment 8908080 [details] [diff] [review]
Part 3 - Simplify GetExistingProperty

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

Nice \o/

::: js/src/vm/NativeObject.cpp
@@ +2145,1 @@
>      if (shape->hasDefaultGetter())

Is this still possible? I thought we would throw for setter only properties.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #8)
> Is this still possible? I thought we would throw for setter only properties.

Not sure; it might be possible with legacy getters/setters? I'll do more clean up in this area, will see what our invariants are.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4724648181d
part 1 - Replace redundant checks in CheckCanChangeAttrs with debug asserts. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/57132aac9262
part 2 - Remove JSPROP_SHARED; ensure accessor props don't have slots. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf3b1eff03c
part 3 - Simplify GetExistingProperty now that slotful getters are gone. r=evilpie

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b4724648181d
https://hg.mozilla.org/mozilla-central/rev/57132aac9262
https://hg.mozilla.org/mozilla-central/rev/eaf3b1eff03c