Closed Bug 1148750 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(18 files, 2 obsolete files)

part 1 - Factor out the lookup common to three branches at the top of NativeDefineProperty
5.65 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 2 - Check extensibility in NativeDefineProperty
2.43 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 3 - Implement ValidateAndApplyPropertyDescriptor step 2
2.11 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 4 - Strip out redundant if-conditions in parts of NativeDefineProperty where shape can't be null
9.78 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 5 - CompletePropertyDescriptor upgrade
1.72 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 6 - Implement ValidateAndApplyPropertyDescriptor up to step 5
5.10 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 7 - Fill in configurable and enumerable
3.54 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 8 - Implement ValidateAndApplyPropertyDescriptor step 6
7.66 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 9 - Implement ValidateAndApplyPropertyDescriptor step 7
10.23 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 10 - js::NativeDefineProperty: Swap the order of the cases in the remaining old code
6.02 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 11 - Remove some code for TypedArray cases rendered unreachable by part 1 of this bug
2.56 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 12 - Reject redefinition of non-writable non-configurable data property as writable. This fixes bug 1073808
2.74 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 13 - Simplify code to fill in desc.writable, if not present, from the existing shape
3.00 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 14 - Reject redefining a non-writable non-configurable data property to have a different value
3.48 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 15 - Stop retaining getter and setter ops when redefining a data property
1.82 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 16 - Implement ValidateAndApplyPropertyDescriptor step 9 (redefining an existing accessor property). Remove CheckAccessorRedefinition
4.94 KB, patch
efaust
: review+
Details | Diff | Splinter Review
part 17 - Remove ApplyOrDefaultAttributes
4.13 KB, patch
efaust
: review+
Details | Diff | Splinter Review
Differences between the original part 3 and the new stack
4.06 KB, patch
Details | Diff | Splinter Review 
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.
Blocks: 1125624
Blocks: 1073808
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: nobody → jorendorff
Status: NEW → ASSIGNED
Attachment #8586142 - Attachment is obsolete: true
Attachment #8586142 - Flags: review?(efaustbmo)
Attachment #8586143 - Attachment is obsolete: true
Attachment #8586143 - Flags: review?(efaustbmo)
Attachment #8586146 - Attachment is obsolete: true
Attachment #8586146 - Flags: review?(efaustbmo)
Attachment #8586142 - Attachment is obsolete: false
Attachment #8586143 - Attachment is obsolete: false
Attachment #8586143 - Flags: review?(efaustbmo)
Attachment #8586142 - Flags: review?(efaustbmo)
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+
(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.
Flags: needinfo?(jorendorff)
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.
Depends on: 1152106
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.
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)
Attachment #8586152 - Attachment is obsolete: true
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)
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)
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)
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)
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)
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+
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+
Attachment #8590909 - Flags: review?(efaustbmo) → review+
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+
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+
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: