[strong] Implement per-object restrictions behaviour of delete operator

[strong] Implement per-object restrictions behaviour of delete operator

Implements the strong mode proposal's restrictions on the behaviour of the
delete operator for strong objects.

Setting the strong bit is still wip, so this change will only affect those
objects that have the bit correctly set. The tests reflect this, and will be
expanded as more objects can be marked as strong.

Attempt 2, last version did not work with API.

BUG=v8:3956
LOG=N

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

[conradw, 2015-05-21 22:48:58]

conradw@chromium.org changed reviewers:
+ arv@chromium.org, rossberg@chromium.org

[conradw, 2015-05-21 22:48:58]

PTAL

[rossberg, 2015-05-22 11:34:51]

https://codereview.chromium.org/1156573002/diff/40001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1156573002/diff/40001/src/messages.h#newcode229
src/messages.h:229: T(StrongDeleteProperty,                                     
                \
Perhaps we should stylise strong object error messages consistently. E.g. always
always formulating them akin to

"On strong object %, property % cannot be deleted"

or something.

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

https://codereview.chromium.org/1156573002/diff/40001/src/objects.cc#newcode5519
src/objects.cc:5519: THROW_NEW_ERROR(
Why do we need to throw here? Returning false should already cause an exception
if invoked in strict mode.

https://codereview.chromium.org/1156573002/diff/40001/src/objects.cc#newcode5542
src/objects.cc:5542: THROW_NEW_ERROR(
Same here.

https://codereview.chromium.org/1156573002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1156573002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21111: TEST(StrongModeObjectDelete) {
Nit: StrongObjectDelete (especially since the delete happens outside the mode)

https://codereview.chromium.org/1156573002/diff/40001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-delete.js (right):

https://codereview.chromium.org/1156573002/diff/40001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-delete.js:21: // Strong functions can't have
properties added to them, and will be tested as a
Instead of this, why not make the test below check with Object.isExtensible, and
skip the respective bits if false?

https://codereview.chromium.org/1156573002/diff/40001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-delete.js:35: return delete o["foo"];
Change this to an actual computation (e.g., Date()+""), otherwise it won't be
distinguishable from the previous (same below).

[conradw, 2015-05-22 13:37:57]

https://codereview.chromium.org/1156573002/diff/40001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1156573002/diff/40001/src/messages.h#newcode229
src/messages.h:229: T(StrongDeleteProperty,                                     
                \
On 2015/05/22 11:34:50, rossberg wrote:
> Perhaps we should stylise strong object error messages consistently. E.g.
always
> always formulating them akin to
> 
> "On strong object %, property % cannot be deleted"
> 
> or something.

Done.

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

https://codereview.chromium.org/1156573002/diff/40001/src/objects.cc#newcode5519
src/objects.cc:5519: THROW_NEW_ERROR(
On 2015/05/22 11:34:50, rossberg wrote:
> Why do we need to throw here? Returning false should already cause an
exception
> if invoked in strict mode.

Discussed offline, throw is needed but is in wrong place.

https://codereview.chromium.org/1156573002/diff/40001/src/objects.cc#newcode5542
src/objects.cc:5542: THROW_NEW_ERROR(
On 2015/05/22 11:34:50, rossberg wrote:
> Same here.

See above.

https://codereview.chromium.org/1156573002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1156573002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21111: TEST(StrongModeObjectDelete) {
On 2015/05/22 11:34:50, rossberg wrote:
> Nit: StrongObjectDelete (especially since the delete happens outside the mode)

Done.

https://codereview.chromium.org/1156573002/diff/40001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-delete.js (right):

https://codereview.chromium.org/1156573002/diff/40001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-delete.js:21: // Strong functions can't have
properties added to them, and will be tested as a
On 2015/05/22 11:34:50, rossberg wrote:
> Instead of this, why not make the test below check with Object.isExtensible,
and
> skip the respective bits if false?

It's kind of nasty because I interleave asserts that the property still exists
after each attempted delete. I didn't want to introduce a lot of logic just for
this one case. The actual special case test is just one extra line.

https://codereview.chromium.org/1156573002/diff/40001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-delete.js:35: return delete o["foo"];
On 2015/05/22 11:34:50, rossberg wrote:
> Change this to an actual computation (e.g., Date()+""), otherwise it won't be
> distinguishable from the previous (same below).

Done. Added as another case (want to keep both of these in case we do something
crazy with [] later).

[rossberg, 2015-05-22 14:01:26]

https://codereview.chromium.org/1156573002/diff/60001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1156573002/diff/60001/src/elements.cc#newcode...
src/elements.cc:1405: dictionary, entry, obj->map()->is_strong());
Do we have to pass down the flag here? It seems that if the object is strong,
there is no point in even making the call, and you can immediately set the
result to false right here.

https://codereview.chromium.org/1156573002/diff/60001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-delete.js (right):

https://codereview.chromium.org/1156573002/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-delete.js:115: assertTrue(delete o);
I'm confused, shouldn't this be a SyntaxError in strict mode?

https://codereview.chromium.org/1156573002/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-delete.js:148: testStrongObjectDelete();
It seems we don't have tests for redundant deletes succeeding.

[conradw, 2015-05-27 18:09:33]

https://codereview.chromium.org/1156573002/diff/60001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1156573002/diff/60001/src/elements.cc#newcode...
src/elements.cc:1405: dictionary, entry, obj->map()->is_strong());
On 2015/05/22 14:01:26, rossberg wrote:
> Do we have to pass down the flag here? It seems that if the object is strong,
> there is no point in even making the call, and you can immediately set the
> result to false right here.

Yes, missed this.

https://codereview.chromium.org/1156573002/diff/60001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-delete.js (right):

https://codereview.chromium.org/1156573002/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-delete.js:115: assertTrue(delete o);
On 2015/05/22 14:01:26, rossberg wrote:
> I'm confused, shouldn't this be a SyntaxError in strict mode?

Yes, mistake here. I can't actually run the strongObjects part of the tests just
now due to the Array/map caching problems. Will re-write tests when rebasing on
top of fix for that.

https://codereview.chromium.org/1156573002/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-delete.js:148: testStrongObjectDelete();
On 2015/05/22 14:01:26, rossberg wrote:
> It seems we don't have tests for redundant deletes succeeding.

Done, I thought they should fail (and should have tested for that anyway).

[conradw, 2015-05-27 18:11:31]

On 2015/05/27 18:09:33, conradw wrote:
> https://codereview.chromium.org/1156573002/diff/60001/src/elements.cc
> File src/elements.cc (right):
> 
>
https://codereview.chromium.org/1156573002/diff/60001/src/elements.cc#newcode...
> src/elements.cc:1405: dictionary, entry, obj->map()->is_strong());
> On 2015/05/22 14:01:26, rossberg wrote:
> > Do we have to pass down the flag here? It seems that if the object is
strong,
> > there is no point in even making the call, and you can immediately set the
> > result to false right here.
> 
> Yes, missed this.
> 
>
https://codereview.chromium.org/1156573002/diff/60001/test/mjsunit/strong/obj...
> File test/mjsunit/strong/object-delete.js (right):
> 
>
https://codereview.chromium.org/1156573002/diff/60001/test/mjsunit/strong/obj...
> test/mjsunit/strong/object-delete.js:115: assertTrue(delete o);
> On 2015/05/22 14:01:26, rossberg wrote:
> > I'm confused, shouldn't this be a SyntaxError in strict mode?
> 
> Yes, mistake here. I can't actually run the strongObjects part of the tests
just
> now due to the Array/map caching problems. Will re-write tests when rebasing
on
> top of fix for that.
> 
>
https://codereview.chromium.org/1156573002/diff/60001/test/mjsunit/strong/obj...
> test/mjsunit/strong/object-delete.js:148: testStrongObjectDelete();
> On 2015/05/22 14:01:26, rossberg wrote:
> > It seems we don't have tests for redundant deletes succeeding.
> 
> Done, I thought they should fail (and should have tested for that anyway).

Caching problems have now been fixed.

[arv (Not doing code reviews), 2015-05-28 15:46:17]

LGTM

Just a few style nits.

If Toon is around you should probably have him review this too since most of the
changes are related to the runtime.

https://codereview.chromium.org/1156573002/diff/140001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1156573002/diff/140001/src/elements.cc#newcod...
src/elements.cc:940: if (obj->map()->is_strong() &&
!backing_store->is_the_hole(key)) {
When do we holes in the backing store of a strong object?

https://codereview.chromium.org/1156573002/diff/140001/test/mjsunit/strong/ob...
File test/mjsunit/strong/object-delete.js (right):

https://codereview.chromium.org/1156573002/diff/140001/test/mjsunit/strong/ob...
test/mjsunit/strong/object-delete.js:53: with(o) {
with (

https://codereview.chromium.org/1156573002/diff/140001/test/mjsunit/strong/ob...
test/mjsunit/strong/object-delete.js:128: let deletePropertyFuncsSloppy =
[deleteFromObjectSloppy,
let deletePropertyFuncsSloppy = [
  deleteFromObjectSloppy,
  deleteFromObjectKeyedSloppy,
  ...
];

https://codereview.chromium.org/1156573002/diff/140001/test/mjsunit/strong/ob...
test/mjsunit/strong/object-delete.js:171: [{configurable:true, value:"bar"},
{configurable: true, value: "bar"},

[conradw, 2015-05-28 16:36:43]

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

[conradw, 2015-05-28 16:36:44]

https://codereview.chromium.org/1156573002/diff/140001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1156573002/diff/140001/src/elements.cc#newcod...
src/elements.cc:940: if (obj->map()->is_strong() &&
!backing_store->is_the_hole(key)) {
On 2015/05/28 15:46:17, arv wrote:
> When do we holes in the backing store of a strong object?

The elements backing store can become some flavour of holey array, so this is
needed to make sure deleting a missing property doesn't reject.

I've just realised this is in the wrong place though, if key is >length, this
trips an assert, so I've fixed that.

https://codereview.chromium.org/1156573002/diff/140001/test/mjsunit/strong/ob...
File test/mjsunit/strong/object-delete.js (right):

https://codereview.chromium.org/1156573002/diff/140001/test/mjsunit/strong/ob...
test/mjsunit/strong/object-delete.js:53: with(o) {
On 2015/05/28 15:46:17, arv wrote:
> with (

Done.

https://codereview.chromium.org/1156573002/diff/140001/test/mjsunit/strong/ob...
test/mjsunit/strong/object-delete.js:128: let deletePropertyFuncsSloppy =
[deleteFromObjectSloppy,
On 2015/05/28 15:46:17, arv wrote:
> let deletePropertyFuncsSloppy = [
>   deleteFromObjectSloppy,
>   deleteFromObjectKeyedSloppy,
>   ...
> ];

Done.

https://codereview.chromium.org/1156573002/diff/140001/test/mjsunit/strong/ob...
test/mjsunit/strong/object-delete.js:171: [{configurable:true, value:"bar"},
On 2015/05/28 15:46:17, arv wrote:
> {configurable: true, value: "bar"},

Done.

[conradw, 2015-06-01 10:57:21]

The CQ bit was checked by conradw@chromium.org

[conradw, 2015-06-01 10:57:22]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1156573002/#ps160001
(title: "cl feedback")

[commit-bot: I haz the power, 2015-06-01 10:57:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156573002/160001

[commit-bot: I haz the power, 2015-06-01 11:39:06]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-06-01 11:39:26]

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