[elements] Handlify SloppyArguments IndexOfValueImpl

[elements] Handlify SloppyArguments IndexOfValueImpl

The raw pointer to the parameter_map might get stale in case of accessors present on
the arguments object.
Drive-by-fix: use nullptr instead of the_hole with isolate access.

BUG=chromium:645680
Committed: https://crrev.com/621f4af7200ef76bfafa671efa6121bb9b6fb630
Cr-Commit-Position: refs/heads/master@{#39359}

[Camillo Bruni, 2016-09-10 02:30:57]

Description was changed from

==========
[elements] Handlify SloppyArguments IndexOfValueImpl

Raw pointer to the parameter_map might get stale in case of accessors present on
the arguments object.

BUG=chromium:645680
==========

to

==========
[elements] Handlify SloppyArguments IndexOfValueImpl

Raw pointer to the parameter_map might get stale in case of accessors present on
the arguments object.
Drive-by-fix: use nullptr instead of the_hole with isolate access.


BUG=chromium:645680
==========

[Camillo Bruni, 2016-09-10 02:31:22]

Description was changed from

==========
[elements] Handlify SloppyArguments IndexOfValueImpl

Raw pointer to the parameter_map might get stale in case of accessors present on
the arguments object.
Drive-by-fix: use nullptr instead of the_hole with isolate access.


BUG=chromium:645680
==========

to

==========
[elements] Handlify SloppyArguments IndexOfValueImpl

The raw pointer to the parameter_map might get stale in case of accessors
present on
the arguments object.
Drive-by-fix: use nullptr instead of the_hole with isolate access.


BUG=chromium:645680
==========

[Camillo Bruni, 2016-09-10 02:32:43]

cbruni@chromium.org changed reviewers:
+ caitp@igalia.com

[Camillo Bruni, 2016-09-10 02:32:44]

PTAL

[Camillo Bruni, 2016-09-10 02:33:15]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 02:33:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 02:45:25]

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

[commit-bot: I haz the power, 2016-09-10 02:45:26]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[caitp, 2016-09-10 02:48:37]

lgtm

https://codereview.chromium.org/2332503002/diff/40001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/2332503002/diff/40001/src/elements.cc#newcode...
src/elements.cc:2998: return index < length ? parameter_map->get(index + 2) :
nullptr;
Looks good I think, but I think there are more consumers of this in elements.cc
that need adjustment

https://codereview.chromium.org/2332503002/diff/40001/test/mjsunit/array-inde...
File test/mjsunit/array-indexing-receiver.js (right):

https://codereview.chromium.org/2332503002/diff/40001/test/mjsunit/array-inde...
test/mjsunit/array-indexing-receiver.js:110: Object.defineProperty(array, 4, {
get() { gc(); return NaN; } });
Adding `gc` all over the place seems like it could make these tests unrealistic,
and reduce coverage. Don't we get this same-ish behaviour for variants with a
reduced gc threshold or whatever? If we need properties which are guaranteed to
cause a gc on access, maybe that should be in addition to, rather than
replacing, the existing ones.

If you think this is still representative of real world JS, then it's fine by me

[Camillo Bruni, 2016-09-10 19:48:24]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 19:48:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 20:11:49]

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

[commit-bot: I haz the power, 2016-09-10 20:11:50]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-09-12 16:59:07]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-09-12 16:59:07]

The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com
Link to the patchset: https://codereview.chromium.org/2332503002/#ps60001
(title: "use bool instead of nullptr")

[commit-bot: I haz the power, 2016-09-12 16:59:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 17:31:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-12 17:32:13]

Description was changed from

==========
[elements] Handlify SloppyArguments IndexOfValueImpl

The raw pointer to the parameter_map might get stale in case of accessors
present on
the arguments object.
Drive-by-fix: use nullptr instead of the_hole with isolate access.


BUG=chromium:645680
==========

to

==========
[elements] Handlify SloppyArguments IndexOfValueImpl

The raw pointer to the parameter_map might get stale in case of accessors
present on
the arguments object.
Drive-by-fix: use nullptr instead of the_hole with isolate access.

BUG=chromium:645680

Committed: https://crrev.com/621f4af7200ef76bfafa671efa6121bb9b6fb630
Cr-Commit-Position: refs/heads/master@{#39359}
==========

[commit-bot: I haz the power, 2016-09-12 17:32:14]

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

[Camillo Bruni, 2016-09-14 16:48:54]

https://codereview.chromium.org/2332503002/diff/40001/test/mjsunit/array-inde...
File test/mjsunit/array-indexing-receiver.js (right):

https://codereview.chromium.org/2332503002/diff/40001/test/mjsunit/array-inde...
test/mjsunit/array-indexing-receiver.js:110: Object.defineProperty(array, 4, {
get() { gc(); return NaN; } });
On 2016/09/10 at 02:48:37, caitp wrote:
> Adding `gc` all over the place seems like it could make these tests
unrealistic, and reduce coverage. Don't we get this same-ish behaviour for
variants with a reduced gc threshold or whatever? If we need properties which
are guaranteed to cause a gc on access, maybe that should be in addition to,
rather than replacing, the existing ones.
> 
> If you think this is still representative of real world JS, then it's fine by
me

the --gc-interval trick only works if there is an allocation happening in
between.
Internally, causing a gc() is probably the worst kind of side-effect you can
cause.
Given that we don't rely on optimizations in this test I don't think that this
causes any unwanted changes to the test.