Add C++ implementation of Object.defineProperties

Add C++ implementation of Object.defineProperties

Not used yet, so this CL shouldn't change behavior.

Committed: https://crrev.com/c706c0fa19a08dfa9bd6ccd86e48aced36fb1ded
Cr-Commit-Position: refs/heads/master@{#31241}

[Jakob Kummerow, 2015-10-02 11:35:49]

jkummerow@chromium.org changed reviewers:
+ verwaest@chromium.org

[Jakob Kummerow, 2015-10-02 11:35:50]

This is now ready for review.

[Toon Verwaest, 2015-10-06 15:55:19]

Awesome work so far, this is pretty complicated...
I still have some comments and hints. I'm fine with converting some of the
JSProxy related comments into TODOs; as you want.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5926
src/objects.cc:5926: Maybe<PropertyAttributes> maybe =
JSObject::GetPropertyAttributes(&it);
On proxy props this will call getPropertyDescriptor trap, while we should be
calling getOwnPropertDescriptor trap.

Additionally, we'd have to convert the resulting object (the descriptor
descriptor :)) using ToPropertyDescriptor, which uses HasProperty and Get to
convert the descriptor. The descriptor itself could (probably?) be a proxy
itself, so the correct protocol needs to be in place.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5967
src/objects.cc:5967: Handle<Object> name,
DefineOwnProperty is defined in 9.1.6 to be called with a "property key" P,
meaning "Name" in V8 speak (afaiu). Even assuming it could be a smi, that does
mean that LookupIterator::PropertyOrElement is guaranteed not to throw.

Rename to "key" whenever a smi is also possible (or index in string form for
that matter)

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5974
src/objects.cc:5974: // TODO(jkummerow): Do we need special support for
arguments? (ES6 9.4.4.2)
I don't think so.
For data properties:

DefineOwnPropertyIgnoreAttributes will either AddDataProperty, in which case
isMapped was false and nothing to be done. Otherwise, it'll call into
SloppyArgumentsElementsAccessor::ReconfigureImpl which breaks regular aliasing
but might restore "slow aliasing" in case that the property is still writable.
(Fast aliasing only works for NONE-attribute elements). (Explaining this just
made me realize that reconfiguring after reconfiguring will break the
slow-aliasing by just overwriting the AliasedArgumentsEntry with the new
incoming value. Ugh!).

Or it'll go in DefineAccessor, in which case we unconditionally undo aliasing.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5975
src/objects.cc:5975: // TODO(jkummerow): Do we need special support for
IntegerIndexedExotic?
IntegerIndexedExotic for data properties is already handled by
DefineOwnPropertyIgnoreAttributes. There are no observable operations (with
typed array receiver) between here and that call. If the lookup iterator returns
"INTEGER_INDEXED_EXOTIC" it basically means it's in any of the out-of-range
"false" cases defined in 9.4.5.3, and will throw using
RedefineNonconfigurableProperty. In the in-range case, but invalid incoming
descriptor, we are in the DATA special case that handles
FixedTypedArrayElements, and again will throw using
RedefineNonconfigurableProperty.

(not sure what this means for |should_throw| however).

The DefineAccessor part as currently in the tree just returns undefined,
however. That should return false as well (throw). (Firefox swallows this as
well through; unless in range, which throws the exception probably because it's
non-configurable...). Other than that, no action needs to be taken.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5991
src/objects.cc:5991: if (!success) return false;
name -> property_key, so shouldn't happen(?).

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode6289
src/objects.cc:6289: if (value->IsString()) return
String::cast(*value)->AsArrayIndex(length);
Ugh ... :)

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode6400
src/objects.cc:6400: // 1. If the [[Value]] field of Desc is absent, then
Most of this code duplicates the ArrayLengthSetter that would have been called
through DefineOwnPropertyIgnoreAttributes (with DONT_FORCE_FIELD). Can we
somehow avoid this duplication?

(Arg, ArrayLengthSetter, ArraySetLength, SetLength, ObservableSetLength ...)

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode6455
src/objects.cc:6455: //      any elements cannot be deleted.
gee ...

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode6486
src/objects.cc:6486: isolate, obj, name, &success, LookupIterator::HIDDEN);
property key "p" (spec) (name -> key), so cannot fail.

We should probably just add DCHECK(IsPropertyKey(key)) or so.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode6494
src/objects.cc:6494: // "MaybeHandle<Object> GetOwnProperty()" in
runtime-object.cc?
TODO(): proxies: call getOwnPropertyDescriptor trap and convert the result (if
object) with ToPropertyDescriptor.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h#newcode1821
src/objects.h:1821: Handle<Object> o,
Please write out o as object

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h#newcode1822
src/objects.h:1822: Handle<Object> name,
Whenever type is object, I'd rather talk about "key" than "name", since Name =
Symbol | String.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h#newcode1836
src/objects.h:1836: bool should_throw);
replace bool should_throw with enum

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h#newcode1841
src/objects.h:1841: static bool GetOwnPropertyDescriptor(Isolate* isolate,
Handle<JSObject> obj,
obj -> object

https://codereview.chromium.org/1368753003/diff/20001/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1368753003/diff/20001/src/property-descriptor...
src/property-descriptor.cc:121: if (!obj->IsJSProxy()) {
I don't think this is good enough. GetPropertyIfPresent, as the spec requires,
just does HasProperty and Get, which will fetch through the prototype chain.
That will still call into proxy handlers if there's a proxy on the prototype
chain. Actually GetPropertyAttributes will result in the wrong trap being
triggered, and this particular check won't prevent that from happening.

https://codereview.chromium.org/1368753003/diff/20001/src/property-descriptor.h
File src/property-descriptor.h (right):

https://codereview.chromium.org/1368753003/diff/20001/src/property-descriptor...
src/property-descriptor.h:11: #include "src/property-details.h"
Can we get away with including less (especially objects.h...) by moving more
code to the .cc file?

[Jakob Kummerow, 2015-10-09 13:46:13]

Thanks for the review, addressed your comment. 

I'd like to defer as much of the Proxy support as possible; this CL is large
enough as it is.
If you want, we can postpone enabling the new implementation until it supports
more cases; however given that proxies are both off by default and rather broken
currently anyway, I'm not sure we have to.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5926
src/objects.cc:5926: Maybe<PropertyAttributes> maybe =
JSObject::GetPropertyAttributes(&it);
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> On proxy props this will call getPropertyDescriptor trap, while we should be
> calling getOwnPropertDescriptor trap.
> 
> Additionally, we'd have to convert the resulting object (the descriptor
> descriptor :)) using ToPropertyDescriptor, which uses HasProperty and Get to
> convert the descriptor. The descriptor itself could (probably?) be a proxy
> itself, so the correct protocol needs to be in place.

Ack. Follow-up -> added TODO comment.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5967
src/objects.cc:5967: Handle<Object> name,
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> DefineOwnProperty is defined in 9.1.6 to be called with a "property key" P,
> meaning "Name" in V8 speak (afaiu). Even assuming it could be a smi, that does
> mean that LookupIterator::PropertyOrElement is guaranteed not to throw.
> 
> Rename to "key" whenever a smi is also possible (or index in string form for
> that matter)

Done.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5974
src/objects.cc:5974: // TODO(jkummerow): Do we need special support for
arguments? (ES6 9.4.4.2)
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> I don't think so.
> For data properties:
> 
> DefineOwnPropertyIgnoreAttributes will either AddDataProperty, in which case
> isMapped was false and nothing to be done. Otherwise, it'll call into
> SloppyArgumentsElementsAccessor::ReconfigureImpl which breaks regular aliasing
> but might restore "slow aliasing" in case that the property is still writable.
> (Fast aliasing only works for NONE-attribute elements). (Explaining this just
> made me realize that reconfiguring after reconfiguring will break the
> slow-aliasing by just overwriting the AliasedArgumentsEntry with the new
> incoming value. Ugh!).
> 
> Or it'll go in DefineAccessor, in which case we unconditionally undo aliasing.

Acknowledged.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5975
src/objects.cc:5975: // TODO(jkummerow): Do we need special support for
IntegerIndexedExotic?
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> IntegerIndexedExotic for data properties is already handled by
> DefineOwnPropertyIgnoreAttributes. There are no observable operations (with
> typed array receiver) between here and that call. If the lookup iterator
returns
> "INTEGER_INDEXED_EXOTIC" it basically means it's in any of the out-of-range
> "false" cases defined in 9.4.5.3, and will throw using
> RedefineNonconfigurableProperty. In the in-range case, but invalid incoming
> descriptor, we are in the DATA special case that handles
> FixedTypedArrayElements, and again will throw using
> RedefineNonconfigurableProperty.
> 
> (not sure what this means for |should_throw| however).
> 
> The DefineAccessor part as currently in the tree just returns undefined,
> however. That should return false as well (throw). (Firefox swallows this as
> well through; unless in range, which throws the exception probably because
it's
> non-configurable...). Other than that, no action needs to be taken.

Acknowledged, changed the comment to reflect that.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode5991
src/objects.cc:5991: if (!success) return false;
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> name -> property_key, so shouldn't happen(?).

Done.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode6289
src/objects.cc:6289: if (value->IsString()) return
String::cast(*value)->AsArrayIndex(length);
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> Ugh ... :)

Acknowledged.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode6400
src/objects.cc:6400: // 1. If the [[Value]] field of Desc is absent, then
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> Most of this code duplicates the ArrayLengthSetter that would have been called
> through DefineOwnPropertyIgnoreAttributes (with DONT_FORCE_FIELD). Can we
> somehow avoid this duplication?

Well, ArrayLengthSetter is mostly concerned with converting the new value to an
array length, which I agree would be nice to unify, see comment in line 6362
above. It's a bit non-trivial, though, and it's only about 20 lines of
duplication, so I don't consider it urgent and would prefer to look into it in a
follow-up. OK?

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode6486
src/objects.cc:6486: isolate, obj, name, &success, LookupIterator::HIDDEN);
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> property key "p" (spec) (name -> key), so cannot fail.
> 
> We should probably just add DCHECK(IsPropertyKey(key)) or so.

Done.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.cc#newcode6494
src/objects.cc:6494: // "MaybeHandle<Object> GetOwnProperty()" in
runtime-object.cc?
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> TODO(): proxies: call getOwnPropertyDescriptor trap and convert the result (if
> object) with ToPropertyDescriptor.

Done.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h#newcode1821
src/objects.h:1821: Handle<Object> o,
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> Please write out o as object

Done. (I was following the spec's naming.)

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h#newcode1822
src/objects.h:1822: Handle<Object> name,
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> Whenever type is object, I'd rather talk about "key" than "name", since Name =
> Symbol | String.

Done.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h#newcode1836
src/objects.h:1836: bool should_throw);
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> replace bool should_throw with enum

Done.

https://codereview.chromium.org/1368753003/diff/20001/src/objects.h#newcode1841
src/objects.h:1841: static bool GetOwnPropertyDescriptor(Isolate* isolate,
Handle<JSObject> obj,
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> obj -> object

Done.

https://codereview.chromium.org/1368753003/diff/20001/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1368753003/diff/20001/src/property-descriptor...
src/property-descriptor.cc:121: if (!obj->IsJSProxy()) {
On 2015/10/06 15:55:18, Toon Verwaest wrote:
> I don't think this is good enough. GetPropertyIfPresent, as the spec requires,
> just does HasProperty and Get, which will fetch through the prototype chain.
> That will still call into proxy handlers if there's a proxy on the prototype
> chain. Actually GetPropertyAttributes will result in the wrong trap being
> triggered, and this particular check won't prevent that from happening.

Acknowledged. I'd prefer to do this in a follow-up CL; I've expanded the TODO
comment to include more detail.

https://codereview.chromium.org/1368753003/diff/20001/src/property-descriptor.h
File src/property-descriptor.h (right):

https://codereview.chromium.org/1368753003/diff/20001/src/property-descriptor...
src/property-descriptor.h:11: #include "src/property-details.h"
On 2015/10/06 15:55:19, Toon Verwaest wrote:
> Can we get away with including less (especially objects.h...) by moving more
> code to the .cc file?

Done. I can forward-declare Isolate and Object, eliminating the need for
objects.h. I don't see a way to get rid of handles.h or property-details.h,
though.

[Toon Verwaest, 2015-10-13 11:51:26]

Looking good, one more comment

https://codereview.chromium.org/1368753003/diff/40001/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1368753003/diff/40001/src/property-descriptor...
src/property-descriptor.cc:45: if (map->prototype() !=
*isolate->initial_object_prototype()) return false;
The properties below could have been added to Object.prototype and
Object.prototype.__proto__ could have been modified? We probably need something
similar to the array_protector used in builtins.cc.

[Jakob Kummerow, 2015-10-13 12:04:43]

https://codereview.chromium.org/1368753003/diff/40001/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1368753003/diff/40001/src/property-descriptor...
src/property-descriptor.cc:45: if (map->prototype() !=
*isolate->initial_object_prototype()) return false;
On 2015/10/13 11:51:26, Toon Verwaest wrote:
> The properties below could have been added to Object.prototype and
> Object.prototype.__proto__ could have been modified? We probably need
something
> similar to the array_protector used in builtins.cc.

Is the map check on the next line not enough to address that concern?

[Toon Verwaest, 2015-10-13 13:32:15]

You're obviously right.
lgtm

https://codereview.chromium.org/1368753003/diff/40001/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1368753003/diff/40001/src/property-descriptor...
src/property-descriptor.cc:45: if (map->prototype() !=
*isolate->initial_object_prototype()) return false;
On 2015/10/13 12:04:43, Jakob wrote:
> On 2015/10/13 11:51:26, Toon Verwaest wrote:
> > The properties below could have been added to Object.prototype and
> > Object.prototype.__proto__ could have been modified? We probably need
> something
> > similar to the array_protector used in builtins.cc.
> 
> Is the map check on the next line not enough to address that concern?

Acknowledged.

[Jakob Kummerow, 2015-10-13 13:34:14]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2015-10-13 13:34:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368753003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368753003/40001

[commit-bot: I haz the power, 2015-10-13 13:36:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-13 13:36:46]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6598)

[Michael Lippautz, 2015-10-13 13:49:08]

mlippautz@chromium.org changed reviewers:
+ mlippautz@chromium.org

[Michael Lippautz, 2015-10-13 13:49:09]

heap/* lgtm

[Jakob Kummerow, 2015-10-13 14:09:00]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2015-10-13 14:09:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlippautz@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1368753003/#ps60001
(title: "make compilers happy")

[commit-bot: I haz the power, 2015-10-13 14:09:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368753003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368753003/60001

[commit-bot: I haz the power, 2015-10-13 14:38:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-13 14:38:53]

Patchset 4 (id:??) landed as
https://crrev.com/c706c0fa19a08dfa9bd6ccd86e48aced36fb1ded
Cr-Commit-Position: refs/heads/master@{#31241}