Array.prototype.slice should only normalize result if it's an array

Array.prototype.slice should only normalize result if it's an array

R=cbruni@chromium.org
BUG=chromium:618788
Committed: https://crrev.com/56ea2f973133a26998f6ba2cc1fb35a0fa679835
Cr-Commit-Position: refs/heads/master@{#36955}

[adamk, 2016-06-10 09:00:19]

https://codereview.chromium.org/2058013002/diff/1/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/2058013002/diff/1/src/js/array.js#newcode732
src/js/array.js:732: %NormalizeElements(deleted_elements);
I should put the same guard here, but I haven't yet found a way to get
deleted_elements to be a TypedArray at this point in the code.

[Camillo Bruni, 2016-06-10 14:04:25]

https://codereview.chromium.org/2058013002/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-crbug-618788.js (right):

https://codereview.chromium.org/2058013002/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-crbug-618788.js:8: assertThrows(() => a.slice());
Can you convince me that we have to throw here?
Normalization is certainly wrong, but slice should just work on any array-like
receiver, right?

[adamk, 2016-06-10 16:33:18]

https://codereview.chromium.org/2058013002/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-crbug-618788.js (right):

https://codereview.chromium.org/2058013002/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-crbug-618788.js:8: assertThrows(() => a.slice());
On 2016/06/10 14:04:25, Camillo Bruni wrote:
> Can you convince me that we have to throw here?
> Normalization is certainly wrong, but slice should just work on any array-like
> receiver, right?

Oh, this is throwing for perfectly "good" reasons (slice fails to set 'length'
on the Int32Array it creates). Would a comment suffice?

[Camillo Bruni, 2016-06-12 12:35:45]

https://codereview.chromium.org/2058013002/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-crbug-618788.js (right):

https://codereview.chromium.org/2058013002/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-crbug-618788.js:8: assertThrows(() => a.slice());
humm, the above code works in FF though (yes my ultimate standard).
Shouldn't then Array.prototype.slice.call(new Int32Array(10)) fail as well (that
currently works just fine)?

[adamk, 2016-06-13 12:16:21]

https://codereview.chromium.org/2058013002/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-crbug-618788.js (right):

https://codereview.chromium.org/2058013002/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-crbug-618788.js:8: assertThrows(() => a.slice());
On 2016/06/12 12:35:45, Camillo Bruni wrote:
> humm, the above code works in FF though (yes my ultimate standard).
> Shouldn't then Array.prototype.slice.call(new Int32Array(10)) fail as well
(that
> currently works just fine)?

From offline discussion: I don't know why Firefox works, but it shouldn't: the
last step of slice is to attempt to set the value of the 'length' property, and
this will throw for TypedArrays (since they have a getter but no setter).

As for the .call() version, that hits the !isArray case of ArraySpeciesCreate,
and so a normal array is returned. The special thing about this test case is
that it passes isArray, but because we've messed with its 'constructor'
property, which causes slice to construct the TypedArray instead of an Array.

[Camillo Bruni, 2016-06-14 09:01:04]

convinced beyond reasonable doubts => LGTM

[adamk, 2016-06-14 09:10:02]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-06-14 09:10:08]

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

[commit-bot: I haz the power, 2016-06-14 09:37:59]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-14 09:38:01]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-14 09:39:29]

Description was changed from

==========
Array.prototype.slice should only normalize result if it's an array

R=cbruni@chromium.org
BUG=chromium:618788
==========

to

==========
Array.prototype.slice should only normalize result if it's an array

R=cbruni@chromium.org
BUG=chromium:618788

Committed: https://crrev.com/56ea2f973133a26998f6ba2cc1fb35a0fa679835
Cr-Commit-Position: refs/heads/master@{#36955}
==========

[commit-bot: I haz the power, 2016-06-14 09:39:30]

Patchset 1 (id:??) landed as
https://crrev.com/56ea2f973133a26998f6ba2cc1fb35a0fa679835
Cr-Commit-Position: refs/heads/master@{#36955}