Protect the emptiness of Array prototype elements with a PropertyCell.

Protect the emptiness of Array prototype elements with a PropertyCell.

Not just emptiness, but also a particular structure.

BUG=v8:4044
LOG=N

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

[mvstanton, 2015-04-21 14:33:40]

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

[mvstanton, 2015-04-21 14:33:41]

Hi Jakob, would you have a look at this follow-on CL? This expands the empty
Array.prototype protection across contexts. It also makes the protection state
queryable through the value of a cell (could become useful in ic handlers).

The meaning of the cell having the value 1 is that:
a) there are no elements in the array or object prototype chain,
b) the array.prototype's prototype is the object prototype,

If this condition is broken in any context, it's considered broken in all
contexts.
Thanks,
--Michael

[Jakob Kummerow, 2015-04-21 15:41:49]

LGTM with nits.

https://codereview.chromium.org/1092043002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1092043002/diff/60001/src/isolate.h#newcode1023
src/isolate.h:1023: void MaintainArrayProtectorOnSetElement(Handle<JSObject>
object);
naming nit: "Maintain" doesn't sound like something is about to change. How
about "Update" or "Invalidate"?

https://codereview.chromium.org/1092043002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1092043002/diff/60001/src/objects.cc#newcode1...
src/objects.cc:17108: auto isolate = cell->GetIsolate();
nit: s/auto/Isolate*/ please?

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

https://codereview.chromium.org/1092043002/diff/60001/test/cctest/test-api.cc...
test/cctest/test-api.cc:16645: printf("script: %s\n", script);
nit: debugging leftover?

https://codereview.chromium.org/1092043002/diff/60001/test/cctest/test-api.cc...
test/cctest/test-api.cc:16661: DCHECK_EQ(true,
i_isolate->IsFastArrayConstructorPrototypeChainIntact());
s/DCHECK/CHECK/, and again below?

[mvstanton, 2015-04-22 07:22:33]

Thanks Jakob, addressed issues.
And, a release mode test failure caused me to add a TODO and --nostress-opt on a
test.

The test expects to deploy the NaN-as-hole optimization, repeatedly, once per
stress run. But since we don't create a new isolate it isn't possible anymore.
That is, future runs will cause compilation that is safer, and avoids the
optimization because the protector cell was polluted.

The TODO is to maybe provide a runtime function for manually reset of this cell,
if it's important enough. But since only one test hits the issue it may not be
yet.
--Michael

https://codereview.chromium.org/1092043002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1092043002/diff/60001/src/isolate.h#newcode1023
src/isolate.h:1023: void MaintainArrayProtectorOnSetElement(Handle<JSObject>
object);
On 2015/04/21 15:41:49, Jakob wrote:
> naming nit: "Maintain" doesn't sound like something is about to change. How
> about "Update" or "Invalidate"?

Good idea, I'll go with Update.

https://codereview.chromium.org/1092043002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1092043002/diff/60001/src/objects.cc#newcode1...
src/objects.cc:17108: auto isolate = cell->GetIsolate();
On 2015/04/21 15:41:49, Jakob wrote:
> nit: s/auto/Isolate*/ please?

Done.

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

https://codereview.chromium.org/1092043002/diff/60001/test/cctest/test-api.cc...
test/cctest/test-api.cc:16645: printf("script: %s\n", script);
On 2015/04/21 15:41:49, Jakob wrote:
> nit: debugging leftover?

Done.

https://codereview.chromium.org/1092043002/diff/60001/test/cctest/test-api.cc...
test/cctest/test-api.cc:16661: DCHECK_EQ(true,
i_isolate->IsFastArrayConstructorPrototypeChainIntact());
On 2015/04/21 15:41:49, Jakob wrote:
> s/DCHECK/CHECK/, and again below?

Done.

[Jakob Kummerow, 2015-04-22 08:02:20]

Still LGTM.

Adding --nostress-opt to that test is fine.

https://codereview.chromium.org/1092043002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1092043002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:2440: PrintF("INVALIDATING YOU!!!\n");
yeah...

[mvstanton, 2015-04-22 08:20:44]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-04-22 08:20:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1092043002/#ps120001
(title: "Try again :p.")

[commit-bot: I haz the power, 2015-04-22 08:20:51]

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

[commit-bot: I haz the power, 2015-04-22 08:50:36]

Committed patchset #7 (id:120001)

[mvstanton, 2015-04-22 09:39:07]

thx!

https://codereview.chromium.org/1092043002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1092043002/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:2440: PrintF("INVALIDATING YOU!!!\n");
On 2015/04/22 08:02:20, Jakob wrote:
> yeah...

I've discovered a bit of drama livens up the workday.

[mvstanton, 2015-04-22 09:56:18]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1099203004/ by mvstanton@chromium.org.

The reason for reverting is: MAC GCSTRESS failure on new test..

[commit-bot: I haz the power, 2015-04-22 13:19:08]

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