Bring NativeDefineProperty up to ES6 [[DefineOwnProperty]] standard behavior, with one exception for resolve hooks

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(18 attachments, 2 obsolete attachments)

5.65 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.43 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.11 KB, patch
efaust
: review+
Details | Diff | Splinter Review
9.78 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.72 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.10 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.54 KB, patch
efaust
: review+
Details | Diff | Splinter Review
7.66 KB, patch
efaust
: review+
Details | Diff | Splinter Review
10.23 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.02 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.56 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.74 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.00 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.48 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.82 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.94 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.13 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.06 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
We're in the final stretch here, and ready to do the pleasant work of rewriting NativeDefineProperty with step numbers from the standard.

One wrinkle will still remain when we are done here. NativeDefineProperty contains some nonstandard code to avoid unchecked recursion when a resolve hook defines a property. We need to find a standard-compliant way to deal with this. Maybe that's more of a hurdle than a wrinkle. But "wrinkle" implies that it's a twist; "hurdle" doesn't. It's really both. A hurkle. Anyway, all that's for another bug.
(Assignee)

Updated

4 years ago
Blocks: 1125624
(Assignee)

Updated

4 years ago
Blocks: 1073808
(Assignee)

Comment 4

4 years ago
Created attachment 8586142 [details] [diff] [review]
part 1 - Factor out the lookup common to three branches at the top of NativeDefineProperty

The existing setup saves a branch. We can't keep it. All that code is about to be completely rewritten. In the standard algorithms, this check is not immediately followed by a branch on this particular condition (desc.hasValue()). Furthermore, to deal with resolve hooks properly, we will later change the condition of this if-statement to something like `if (resolving)`, which will not be something we can common up with any other branch in this function.
Attachment #8586142 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
Created attachment 8586143 [details] [diff] [review]
part 2 - Check extensibility in NativeDefineProperty
Attachment #8586143 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8586142 - Attachment is obsolete: true
Attachment #8586142 - Flags: review?(efaustbmo)
(Assignee)

Comment 7

4 years ago
Created attachment 8586146 [details] [diff] [review]
part 3 - Rewrite the rest of NativeDefineProperty. At this point it stops being practical to continue in small chunks
Attachment #8586146 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8586143 - Attachment is obsolete: true
Attachment #8586143 - Flags: review?(efaustbmo)
(Assignee)

Comment 8

4 years ago
Created attachment 8586152 [details] [diff] [review]
part 3 - Rewrite the rest of NativeDefineProperty. At this point it stops being practical to continue in small chunks
Attachment #8586152 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8586146 - Attachment is obsolete: true
Attachment #8586146 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8586142 - Attachment is obsolete: false
(Assignee)

Updated

4 years ago
Attachment #8586143 - Attachment is obsolete: false
Attachment #8586143 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8586142 - Flags: review?(efaustbmo)

Comment 9

4 years ago
Comment on attachment 8586142 [details] [diff] [review]
part 1 - Factor out the lookup common to three branches at the top of NativeDefineProperty

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

Yeah, but this is cleaner. I agree that the other strategy is not future-stable.
Attachment #8586142 - Flags: review?(efaustbmo) → review+
Comment on attachment 8586143 [details] [diff] [review]
part 2 - Check extensibility in NativeDefineProperty

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

r=me with the caveat that I think we have an impl of ValidateAndApply... in the scripted proxy code. We should look into what, if any, code can be merged with that implementation.
Attachment #8586143 - Flags: review?(efaustbmo) → review+
Comment on attachment 8586152 [details] [diff] [review]
part 3 - Rewrite the rest of NativeDefineProperty. At this point it stops being practical to continue in small chunks

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

This looks good. Again, we should try to common up as much as possible with the other impl. ISTR it only does the apply, not the validate, so we should probably keep this one.

::: js/src/jsapi.h
@@ +2736,5 @@
> +    }
> +    void setSetterObject(JSObject* obj) {
> +        desc()->setter = reinterpret_cast<JSSetterOp>(obj);
> +        desc()->attrs &= ~(JSPROP_IGNORE_VALUE | JSPROP_IGNORE_READONLY);
> +        desc()->attrs |= JSPROP_SETTER | JSPROP_SHARED;

yes. Much less footgun this way :)

::: js/src/jsobjinlines.h
@@ -840,5 @@
>      return true;
>  }
>  
> -static inline unsigned
> -ApplyAttributes(unsigned attrs, bool enumerable, bool writable, bool configurable)

My god am I glad to see this go.

::: js/src/tests/ecma_6/Class/classPrototype.js
@@ -30,5 @@
> -} catch (e if e instanceof TypeError) {
> -    throw new Error("Congrats on making initprop respect non-writable " +
> -                    "non-configurable properties. Uncomment the test below " +
> -                    "for bonus points.");
> -/*

Yay! A sleeper test awoken :) Bonus points.
Attachment #8586152 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 12

4 years ago
(In reply to Eric Faust [:efaust] from comment #10)
> r=me with the caveat that I think we have an impl of ValidateAndApply... in
> the scripted proxy code. We should look into what, if any, code can be
> merged with that implementation.

Agreed. Filed follow-up bug 1150140.
Backed out (along with every other patch from that massive push) in https://hg.mozilla.org/integration/mozilla-inbound/rev/386c8b5b73c0 for talos other timeouts:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3fc49391f7fe&filter-searchStr=talos oth
Flags: needinfo?(jorendorff)
(Assignee)

Updated

4 years ago
Flags: needinfo?(jorendorff)
(Assignee)

Comment 36

4 years ago
Looking good, finally. Bug 1152106 is part of this; I also revised some of the changes in this bug; I'll post the new code for review tonight or tomorrow.
(Assignee)

Updated

4 years ago
Depends on: 1152106
(Assignee)

Comment 40

4 years ago
I told Eric that it would be possible after all to divide Part 3 into many smaller patches, and he decided that in that case, it would be worth re-reviewing it. So let's assume that bug 1152106 will be resolved in some sensible way, and get started on that.

The patches I'm about to upload add up to almost exactly what's in the original "Part 3". I'll also upload another diff showing what's different once you apply the whole stack. Not much. Eric's seen it already.

Eric, please look in particular for opportunities to add new tests where behavior changes. Object.defineProperty() is of course already very nearly correct. But it'd be nice to know what user-visible behavior we're fixing here (both in Object.defineProperty and elsewhere) and add test coverage for that.
(Assignee)

Comment 41

4 years ago
Created attachment 8590900 [details] [diff] [review]
part 3 - Implement ValidateAndApplyPropertyDescriptor step 2

The new comment "Filling in desc:" is aspirational for now, but it gradually becomes true in the subsequent patches in this bug.
Attachment #8590900 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8586152 - Attachment is obsolete: true
(Assignee)

Comment 42

4 years ago
Created attachment 8590901 [details] [diff] [review]
part 4 - Strip out redundant if-conditions in parts of NativeDefineProperty where shape can't be null
Attachment #8590901 - Flags: review?(efaustbmo)
(Assignee)

Comment 43

4 years ago
Created attachment 8590902 [details] [diff] [review]
part 5 - CompletePropertyDescriptor upgrade
Attachment #8590902 - Flags: review?(efaustbmo)
(Assignee)

Comment 44

4 years ago
Created attachment 8590903 [details] [diff] [review]
part 6 - Implement ValidateAndApplyPropertyDescriptor up to step 5
Attachment #8590903 - Flags: review?(efaustbmo)
(Assignee)

Comment 45

4 years ago
Created attachment 8590904 [details] [diff] [review]
part 7 - Fill in configurable and enumerable

This changes MutableHandle<PropertyDescriptor>::setEnumerable() to take a boolean argument, and makes it unconditionally clear JSPROP_IGNORE_ENUMERATE, as a convenience. A similar setConfigurable() method is also added.
Attachment #8590904 - Flags: review?(efaustbmo)
(Assignee)

Comment 46

4 years ago
Created attachment 8590905 [details] [diff] [review]
part 8 - Implement ValidateAndApplyPropertyDescriptor step 6

This also makes some changes to MutableHandle<PropertyDescriptor>, for convenience:

*   Make desc.setGetterObject() and desc.setSetterObject() quietly change *this from a generic or data descriptor into an accessor descriptor, if need be.

*   Make setWritable() clear the JSPROP_IGNORE_READONLY bit if present. (Breaking the symmetry a bit, it won't change *this from an accessor descriptor to a data descriptor. Instead, it asserts you're not doing that.)
Attachment #8590905 - Flags: review?(efaustbmo)
(Assignee)

Comment 47

4 years ago
Created attachment 8590906 [details] [diff] [review]
part 9 - Implement ValidateAndApplyPropertyDescriptor step 7

The new code takes over some cases that used to be handled by each of the three cases that follow it. Therefore there are changes in all three cases, particularly the desc.isAccessorDescriptor() case, which no longer needs the sparsify code.
Attachment #8590906 - Flags: review?(efaustbmo)
(Assignee)

Comment 48

4 years ago
Created attachment 8590907 [details] [diff] [review]
part 10 - js::NativeDefineProperty: Swap the order of the cases in the remaining old code
Attachment #8590907 - Flags: review?(efaustbmo)
(Assignee)

Comment 49

4 years ago
Created attachment 8590909 [details] [diff] [review]
part 11 - Remove some code for TypedArray cases rendered unreachable by part 1 of this bug
Attachment #8590909 - Flags: review?(efaustbmo)
(Assignee)

Comment 50

4 years ago
Created attachment 8590911 [details] [diff] [review]
part 12 - Reject redefinition of non-writable non-configurable data property as writable. This fixes bug 1073808
Attachment #8590911 - Flags: review?(efaustbmo)
(Assignee)

Comment 51

4 years ago
Created attachment 8590912 [details] [diff] [review]
part 13 - Simplify code to fill in desc.writable, if not present, from the existing shape
Attachment #8590912 - Flags: review?(efaustbmo)
(Assignee)

Comment 52

4 years ago
Created attachment 8590913 [details] [diff] [review]
part 14 - Reject redefining a non-writable non-configurable data property to have a different value
Attachment #8590913 - Flags: review?(efaustbmo)
(Assignee)

Comment 53

4 years ago
Created attachment 8590915 [details] [diff] [review]
part 15 - Stop retaining getter and setter ops when redefining a data property

StandardDefineProperty already does this. In short, redefining a magical data property like array.length or an arguments element should make it nonmagical.

In fact, it is an engine invariant that Shapes have either both JSPROP_GETTER and JSPROP_SETTER or neither: they never have e.g. a setter object and a getter op. As of recently the GC depends on this. So this change is necessary for memory safety.
Attachment #8590915 - Flags: review?(efaustbmo)
(Assignee)

Comment 54

4 years ago
Created attachment 8590916 [details] [diff] [review]
part 16 - Implement ValidateAndApplyPropertyDescriptor step 9 (redefining an existing accessor property). Remove CheckAccessorRedefinition
Attachment #8590916 - Flags: review?(efaustbmo)
(Assignee)

Comment 55

4 years ago
Created attachment 8590918 [details] [diff] [review]
part 17 - Remove ApplyOrDefaultAttributes

At this point, each path through the code completely fills in desc, so the final call to ApplyOrDefaultAttributes is redundant. Note that AddOrChangeProperty begins with `desc.assertComplete()`, so we'll find out soon enough if this is not the case.
Attachment #8590918 - Flags: review?(efaustbmo)
(Assignee)

Comment 57

4 years ago
Created attachment 8591047 [details] [diff] [review]
Differences between the original part 3 and the new stack
Comment on attachment 8590900 [details] [diff] [review]
part 3 - Implement ValidateAndApplyPropertyDescriptor step 2

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

Yep
Attachment #8590900 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590901 [details] [diff] [review]
part 4 - Strip out redundant if-conditions in parts of NativeDefineProperty where shape can't be null

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

::: js/src/vm/NativeObject.cpp
@@ +1341,5 @@
>              return false;
>          return result.succeed();
>      }
>  
>      // If defining a getter or setter, we must check for its counterpart and

This is dumb, but can we stick a MOZ_ASSERT(shape) above this comment, even though it's patently obviously true, just to guard against future refactoring stupidness? Maybe this isn't necessary, but I've seen this idea elsewhere in the codebase, and it certainly doesn't hurt.
Attachment #8590901 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590902 [details] [diff] [review]
part 5 - CompletePropertyDescriptor upgrade

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

I assume assertValid asserts that desc.object is non-null?
Attachment #8590902 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590903 [details] [diff] [review]
part 6 - Implement ValidateAndApplyPropertyDescriptor up to step 5

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

Good, assuming we are still using JSPROP_REDEFINE_NONCONFIGURABLE in that one spot.

::: js/src/tests/ecma_6/Class/classPrototype.js
@@ -29,5 @@
> -          }\`);
> -} catch (e if e instanceof TypeError) {
> -    throw new Error("Congrats on making initprop respect non-writable " +
> -                    "non-configurable properties. Uncomment the test below " +
> -                    "for bonus points.");

jorendorff++ :)

::: js/src/vm/NativeObject.cpp
@@ +1352,5 @@
> +    // access to accessors found on such an object. Bug 1105518 contemplates
> +    // removing this hack.
> +    bool skipRedefineChecks = (desc.attributes() & JSPROP_REDEFINE_NONCONFIGURABLE) &&
> +                              obj->is<GlobalObject>() &&
> +                              !obj->getClass()->isDOMClass();

Where did we land on the XPCGlobalYadaYada thing? Did we go back to REDEFINE_NONCONFIGURABLE?
Attachment #8590903 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590904 [details] [diff] [review]
part 7 - Fill in configurable and enumerable

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

::: js/src/jsapi.h
@@ +2702,5 @@
>      }
>  
> +    void setConfigurable(bool configurable) {
> +        setAttributes((desc()->attrs & ~(JSPROP_IGNORE_PERMANENT | JSPROP_PERMANENT)) |
> +                      (configurable ? 0 : JSPROP_PERMANENT));

Yeah, this never got updated when IGNORE was added. This is a nice touch.

::: js/src/vm/NativeObject.cpp
@@ +1189,5 @@
>      return Throw(cx->asJSContext(), shape->propid(), JSMSG_CANT_REDEFINE_PROP);
>  }
>  
>  static bool
> +AddOrChangeProperty(ExclusiveContext* cx, HandleNativeObject obj, HandleId id,

It's cool that the type system lets us specify this now.
Attachment #8590904 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590905 [details] [diff] [review]
part 8 - Implement ValidateAndApplyPropertyDescriptor step 6

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

::: js/src/vm/NativeObject.cpp
@@ +1268,5 @@
> +                    typename MaybeRooted<Shape*, allowGC>::HandleType shape,
> +                    typename MaybeRooted<Value, allowGC>::MutableHandleType vp);
> +
> +static bool
> +GetExistingPropertyValue(ExclusiveContext* cx, HandleNativeObject obj, HandleId id,

This is a really nice abstraction. I've been generally pleasantly surprised with how well we've been able to hide the various random object types flowing through here behind sensible helpers in the core algorithm.
Attachment #8590905 - Flags: review?(efaustbmo) → review+

Updated

4 years ago
Attachment #8590906 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590907 [details] [diff] [review]
part 10 - js::NativeDefineProperty: Swap the order of the cases in the remaining old code

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

APPROVED.
Attachment #8590907 - Flags: review?(efaustbmo) → review+

Updated

4 years ago
Attachment #8590909 - Flags: review?(efaustbmo) → review+

Updated

4 years ago
Attachment #8590911 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590912 [details] [diff] [review]
part 13 - Simplify code to fill in desc.writable, if not present, from the existing shape

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

::: js/src/vm/NativeObject.cpp
@@ +1460,5 @@
>                  shape = obj->lookup(cx, id);
>              }
>  
> +            // We are at most changing some attributes. Take everything from
> +            // the shape that we aren't changing.

Ah, we don't need to deal with this, because we flow into AddOrChangeProperty below, right?
Attachment #8590912 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590913 [details] [diff] [review]
part 14 - Reject redefining a non-writable non-configurable data property to have a different value

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

Nice.
Attachment #8590913 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590915 [details] [diff] [review]
part 15 - Stop retaining getter and setter ops when redefining a data property

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

Agreed.
Attachment #8590915 - Flags: review?(efaustbmo) → review+

Updated

4 years ago
Attachment #8590916 - Flags: review?(efaustbmo) → review+
Comment on attachment 8590918 [details] [diff] [review]
part 17 - Remove ApplyOrDefaultAttributes

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

Good. Glad to see this mistake go.
Attachment #8590918 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 70

4 years ago
Comment 60 (reviewing part 5, attachment 8590902 [details] [diff] [review]):
> I assume assertValid asserts that desc.object is non-null?

No, because DefineProperty should completely ignore desc.object(), and
callers should pass null. I'll file a bug about this. Already have the
patch.

The rule here is that we should avoid redundant arguments, as that is
begging for inconsistencies. If desc.object() is used, how is it
different from the obj argument?

(Alternatively, I could have changed DefineProperty to use desc.object()
as the target object *instead of* the obj parameter, which could simply
be dropped. But I think keeping the obj parameter is more sensible.
Proxies in particular treat `obj` as separate from `desc`.)

Comment 61 (reviewing part 6, attachment 8590903 [details] [diff] [review]):
> Where did we land on the XPCGlobalYadaYada thing? Did we go back to
> REDEFINE_NONCONFIGURABLE?

bholley was able to handhold me through actually fixing it.
No new `JSPROP_REDEFINE_NONCONFIGURABLE` uses. Yay.

Comment 65 (reviewing part 13, attachment 8590912 [details] [diff] [review]):
> Ah, we don't need to deal with this, because we flow into
> AddOrChangeProperty below, right?

Not exactly - it's because the patch adds code (above this comment) to
deal with the writable bit, and part 7 takes care of enumerable and
configurable. Those are the only bits we have on data properties, so no
more bitmath is necessary.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1073808
You need to log in before you can comment on or make changes to this bug.