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

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: evilpie, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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.
Summary: Remove slot-less getters/setters (without JSPROP_SHARED) → Remove slotful (not slot-less) getters/setters (without JSPROP_SHARED)
(Reporter)

Comment 1

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

Updated

2 years ago
Blocks: 1389159
(Assignee)

Comment 2

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

Comment 3

2 years ago
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8908076 - Flags: review?(evilpies)
(Assignee)

Comment 4

2 years ago
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.
Attachment #8908077 - Flags: review?(evilpies)
(Assignee)

Comment 5

2 years ago
This removes some now-dead code in GetExistingProperty. Accessor props no longer have slots.
Attachment #8908080 - Flags: review?(evilpies)
(Assignee)

Comment 6

2 years ago
This was green on Try a while ago, FWIW.
(Reporter)

Comment 7

2 years ago
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.
Attachment #8908076 - Flags: review?(evilpies) → review+
(Reporter)

Updated

2 years ago
Attachment #8908077 - Flags: review?(evilpies) → review+
(Reporter)

Comment 8

2 years ago
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.
Attachment #8908080 - Flags: review?(evilpies) → review+
(Assignee)

Comment 9

2 years ago
(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

2 years ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Updated

2 years ago
Blocks: 1403136

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b4724648181d
https://hg.mozilla.org/mozilla-central/rev/57132aac9262
https://hg.mozilla.org/mozilla-central/rev/eaf3b1eff03c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Updated

2 years ago
Blocks: 1404310
Depends on: 1407058
Duplicate of this bug: 1164874
You need to log in before you can comment on or make changes to this bug.