Fix ArrayConcat for JSValues/JSFunctions/JSRegExps with @@isConcatSpreadable

Fix ArrayConcat for JSValues/JSFunctions/JSRegExps with @@isConcatSpreadable

LOG=N
BUG=v8:3764, v8:3765
R=dslomov@chromium.org

Committed: https://crrev.com/525385ab50d5173a514148fce875012018b6a2c1
Cr-Commit-Position: refs/heads/master@{#25855}

[caitp (gmail), 2014-12-16 06:14:36]

caitpotter88@gmail.com changed reviewers:
+ dslomov@chromium.org

[caitp (gmail), 2014-12-16 06:16:26]

https://codereview.chromium.org/799803003/diff/1/src/runtime/runtime-array.cc
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/799803003/diff/1/src/runtime/runtime-array.cc...
src/runtime/runtime-array.cc:462: // Other wrapped types are not array-like.
I don't think this is really the right thing to do --- but other wrapper types
will simply crash here if their length value is greater than their elements
count, which is wrong.

[Dmitry Lomov (no reviews), 2014-12-16 07:27:30]

https://codereview.chromium.org/799803003/diff/1/src/runtime/runtime-array.cc
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/799803003/diff/1/src/runtime/runtime-array.cc...
src/runtime/runtime-array.cc:462: // Other wrapped types are not array-like.
On 2014/12/16 06:16:25, caitp wrote:
> I don't think this is really the right thing to do --- but other wrapper types
> will simply crash here if their length value is greater than their elements
> count, which is wrong.

The following needs to be made to work:
Number.prototype[Symbol.isConcatSpreadable] = true;
Number.prototype[0] = 1;
Number.prototype[1] = 2;
Number.prototype.length = 2;
[].concat(new Number(42))
In fact, I am surprised you need to do anything special for String wrappers
here. If you do, you should have issues with other non-exotic objects as well.

[caitp (gmail), 2014-12-16 13:04:12]

On 2014/12/16 07:27:30, Dmitry Lomov (chromium) wrote:
> https://codereview.chromium.org/799803003/diff/1/src/runtime/runtime-array.cc
> File src/runtime/runtime-array.cc (right):
> 
>
https://codereview.chromium.org/799803003/diff/1/src/runtime/runtime-array.cc...
> src/runtime/runtime-array.cc:462: // Other wrapped types are not array-like.
> On 2014/12/16 06:16:25, caitp wrote:
> > I don't think this is really the right thing to do --- but other wrapper
types
> > will simply crash here if their length value is greater than their elements
> > count, which is wrong.
> 
> The following needs to be made to work:
> Number.prototype[Symbol.isConcatSpreadable] = true;
> Number.prototype[0] = 1;
> Number.prototype[1] = 2;
> Number.prototype.length = 2;
> [].concat(new Number(42))
> In fact, I am surprised you need to do anything special for String wrappers
> here. If you do, you should have issues with other non-exotic objects as well.

There are, and there are still issues for Strings too, unfortunately. will look
more into it today

[caitp (gmail), 2014-12-16 14:46:00]

On 2014/12/16 13:04:12, caitp wrote:
> On 2014/12/16 07:27:30, Dmitry Lomov (chromium) wrote:
> >
https://codereview.chromium.org/799803003/diff/1/src/runtime/runtime-array.cc
> > File src/runtime/runtime-array.cc (right):
> > 
> >
>
https://codereview.chromium.org/799803003/diff/1/src/runtime/runtime-array.cc...
> > src/runtime/runtime-array.cc:462: // Other wrapped types are not array-like.
> > On 2014/12/16 06:16:25, caitp wrote:
> > > I don't think this is really the right thing to do --- but other wrapper
> types
> > > will simply crash here if their length value is greater than their
elements
> > > count, which is wrong.
> > 
> > The following needs to be made to work:
> > Number.prototype[Symbol.isConcatSpreadable] = true;
> > Number.prototype[0] = 1;
> > Number.prototype[1] = 2;
> > Number.prototype.length = 2;
> > [].concat(new Number(42))
> > In fact, I am surprised you need to do anything special for String wrappers
> > here. If you do, you should have issues with other non-exotic objects as
well.
> 
> There are, and there are still issues for Strings too, unfortunately. will
look
> more into it today

I think this version works a bit better, but there are so many potentially buggy
types to handle here.

[Dmitry Lomov (no reviews), 2014-12-16 15:25:45]

https://codereview.chromium.org/799803003/diff/80001/src/runtime/runtime-arra...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/799803003/diff/80001/src/runtime/runtime-arra...
src/runtime/runtime-array.cc:493: if (receiver->elements() ==
isolate->heap()->empty_fixed_array()) {
Let's aim for 'make it right, then make it fast' approach.
Let's go down this route for all 'unsafe' receivers.

[caitp (gmail), 2014-12-16 15:33:37]

https://codereview.chromium.org/799803003/diff/80001/src/runtime/runtime-arra...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/799803003/diff/80001/src/runtime/runtime-arra...
src/runtime/runtime-array.cc:493: if (receiver->elements() ==
isolate->heap()->empty_fixed_array()) {
On 2014/12/16 15:25:45, Dmitry Lomov (chromium) wrote:
> Let's aim for 'make it right, then make it fast' approach.
> Let's go down this route for all 'unsafe' receivers.

So more of an `if (!(receiver->IsJSArray() || receiver->IsJSTypedArray()))`
thing?

[Dmitry Lomov (no reviews), 2014-12-16 18:21:54]

On 2014/12/16 15:33:37, caitp wrote:
>
https://codereview.chromium.org/799803003/diff/80001/src/runtime/runtime-arra...
> File src/runtime/runtime-array.cc (right):
> 
>
https://codereview.chromium.org/799803003/diff/80001/src/runtime/runtime-arra...
> src/runtime/runtime-array.cc:493: if (receiver->elements() ==
> isolate->heap()->empty_fixed_array()) {
> On 2014/12/16 15:25:45, Dmitry Lomov (chromium) wrote:
> > Let's aim for 'make it right, then make it fast' approach.
> > Let's go down this route for all 'unsafe' receivers.
> 
> So more of an `if (!(receiver->IsJSArray() || receiver->IsJSTypedArray()))`
> thing?

Yes. Be careful with typed arrays though, their length can be faked with
Object.defineProperty:
   var a = new Uint8Array(10);
   Object.defineProperty(a, 'length', { value: 256 });
   a.length // 256

[Dmitry Lomov (no reviews), 2014-12-16 19:37:57]

lgtm

[caitp (gmail), 2014-12-16 19:38:48]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2014-12-16 19:39:31]

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

[commit-bot: I haz the power, 2014-12-16 20:04:23]

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

[commit-bot: I haz the power, 2014-12-16 20:04:23]

Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1807)

[caitp (gmail), 2014-12-16 20:29:13]

On 2014/12/16 20:04:23, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   v8_linux_arm64_rel on tryserver.v8
>
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1807)

The big TypedArray tests I added hang on arm, shrinking those down a bit...

[caitp (gmail), 2014-12-16 20:31:15]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2014-12-16 20:31:52]

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

[commit-bot: I haz the power, 2014-12-16 21:08:19]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-12-17 10:14:41]

Patchset 11 (id:??) landed as
https://crrev.com/525385ab50d5173a514148fce875012018b6a2c1
Cr-Commit-Position: refs/heads/master@{#25855}