Bug 142933 – Redefining a property should not change its insertion index (Object.keys order)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joseph Pecoraro]

* SUMMARY
Redefining a property should not change its index in Object.keys.

* TEST:
var o = {};
o.x = 1;
o.y = 2;
o.__defineGetter__("x", function() {});
Object.keys(o);

* EXPECTED
["x", "y"]

* ACTUAL
["y", "x"]

* NOTES
- Firefox outputs expected
- Chrome outputs expected

[Joseph Pecoraro]

My guess would be the deleteProperty called from JSObject::defineOwnNonIndexProperty, removes the "x" and then re-adds "x" to the end. We should have a replace here, that avoids the deletion.

[Joseph Pecoraro]

Relevant parts of the spec are:

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-validateandapplypropertydescriptor
- [[DefineOwnProperty]] -> ValidateAndApplyPropertyDescriptor -> "Convert the property ... from a data property to an accessor property"

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-ownpropertykeys
- [[OwnPropertyKeys]] -> enumerates indexes, properties "in property creation order", and Symbols in proprty creation order

Though __defineGetter__ and __defineSetter__ are not specced, this should be no different then a Object.defineProperty changing a data descriptor to an accessor (and vice versa).

Now lets see if there is a JSC internal reason for the property deletion.

[Joseph Pecoraro]

Doesn't appear to be any particular reason. The implementation has always had a delete since Object.defineProperty was first added for ES5 support years ago. ES5 has the update wording here.

[Joseph Pecoraro]

Hah, the ChangeLog had:

> Currently defineOwnProperty uses a delete followed by a put to redefine
> attributes of a property, clearly this is less efficient than it could be
> but we can improve this if it needs to be possible in future.

I guess now is that time! But necessarily for efficiency =).

[Joseph Pecoraro]

Created attachment 249247 [details]
[PATCH] Work In Progress - Needs Cleanup

Work in progress, not for review. Just saving my progress. I'll likely not pick this up again for a few weeks.

  - passes all tests as written
  - need to write many new tests to test this
  - needs some cleanup, as noted by FIXMEs
  - we may be able to remove DefineOwnPropertyScope and its uses throughout JSC

[Alexey Shvayka]

Created attachment 398372 [details]
Patch

[Alexey Shvayka]

Comment on attachment 398372 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398372&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4325
> +            push(@implContent, "        JSCell::deleteProperty(this, globalObject(), propertyName);\n");

I will make sure to ASSERT delete success here and below, just making sure tests pass for now.

> JSTests/test262/expectations.yaml:1012
> +  default: 'Test262Error: Expected [a, name] and [name, a] to have the same contents. '

We are dealing with JSFunction's non-reified "name" property, hence the failure here and below (for "length").

[Alexey Shvayka]

Created attachment 398417 [details]
Patch

Set hasReadOnlyOrGetterSetterPropertiesExcludingProto(), unset PropertyAttribute::ReadOnly for accessors, adjust tests.

[Alexey Shvayka]

Created attachment 398787 [details]
Patch

Bring back DeletePropertyMode, always call attributeChangeTransition(), add tests and microbenchmarks.

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #9)
> Bring back DeletePropertyMode, always call attributeChangeTransition(), add
> tests and microbenchmarks.

While this patch enables complete removal of DeletePropertyMode, I'd rather do it in a follow-up, along with implementing CanDeclareGlobalFunction for JSGlobalObject::addFunction().


Cold runs, --inner 4:

                                                r261263                    patch                                       

redefine-property-accessor-dictionary       64.1904+-0.5412     ^     39.7252+-2.3096        ^ definitely 1.6159x faster
redefine-property-accessor                  22.1282+-0.3532     ^     21.0878+-0.5099        ^ definitely 1.0493x faster
redefine-property-data-dictionary           60.6094+-0.4155     ^     32.5806+-1.2206        ^ definitely 1.8603x faster
redefine-property-data                      18.8928+-1.0337     ^     16.7405+-0.7271        ^ definitely 1.1286x faster

<geometric>                                 35.7080+-0.3323     ^     25.9930+-0.4242        ^ definitely 1.3738x faster

[Yusuke Suzuki]

(In reply to Alexey Shvayka from comment #10)
> (In reply to Alexey Shvayka from comment #9)
> > Bring back DeletePropertyMode, always call attributeChangeTransition(), add
> > tests and microbenchmarks.
> 
> While this patch enables complete removal of DeletePropertyMode, I'd rather
> do it in a follow-up, along with implementing CanDeclareGlobalFunction for
> JSGlobalObject::addFunction().
> 
> 
> Cold runs, --inner 4:
> 
>                                                 r261263                   
> patch                                       
> 
> redefine-property-accessor-dictionary       64.1904+-0.5412     ^    
> 39.7252+-2.3096        ^ definitely 1.6159x faster
> redefine-property-accessor                  22.1282+-0.3532     ^    
> 21.0878+-0.5099        ^ definitely 1.0493x faster
> redefine-property-data-dictionary           60.6094+-0.4155     ^    
> 32.5806+-1.2206        ^ definitely 1.8603x faster
> redefine-property-data                      18.8928+-1.0337     ^    
> 16.7405+-0.7271        ^ definitely 1.1286x faster
> 
> <geometric>                                 35.7080+-0.3323     ^    
> 25.9930+-0.4242        ^ definitely 1.3738x faster

Awesome!

[Yusuke Suzuki]

Comment on attachment 398787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398787&action=review

Looks nice. But I think some of Structures' flags are missing.

> Source/JavaScriptCore/runtime/Structure.cpp:676
> +    if (attributes & (PropertyAttribute::ReadOnly | PropertyAttribute::Accessor))
> +        structure->setHasReadOnlyOrGetterSetterPropertiesExcludingProto(true);

Do we ensure that propertyName is not __proto__?

> Source/JavaScriptCore/runtime/Structure.cpp:677
> +

Is it OK not to set setHasGetterSetterProperties / setHasCustomGetterSetterProperties etc. Can you review all the flags of Structure?

[Alexey Shvayka]

Created attachment 399662 [details]
Patch

Use setContainsReadOnlyProperties(), improve tests, and tweak ChangeLog.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #12)
> Is it OK not to set setHasGetterSetterProperties /
> setHasCustomGetterSetterProperties etc. Can you review all the flags of
> Structure?

Thank you for review, Yusuke.

I've vetted all the flags and seems like we are safe here: all call sites of putDirectInternal<PutModeDefineOwnProperty>
perform setHasGetterSetterPropertiesWithProtoCheck(), including custom accessors. __proto__ property creation is also
handled earlier in Structure::add().

Tests are updated to cover __proto__ and indexed properties.

[Alexey Shvayka]

Created attachment 402168 [details]
Patch

Rebase test262/expectations.yaml and simplify test added in r263070.

[Saam Barati]

Comment on attachment 402168 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402168&action=review

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:108
> +    exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message), static_cast<unsigned>(PropertyAttribute::DontEnum));

should this be its own patch?

> Source/JavaScriptCore/runtime/JSObject.cpp:3583
> +                object->putDirect(vm, propertyName, value, descriptor.attributes() & ~PropertyAttribute::Accessor);

why are you masking out accessor? Why is that not an assert?

> Source/JavaScriptCore/runtime/JSObject.cpp:3654
> +        object->putDirect(vm, propertyName, value, attributes & ~PropertyAttribute::Accessor);

why is masking needed?

> Source/JavaScriptCore/runtime/JSObjectInlines.h:317
> +            if (mode == PutModeDefineOwnProperty && (attributes != currentAttributes || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue)))

so we always transition for defining a new accessor? Is this to maintain the invariant before that we did delete?

I think semantically we only care about doing this for customs and custom values

[Joseph Pecoraro]

Awesome!

[Alexey Shvayka]

(In reply to Saam Barati from comment #16)

I appreciate you taking a look, Saam.

> > Source/JavaScriptCore/runtime/ErrorInstance.cpp:108
> > +    exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message), static_cast<unsigned>(PropertyAttribute::DontEnum));
> 
> should this be its own patch?

We can land ErrorInstance.cpp change before the main patch, but we can't make it a follow-up since JSTests/stress/native-error-properties.js fails w/o it.
Because of the changes in JSObjectInlines.h, we need to explicitly set attributes to avoid Structure::attributeChangeTransition() branch.
With this patch, putDirectWithoutTransition() will fail loudly if data property already exists with different attributes.

> > Source/JavaScriptCore/runtime/JSObject.cpp:3583
> > +                object->putDirect(vm, propertyName, value, descriptor.attributes() & ~PropertyAttribute::Accessor);
> 
> why are you masking out accessor? Why is that not an assert?
> 
> > Source/JavaScriptCore/runtime/JSObject.cpp:3654
> > +        object->putDirect(vm, propertyName, value, attributes & ~PropertyAttribute::Accessor);
> 
> why is masking needed?

Masking doesn't feel right to me either; please note that I am not adding new instances, but merely preserving ones we had in putDescriptor().
Since there are similar masking outs in putIndexedDescriptor(), we can make a follow-up that moves them to PropertyDescriptor::attributesOverridingCurrent().

> > Source/JavaScriptCore/runtime/JSObjectInlines.h:317
> > +            if (mode == PutModeDefineOwnProperty && (attributes != currentAttributes || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue)))
> 
> so we always transition for defining a new accessor? Is this to maintain the
> invariant before that we did delete?

JSTests/stress/intrinsic-getter-with-poly-proto-getter-change.js fails if we don't preserve this invariant.
Please also note `current.equalTo(globalObject, descriptor)` check that avoids putDirect* route if accessor descriptor's [[Get]] and [[Set]] are the same.



(In reply to Joseph Pecoraro from comment #17)
> Awesome!

Thank you for sharing WIP patch!

[Alexey Shvayka]

Created attachment 403690 [details]
Patch

Rebase test262 expectations.

[Joseph Pecoraro]

Comment on attachment 403690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403690&action=review

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:108
> -    exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message));
> +    exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message), static_cast<unsigned>(PropertyAttribute::DontEnum));

Should this be separate? At the least it should be called out in the ChangeLog.

> LayoutTests/js/script-tests/object-literal-duplicate-properties.js:-57
> -// FIXME: <https://webkit.org/b/142933> Redefining a property should not change its insertion index (Object.keys order)

Awesome! And since we are here can we drive-by update the comment on line 12 to say "... eliminated by basic properties.". This typo of mine has annoyed me for a while!

[Alexey Shvayka]

(In reply to Joseph Pecoraro from comment #20)
> Should this be separate? At the least it should be called out in the
> ChangeLog.

Due to a reason mentioned in comment #16, we can't make it a follow-up, so let's land it first: https://webkit.org/b/214089.

> Awesome! And since we are here can we drive-by update the comment on line 12
> to say "... eliminated by basic properties.". This typo of mine has annoyed
> me for a while!

Thank you, I will fix it during next rebase.

[Alexey Shvayka]

Created attachment 403934 [details]
Patch

Rebase patch to drop ErrorInstance.cpp change and fix typos.

[Saam Barati]

(In reply to Alexey Shvayka from comment #18)
> (In reply to Saam Barati from comment #16)
> 
> I appreciate you taking a look, Saam.
> 
> > > Source/JavaScriptCore/runtime/ErrorInstance.cpp:108
> > > +    exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message), static_cast<unsigned>(PropertyAttribute::DontEnum));
> > 
> > should this be its own patch?
> 
> We can land ErrorInstance.cpp change before the main patch, but we can't
> make it a follow-up since JSTests/stress/native-error-properties.js fails
> w/o it.
> Because of the changes in JSObjectInlines.h, we need to explicitly set
> attributes to avoid Structure::attributeChangeTransition() branch.
> With this patch, putDirectWithoutTransition() will fail loudly if data
> property already exists with different attributes.
> 
> > > Source/JavaScriptCore/runtime/JSObject.cpp:3583
> > > +                object->putDirect(vm, propertyName, value, descriptor.attributes() & ~PropertyAttribute::Accessor);
> > 
> > why are you masking out accessor? Why is that not an assert?
> > 
> > > Source/JavaScriptCore/runtime/JSObject.cpp:3654
> > > +        object->putDirect(vm, propertyName, value, attributes & ~PropertyAttribute::Accessor);
> > 
> > why is masking needed?
> 
> Masking doesn't feel right to me either; please note that I am not adding
> new instances, but merely preserving ones we had in putDescriptor().
> Since there are similar masking outs in putIndexedDescriptor(), we can make
> a follow-up that moves them to
> PropertyDescriptor::attributesOverridingCurrent().
> 
> > > Source/JavaScriptCore/runtime/JSObjectInlines.h:317
> > > +            if (mode == PutModeDefineOwnProperty && (attributes != currentAttributes || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue)))
> > 
> > so we always transition for defining a new accessor? Is this to maintain the
> > invariant before that we did delete?
> 
> JSTests/stress/intrinsic-getter-with-poly-proto-getter-change.js fails if we
> don't preserve this invariant.
> Please also note `current.equalTo(globalObject, descriptor)` check that
> avoids putDirect* route if accessor descriptor's [[Get]] and [[Set]] are the
> same.

I think we need to transition when changing custom value/custom accessor. But changing GetterSetter without transitioning should work, since all of our ICs are dynamic over this value.

> 
> 
> 
> (In reply to Joseph Pecoraro from comment #17)
> > Awesome!
> 
> Thank you for sharing WIP patch!

[Saam Barati]

Comment on attachment 403934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403934&action=review

r=me with one comment/question

> Source/JavaScriptCore/ChangeLog:61
> -        beind redefined with PropertyAttribute::None.
> +        being redefined with PropertyAttribute::None.

no need to fix this now IMO. We usually don't go back and fix ChangeLog typos afaik.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:317
> +            if (mode == PutModeDefineOwnProperty && (attributes != currentAttributes || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue)))

same comment here. I think we should be ok with just CustomAccessorOrValue. Do tests failing just using CustomAccessorOrValue? If so, we should find out why.

[Alexey Shvayka]

Created attachment 404321 [details]
Patch

Fix flaky microbenchmarks, add FIXMEs, undo ChangeLog typo fix.

[Alexey Shvayka]

(In reply to Saam Barati from comment #24)
> r=me with one comment/question

Thank you for review!

> same comment here. I think we should be ok with just CustomAccessorOrValue.
> Do tests failing just using CustomAccessorOrValue? If so, we should find out
> why.

Only poly prototype __proto__ test (JSTests/stress/intrinsic-getter-with-poly-proto-getter-change.js) fails.
Changing non-intrinsic GetterSetter w/o transition works and is tested by JSTests/stress/redefine-property-{get,set}.js.
I've filed https://bugs.webkit.org/show_bug.cgi?id=214342 and added FIXMEs.

[EWS]

Committed r264574: <https://trac.webkit.org/changeset/264574>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404321 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65776081>

[Yusuke Suzuki]

This change causes 10% regression in Speedometer2/Ember-Debug. Can you take a look?

[Alexey Shvayka]

*** Bug 202139 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

(In reply to Yusuke Suzuki from comment #29)
> This change causes 10% regression in Speedometer2/Ember-Debug. Can you take a look?

Did this ever get looked at?

[Alexey Shvayka]

(In reply to Joseph Pecoraro from comment #31)
> (In reply to Yusuke Suzuki from comment #29)
> > This change causes 10% regression in Speedometer2/Ember-Debug. Can you take a look?
> 
> Did this ever get looked at?

Yes, this regression was fixed in r265642.