Fix another corner-case behavior of Object::SetSuperProperty.

Fix another corner-case behavior of Object::SetSuperProperty.

If the property is a data property on the holder (or does not exist) and is a readonly data property in the receiver, then we must fail.

R=rossberg, verwaest@chromium.org
BUG=

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

[rossberg, 2015-11-03 10:55:11]

https://codereview.chromium.org/1424233005/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1424233005/diff/1/src/objects.cc#newcode3773
src/objects.cc:3773: if (!JSObject::IsExtensible(
I don't think this check is correct. Extensibility doesn't matter if the
property already exists.

[neis, 2015-11-03 11:03:25]

Description was changed from

==========
Fix two further corner-case behaviors of Object::SetSuperProperty.

Suppose the property is a data property on the holder (or does not
exist) and is a data property in the receiver.  Then we must fail if the
property is readonly in the receiver or the receiver is not extensible.

R=rossberg, verwaest@chromium.org
BUG=
==========

to

==========
Fix another corner-case behavior of Object::SetSuperProperty.

If the property is a data property on the holder (or does not exist) and is a
readonly data property in the receiver, then we must fail.

R=rossberg, verwaest@chromium.org
BUG=
==========

[neis, 2015-11-03 11:08:52]

On 2015/11/03 10:55:11, rossberg wrote:
> https://codereview.chromium.org/1424233005/diff/1/src/objects.cc
> File src/objects.cc (right):
> 
> https://codereview.chromium.org/1424233005/diff/1/src/objects.cc#newcode3773
> src/objects.cc:3773: if (!JSObject::IsExtensible(
> I don't think this check is correct. Extensibility doesn't matter if the
> property already exists.

Sorry, I misread the spec.
PTAL.

[rossberg, 2015-11-03 11:21:52]

lgtm

[Toon Verwaest, 2015-11-03 12:06:52]

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

https://codereview.chromium.org/1424233005/diff/20001/src/objects.cc#newcode3773
src/objects.cc:3773: &own_lookup, value, details.attributes(), should_throw);
Won't this just SetDataProperty(it, value, THROW_ON_ERROR)?

[neis, 2015-11-03 12:56:07]

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

https://codereview.chromium.org/1424233005/diff/20001/src/objects.cc#newcode3773
src/objects.cc:3773: &own_lookup, value, details.attributes(), should_throw);
On 2015/11/03 12:06:52, Toon Verwaest wrote:
> Won't this just SetDataProperty(it, value, THROW_ON_ERROR)?

Not THROW_ON_ERROR, but yeah, looks like this will always just call
SetDataProperty.  I simplified the code accordingly.  PTAL.

[Toon Verwaest, 2015-11-03 13:18:26]

https://codereview.chromium.org/1424233005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1424233005/diff/40001/src/objects.cc#newcode3772
src/objects.cc:3772: return SetDataProperty(&own_lookup, value, should_throw);
DefineOwnPropertyIgnoreAttributes used to pass in THROW_ON_ERROR, this is a
change. Which made me realize: should_throw doesn't seem to be used by
SetDataProperty?

[neis, 2015-11-03 13:40:52]

On 2015/11/03 13:18:26, Toon Verwaest wrote:
> https://codereview.chromium.org/1424233005/diff/40001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/1424233005/diff/40001/src/objects.cc#newcode3772
> src/objects.cc:3772: return SetDataProperty(&own_lookup, value, should_throw);
> DefineOwnPropertyIgnoreAttributes used to pass in THROW_ON_ERROR, this is a
> change. Which made me realize: should_throw doesn't seem to be used by
> SetDataProperty?

The call to DefineOwnPropertyIgnoreAttributes just forwards its should_throw
argument.  But you're right, SetDataProperty doesn't need it.  New patch set.

[Toon Verwaest, 2015-11-03 14:08:59]

lgtm thanks

[neis, 2015-11-03 14:14:52]

The CQ bit was checked by neis@chromium.org

[neis, 2015-11-03 14:14:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1424233005/#ps60001
(title: "Remove should_throw argument of SetDataProperty.")

[commit-bot: I haz the power, 2015-11-03 14:14:57]

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

[commit-bot: I haz the power, 2015-11-03 14:53:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-03 14:53:15]

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