ClearNonLiveTransitions has to hold on to non-map values.

ClearNonLiveTransitions has to hold on to non-map values.
This ensures that we don't accidentally throw away getters and/or setters that are still needed. To make sure the bug gets triggered, we have to construct a situation where the map is on the live side of a live->non-live transition. This ensures that the map is passed to ClearNonLiveTransitions.

BUG=v8:2163
TEST=test/mjsunit/regress/regress-2163.js

Committed: https://code.google.com/p/v8/source/detail?r=11713

[Michael Starzinger, 2012-06-05 09:12:48]

Nice catch! Small drive-by comment.

https://chromiumcodereview.appspot.com/10535004/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10535004/diff/1/src/objects.cc#newcode...
src/objects.cc:7403: *keep_entry = true;
I don't think this fix is completely correct, because in the ELEMENTS_TRANSITION
case we might have a slot in the backing store that is undefined. In this case
the backing store might be kept even though it is empty.

[Toon Verwaest, 2012-06-05 10:29:16]

Addressed Michael's comment. Please take a look.

https://chromiumcodereview.appspot.com/10535004/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10535004/diff/1/src/objects.cc#newcode...
src/objects.cc:7403: *keep_entry = true;
On 2012/06/05 09:12:48, Michael Starzinger wrote:
> I don't think this fix is completely correct, because in the
ELEMENTS_TRANSITION
> case we might have a slot in the backing store that is undefined. In this case
> the backing store might be kept even though it is empty.

Done.

[Sven Panne, 2012-06-05 10:52:01]

LGTM with nits.

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc#newcode7403
src/objects.cc:7403: if (Marking::MarkBitFrom(map).Get()) {
Nit: To be more consistent with the rest of the code, remove the braces and put
the return on the same line.

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc#newcode7422
src/objects.cc:7422: bool keep_entry = false;
Move the initialization to the CALLBACKS case, this is more local.

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc#newcode7438
src/objects.cc:7438: if (ClearBackPointer(heap, array->get(j))) {
Just use "target", it's cleaner... ;-)

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc#newcode7453
src/objects.cc:7453: Object* setter = accessors->setter();
Move this down before "if (setter->IsMap())...", again making everything as
local as possible.

[Toon Verwaest, 2012-06-05 11:23:27]

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc#newcode7403
src/objects.cc:7403: if (Marking::MarkBitFrom(map).Get()) {
On 2012/06/05 10:52:01, Sven Panne wrote:
> Nit: To be more consistent with the rest of the code, remove the braces and
put
> the return on the same line.

Done.

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc#newcode7422
src/objects.cc:7422: bool keep_entry = false;
On 2012/06/05 10:52:01, Sven Panne wrote:
> Move the initialization to the CALLBACKS case, this is more local.

Done.

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc#newcode7438
src/objects.cc:7438: if (ClearBackPointer(heap, array->get(j))) {
On 2012/06/05 10:52:01, Sven Panne wrote:
> Just use "target", it's cleaner... ;-)

Done.

http://codereview.chromium.org/10535004/diff/4001/src/objects.cc#newcode7453
src/objects.cc:7453: Object* setter = accessors->setter();
On 2012/06/05 10:52:01, Sven Panne wrote:
> Move this down before "if (setter->IsMap())...", again making everything as
> local as possible.

Done.