[typedarrays] Check detached buffer at start of typed array methods

[typedarrays] Check detached buffer at start of typed array methods

- Throw TypeError in ValidateTypedArray, matching JSC, SpiderMonkey
  and ChakraCore.
- Validate typed arrays at start of each typed array prototype
  methods in src/js/typedarrays.js
- Add tests to check detached buffers
- Remove an unnecessary parameter of TypedArraySpeciesCreate
  in src/js/typedarrays.js
- Standardize TypedArray.prototype.subarray
- Update test262.status to pass detached buffer tests

BUG=v8:4648, v8:4665, v8:4953
Review-Url: https://codereview.chromium.org/2778623003
Cr-Commit-Position: refs/heads/master@{#44357}
Committed: https://chromium.googlesource.com/v8/v8/+/238d5b4453d9166aaddce76a5393514d977238d4

[Choongwoo Han, 2017-03-26 06:46:45]

Description was changed from

==========
[typedarrays] Check detached buffer at start of typed array methods

- Throw TypeError in ValidateTypedArray
- Validate typed arrays at start of each typed array prototype
  methods in src/js/typedarrays.js
- Add tests to check detached buffers
- Remove an unnecessary parameter of TypedArraySpeciesCreate
  in src/js/typedarrays.js
- Standardize TypedArray.prototype.subarray
- Update test262.status to pass detached buffer tests

BUG=v8:4648,v8:4665
==========

to

==========
[typedarrays] Check detached buffer at start of typed array methods

- Throw TypeError in ValidateTypedArray
- Validate typed arrays at start of each typed array prototype
  methods in src/js/typedarrays.js
- Add tests to check detached buffers
- Remove an unnecessary parameter of TypedArraySpeciesCreate
  in src/js/typedarrays.js
- Standardize TypedArray.prototype.subarray
- Update test262.status to pass detached buffer tests

BUG=v8:4648,v8:4665
==========

[Choongwoo Han, 2017-03-26 06:46:45]

cwhan.tunz@gmail.com changed reviewers:
+ adamk@chromium.org, littledan@chromium.org

[Choongwoo Han, 2017-03-26 06:47:23]

littledan@, adamk@, Could you take a look?

[adamk, 2017-03-27 19:48:20]

My big concern here is performance due to the added runtime call, see below.

https://codereview.chromium.org/2778623003/diff/1/src/builtins/builtins-typed...
File src/builtins/builtins-typedarray.cc (left):

https://codereview.chromium.org/2778623003/diff/1/src/builtins/builtins-typed...
src/builtins/builtins-typedarray.cc:50: const char* method =
"%TypedArray%.prototype.copyWithin";
Why did this change? "%TypedArray%" is the spec name, and there's nothing called
"TypedArray" attached to the global object.

https://codereview.chromium.org/2778623003/diff/1/src/js/typedarray.js
File src/js/typedarray.js (right):

https://codereview.chromium.org/2778623003/diff/1/src/js/typedarray.js#newcode96
src/js/typedarray.js:96: %ValidateTypedArray(newTypedArray, "TypedArrayCreate");
I'm worried that adding a runtime call here might slow down construction. Is
there any perf testing you've done on this change?

https://codereview.chromium.org/2778623003/diff/1/src/js/typedarray.js#newcod...
src/js/typedarray.js:346: %ValidateTypedArray(this,
"TypedArray.prototype.every");
Hmm, again this is likely to be much more expensive than the previous check.

[Camillo Bruni, 2017-03-27 19:58:13]

cbruni@chromium.org changed reviewers:
+ cbruni@chromium.org, petermarshall@chromium.org

[Camillo Bruni, 2017-03-27 19:58:14]

Peter has been implementing TypedArray constructors in CSA, we could probably
push the neutered checks to there.

[Choongwoo Han, 2017-03-28 09:50:58]

I uploaded a patch again. please take another look.

https://codereview.chromium.org/2778623003/diff/1/src/builtins/builtins-typed...
File src/builtins/builtins-typedarray.cc (left):

https://codereview.chromium.org/2778623003/diff/1/src/builtins/builtins-typed...
src/builtins/builtins-typedarray.cc:50: const char* method =
"%TypedArray%.prototype.copyWithin";
On 2017/03/27 19:48:20, adamk wrote:
> Why did this change? "%TypedArray%" is the spec name, and there's nothing
called
> "TypedArray" attached to the global object.

Done.

https://codereview.chromium.org/2778623003/diff/1/src/js/typedarray.js
File src/js/typedarray.js (right):

https://codereview.chromium.org/2778623003/diff/1/src/js/typedarray.js#newcode96
src/js/typedarray.js:96: %ValidateTypedArray(newTypedArray, "TypedArrayCreate");
On 2017/03/27 19:48:20, adamk wrote:
> I'm worried that adding a runtime call here might slow down construction. Is
> there any perf testing you've done on this change?

Actually this function is not related with default construction (new
TypedArray(..)), but is called from subarray, .of, .from.

https://codereview.chromium.org/2778623003/diff/1/src/js/typedarray.js#newcod...
src/js/typedarray.js:346: %ValidateTypedArray(this,
"TypedArray.prototype.every");
On 2017/03/27 19:48:20, adamk wrote:
> Hmm, again this is likely to be much more expensive than the previous check.

Yes, it was too expensive than my expectation (2x overhead). So, I updated to
keep using the inline typed array check, and additional neutered checks through
runtime call. this is 5% overhead when I run the following minimal benchmark in
my machine.

```test.js
var arr = new Uint8Array(1024);

var t = performance.now();
for (var i = 0; i < 10000000; i++) {
  arr.every((v) => false)
}
print(performance.now() - t);
```
before: 896.7108000000001 (on average)
after: 939.5634 (on average)
939.5634 / 896.7108000000001 = 1.0477886515920183

[Dan Ehrenberg, 2017-03-28 19:39:28]

I don't quite understand why this version should be any faster. It looks like
it's still a runtime call each time.

About semantics: When implementing these methods, I left them as not throwing
originally to be consistent with the way that property access doesn't throw.
However, in interactive testing with Firefox, I can see that FF ships exactly
what this patch implements: throwing when entering the methods, but property
access still returns undefined. So it seems like this is a consistent enough
middle point, and we have evidence of its web-compatibility.

Probably worth checking Edge and Safari for compatibility clues as well.

https://codereview.chromium.org/2778623003/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2778623003/diff/20001/src/objects-inl.h#newco...
src/objects-inl.h:6910: // current implementations, and it's much useful to
return array for now.
Nit: I agree with your reasoning here; I think you can remove
"TODO(cwhan.tunz):" and just have this comment.

https://codereview.chromium.org/2778623003/diff/20001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-353004.js (left):

https://codereview.chromium.org/2778623003/diff/20001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-353004.js:76: assertEquals(0, array12.length);
Rather than deleting the test, can you change it to expect what's the new
current behavior? In this case, it should throw a TypeError.

https://codereview.chromium.org/2778623003/diff/20001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-4665.js (left):

https://codereview.chromium.org/2778623003/diff/20001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-4665.js:31: assertEquals(8, buf4.length);
Why was this test deleted?

[Choongwoo Han, 2017-03-29 05:36:55]

The main bottleneck of the previous version was to convert from String to
CString, so this version is faster. when I measured overhead of patch#2 again, I
found that the overhead is quite vary. sometimes it has 1.3x (30%) overhead in
my machine. I'm not sure that this is acceptable overhead or not.

Safari seems to have the same behavior: throwing at entering methods, and not
throwing when accessing properties.

https://codereview.chromium.org/2778623003/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2778623003/diff/20001/src/objects-inl.h#newco...
src/objects-inl.h:6910: // current implementations, and it's much useful to
return array for now.
On 2017/03/28 19:39:28, Dan Ehrenberg wrote:
> Nit: I agree with your reasoning here; I think you can remove
> "TODO(cwhan.tunz):" and just have this comment.

Done.

https://codereview.chromium.org/2778623003/diff/20001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-353004.js (left):

https://codereview.chromium.org/2778623003/diff/20001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-353004.js:76: assertEquals(0, array12.length);
On 2017/03/28 19:39:28, Dan Ehrenberg wrote:
> Rather than deleting the test, can you change it to expect what's the new
> current behavior? In this case, it should throw a TypeError.

Done.

https://codereview.chromium.org/2778623003/diff/20001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-4665.js (left):

https://codereview.chromium.org/2778623003/diff/20001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-4665.js:31: assertEquals(8, buf4.length);
On 2017/03/28 19:39:28, Dan Ehrenberg wrote:
> Why was this test deleted?

Changed it to expect the current behavior.

[Choongwoo Han, 2017-03-29 12:04:10]

Inlining WasNeutered Done.

Overhead: 733.9846 / 720.6404 = 1.0185171411427945,
when I ran the above benchmark 10 times.

[adamk, 2017-03-29 22:46:46]

Peter, what are your thoughts here?

[petermarshall, 2017-03-30 08:54:40]

On 2017/03/29 at 22:46:46, adamk wrote:
> Peter, what are your thoughts here?

I think this is a good idea, provided performance isn't affected by more than a
few percent. I don't really have the background as to why this isn't currently
spec-compliant, but I'm happy to make it so.

[adamk, 2017-03-30 18:58:32]

adamk@chromium.org changed reviewers:
+ bmeurer@chromium.org

[adamk, 2017-03-30 18:58:32]

This looks much better to me with the neutered check inlined, +bmeurer for
compiler/OWNERS.

https://codereview.chromium.org/2778623003/diff/80001/src/js/typedarray.js
File src/js/typedarray.js (right):

https://codereview.chromium.org/2778623003/diff/80001/src/js/typedarray.js#ne...
src/js/typedarray.js:76: function ValidateTypedArray(array, method) {
Please add a comment making it clear that this is equivalent to the same-named
spec operation, e.g.,

// ES#sec-validatetypedarray

https://codereview.chromium.org/2778623003/diff/80001/src/js/typedarray.js#ne...
src/js/typedarray.js:76: function ValidateTypedArray(array, method) {
Nit: I'd prefer the name "methodName" instead of "method" to make it clear this
is a string.

[Choongwoo Han, 2017-03-31 00:39:43]

https://codereview.chromium.org/2778623003/diff/80001/src/js/typedarray.js
File src/js/typedarray.js (right):

https://codereview.chromium.org/2778623003/diff/80001/src/js/typedarray.js#ne...
src/js/typedarray.js:76: function ValidateTypedArray(array, method) {
On 2017/03/30 18:58:32, adamk wrote:
> Please add a comment making it clear that this is equivalent to the same-named
> spec operation, e.g.,
> 
> // ES#sec-validatetypedarray

Done.

https://codereview.chromium.org/2778623003/diff/80001/src/js/typedarray.js#ne...
src/js/typedarray.js:76: function ValidateTypedArray(array, method) {
On 2017/03/30 18:58:32, adamk wrote:
> Nit: I'd prefer the name "methodName" instead of "method" to make it clear
this
> is a string.

Done.

[Benedikt Meurer, 2017-03-31 03:54:08]

LGTM on compiler

[Camillo Bruni, 2017-04-03 08:18:14]

LGTM with nits.

https://codereview.chromium.org/2778623003/diff/120001/src/js/typedarray.js
File src/js/typedarray.js (right):

https://codereview.chromium.org/2778623003/diff/120001/src/js/typedarray.js#n...
src/js/typedarray.js:404: ValidateTypedArray(this,
"TypeArray.prototype.filter");
nit: %TypedArray%.prototype.filter

https://codereview.chromium.org/2778623003/diff/120001/src/js/typedarray.js#n...
src/js/typedarray.js:506: ValidateTypedArray(this,
"%TypedArray%.prototype.join");
I thought this would fix built-ins/TypedArray/prototype/toString/detached-buffer
in test262?

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/es6/typed...
File test/mjsunit/es6/typedarray-tostring.js (right):

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/es6/typed...
test/mjsunit/es6/typedarray-tostring.js:106: assertThrows(() =>
array.toLocalString(), TypeError);
Please add the toString case as well (or a TODO if it still fails):
assertThrows(() => array.toString(), TypeError);

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
File test/mjsunit/regress/regress-4665.js (right):

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-4665.js:17: 
Would you mind extending this test a bit to make it clearer?

let seen_args = [];

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-4665.js:18: function SecondBuffer (arg) {
seen_args.push(arg);

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-4665.js:26: var buf3 = new SecondBuffer(10)
assertEquals([10], seen_args);

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-4665.js:30: assertEquals(10, buf4.length);
assertEquals([10, buf3.buffer], seen_args);

[Camillo Bruni, 2017-04-03 08:18:39]

and please wait for an LGTM from littledan

[Camillo Bruni, 2017-04-03 08:19:24]

https://codereview.chromium.org/2778623003/diff/120001/src/crankshaft/hydroge...
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/2778623003/diff/120001/src/crankshaft/hydroge...
src/crankshaft/hydrogen.cc:9930: DCHECK(expr->arguments()->length() == 1);
nit: DHECK_EQ(...)

[Choongwoo Han, 2017-04-03 08:42:29]

thanks

https://codereview.chromium.org/2778623003/diff/120001/src/crankshaft/hydroge...
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/2778623003/diff/120001/src/crankshaft/hydroge...
src/crankshaft/hydrogen.cc:9930: DCHECK(expr->arguments()->length() == 1);
On 2017/04/03 08:19:23, Camillo Bruni wrote:
> nit: DHECK_EQ(...)

Done.

https://codereview.chromium.org/2778623003/diff/120001/src/js/typedarray.js
File src/js/typedarray.js (right):

https://codereview.chromium.org/2778623003/diff/120001/src/js/typedarray.js#n...
src/js/typedarray.js:404: ValidateTypedArray(this,
"TypeArray.prototype.filter");
On 2017/04/03 08:18:14, Camillo Bruni wrote:
> nit: %TypedArray%.prototype.filter

Done.

https://codereview.chromium.org/2778623003/diff/120001/src/js/typedarray.js#n...
src/js/typedarray.js:506: ValidateTypedArray(this,
"%TypedArray%.prototype.join");
On 2017/04/03 08:18:14, Camillo Bruni wrote:
> I thought this would fix
built-ins/TypedArray/prototype/toString/detached-buffer
> in test262?

ah, you are right. Done.

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/es6/typed...
File test/mjsunit/es6/typedarray-tostring.js (right):

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/es6/typed...
test/mjsunit/es6/typedarray-tostring.js:106: assertThrows(() =>
array.toLocalString(), TypeError);
On 2017/04/03 08:18:14, Camillo Bruni wrote:
> Please add the toString case as well (or a TODO if it still fails):
> assertThrows(() => array.toString(), TypeError);

Done.

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
File test/mjsunit/regress/regress-4665.js (right):

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-4665.js:17: 
On 2017/04/03 08:18:14, Camillo Bruni wrote:
> Would you mind extending this test a bit to make it clearer?
> 
> let seen_args = [];

Done.

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-4665.js:18: function SecondBuffer (arg) {
On 2017/04/03 08:18:14, Camillo Bruni wrote:
> seen_args.push(arg);

Done.

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-4665.js:26: var buf3 = new SecondBuffer(10)
On 2017/04/03 08:18:14, Camillo Bruni wrote:
> assertEquals([10], seen_args);

Done.

https://codereview.chromium.org/2778623003/diff/120001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-4665.js:30: assertEquals(10, buf4.length);
On 2017/04/03 08:18:14, Camillo Bruni wrote:
> assertEquals([10, buf3.buffer], seen_args);

Done.

[Choongwoo Han, 2017-04-03 08:42:43]

The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 08:42:49]

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, 2017-04-03 09:04:49]

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

[commit-bot: I haz the power, 2017-04-03 09:04:50]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/20237)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Choongwoo Han, 2017-04-03 09:25:00]

Description was changed from

==========
[typedarrays] Check detached buffer at start of typed array methods

- Throw TypeError in ValidateTypedArray
- Validate typed arrays at start of each typed array prototype
  methods in src/js/typedarrays.js
- Add tests to check detached buffers
- Remove an unnecessary parameter of TypedArraySpeciesCreate
  in src/js/typedarrays.js
- Standardize TypedArray.prototype.subarray
- Update test262.status to pass detached buffer tests

BUG=v8:4648,v8:4665
==========

to

==========
[typedarrays] Check detached buffer at start of typed array methods

- Throw TypeError in ValidateTypedArray
- Validate typed arrays at start of each typed array prototype
  methods in src/js/typedarrays.js
- Add tests to check detached buffers
- Remove an unnecessary parameter of TypedArraySpeciesCreate
  in src/js/typedarrays.js
- Standardize TypedArray.prototype.subarray
- Update test262.status to pass detached buffer tests

BUG=v8:4648,v8:4665,v8:4953
==========

[Choongwoo Han, 2017-04-03 09:25:10]

The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 09:25:16]

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, 2017-04-03 09:44:22]

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

[commit-bot: I haz the power, 2017-04-03 09:44:23]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[Choongwoo Han, 2017-04-03 09:50:30]

The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 09:50:32]

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, 2017-04-03 10:15:07]

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

[commit-bot: I haz the power, 2017-04-03 10:15:08]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2017-04-03 11:44:06]

Description was changed from

==========
[typedarrays] Check detached buffer at start of typed array methods

- Throw TypeError in ValidateTypedArray
- Validate typed arrays at start of each typed array prototype
  methods in src/js/typedarrays.js
- Add tests to check detached buffers
- Remove an unnecessary parameter of TypedArraySpeciesCreate
  in src/js/typedarrays.js
- Standardize TypedArray.prototype.subarray
- Update test262.status to pass detached buffer tests

BUG=v8:4648,v8:4665,v8:4953
==========

to

==========
[typedarrays] Check detached buffer at start of typed array methods

- Throw TypeError in ValidateTypedArray, matching JSC, SpiderMonkey
  and ChakraCore.
- Validate typed arrays at start of each typed array prototype
  methods in src/js/typedarrays.js
- Add tests to check detached buffers
- Remove an unnecessary parameter of TypedArraySpeciesCreate
  in src/js/typedarrays.js
- Standardize TypedArray.prototype.subarray
- Update test262.status to pass detached buffer tests

BUG=v8:4648,v8:4665,v8:4953
==========

[Dan Ehrenberg, 2017-04-03 11:46:50]

lgtm

Thanks so much for coming in and implementing this fix!

Added a short note in the commit message about how this matches other browsers.

Nit: Might be better to start referring to 'neutered' as 'detached' to match the
ES spec, but we already call it neutered all over the place, as does HTML, so
this is fine.

[Choongwoo Han, 2017-04-04 03:46:58]

The CQ bit was checked by cwhan.tunz@gmail.com

[Choongwoo Han, 2017-04-04 03:46:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2778623003/#ps180001
(title: "pass test262 for subarray")

[commit-bot: I haz the power, 2017-04-04 03:47:10]

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, 2017-04-04 03:48:49]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491277618673230,
"parent_rev": "6cdd3775862e29b74218f85d8c141911caa2befd", "commit_rev":
"238d5b4453d9166aaddce76a5393514d977238d4"}

[commit-bot: I haz the power, 2017-04-04 03:48:56]

Description was changed from

==========
[typedarrays] Check detached buffer at start of typed array methods

- Throw TypeError in ValidateTypedArray, matching JSC, SpiderMonkey
  and ChakraCore.
- Validate typed arrays at start of each typed array prototype
  methods in src/js/typedarrays.js
- Add tests to check detached buffers
- Remove an unnecessary parameter of TypedArraySpeciesCreate
  in src/js/typedarrays.js
- Standardize TypedArray.prototype.subarray
- Update test262.status to pass detached buffer tests

BUG=v8:4648,v8:4665,v8:4953
==========

to

==========
[typedarrays] Check detached buffer at start of typed array methods

- Throw TypeError in ValidateTypedArray, matching JSC, SpiderMonkey
  and ChakraCore.
- Validate typed arrays at start of each typed array prototype
  methods in src/js/typedarrays.js
- Add tests to check detached buffers
- Remove an unnecessary parameter of TypedArraySpeciesCreate
  in src/js/typedarrays.js
- Standardize TypedArray.prototype.subarray
- Update test262.status to pass detached buffer tests

BUG=v8:4648,v8:4665,v8:4953

Review-Url: https://codereview.chromium.org/2778623003
Cr-Commit-Position: refs/heads/master@{#44357}
Committed:
https://chromium.googlesource.com/v8/v8/+/238d5b4453d9166aaddce76a5393514d977...
==========

[commit-bot: I haz the power, 2017-04-04 03:48:57]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/v8/v8/+/238d5b4453d9166aaddce76a5393514d977...

[Michael Achenbach, 2017-04-04 08:00:38]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2793233003/ by machenbach@chromium.org.

The reason for reverting is: Breaks layout tests:
https://build.chromium.org/p/tryserver.v8/builders/v8_linux_blink_rel/builds/...

Changes:
https://storage.googleapis.com/chromium-layout-test-archives/v8_linux_blink_r...

See:
https://github.com/v8/v8/wiki/Blink-layout-tests.

[Dan Ehrenberg, 2017-04-04 10:10:37]

On 2017/04/04 08:00:38, Michael Achenbach wrote:
> A revert of this CL (patchset #10 id:180001) has been created in
> https://codereview.chromium.org/2793233003/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: Breaks layout tests:
>
https://build.chromium.org/p/tryserver.v8/builders/v8_linux_blink_rel/builds/...
> 
> Changes:
>
https://storage.googleapis.com/chromium-layout-test-archives/v8_linux_blink_r...
> 
> See:
> https://github.com/v8/v8/wiki/Blink-layout-tests.

Looks like this test (which you can find in a Chromium checkout at
src/third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html)
fails because, on success, it converts a detached TypedArray to a String (see "+
third" on line 39, as called from line 89). One way to update the test for this
would be to call Object.prototype.toString.call() on third--keep in mind this
will require updating window-postmessage-args-expected.txt as well.

cwhan.tunz, do you think you could do this in a separate Chromium CL?

[Choongwoo Han, 2017-04-04 15:05:24]

On 2017/04/04 10:10:37, Dan Ehrenberg wrote:
> On 2017/04/04 08:00:38, Michael Achenbach wrote:
> > A revert of this CL (patchset #10 id:180001) has been created in
> > https://codereview.chromium.org/2793233003/ by
mailto:machenbach@chromium.org.
> > 
> > The reason for reverting is: Breaks layout tests:
> >
>
https://build.chromium.org/p/tryserver.v8/builders/v8_linux_blink_rel/builds/...
> > 
> > Changes:
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/v8_linux_blink_r...
> > 
> > See:
> > https://github.com/v8/v8/wiki/Blink-layout-tests.
> 
> Looks like this test (which you can find in a Chromium checkout at
>
src/third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html)
> fails because, on success, it converts a detached TypedArray to a String (see
"+
> third" on line 39, as called from line 89). One way to update the test for
this
> would be to call Object.prototype.toString.call() on third--keep in mind this
> will require updating window-postmessage-args-expected.txt as well.
> 
> cwhan.tunz, do you think you could do this in a separate Chromium CL?

maybe I need some time to setup and learn the chromium environment, but I don't
have time this week. Could you do this if you don't mind? or, I can do this in
next week (or two weeks later).

[Dan Ehrenberg, 2017-04-04 17:24:22]

On 2017/04/04 15:05:24, Choongwoo Han wrote:
> On 2017/04/04 10:10:37, Dan Ehrenberg wrote:
> > On 2017/04/04 08:00:38, Michael Achenbach wrote:
> > > A revert of this CL (patchset #10 id:180001) has been created in
> > > https://codereview.chromium.org/2793233003/ by
> mailto:machenbach@chromium.org.
> > > 
> > > The reason for reverting is: Breaks layout tests:
> > >
> >
>
https://build.chromium.org/p/tryserver.v8/builders/v8_linux_blink_rel/builds/...
> > > 
> > > Changes:
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/v8_linux_blink_r...
> > > 
> > > See:
> > > https://github.com/v8/v8/wiki/Blink-layout-tests.
> > 
> > Looks like this test (which you can find in a Chromium checkout at
> >
>
src/third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html)
> > fails because, on success, it converts a detached TypedArray to a String
(see
> "+
> > third" on line 39, as called from line 89). One way to update the test for
> this
> > would be to call Object.prototype.toString.call() on third--keep in mind
this
> > will require updating window-postmessage-args-expected.txt as well.
> > 
> > cwhan.tunz, do you think you could do this in a separate Chromium CL?
> 
> maybe I need some time to setup and learn the chromium environment, but I
don't
> have time this week. Could you do this if you don't mind? or, I can do this in
> next week (or two weeks later).

The patch has been reverted, so there's no rush for a test fix this instant. I
think this can wait a couple weeks to reland. I'm happy if you're willing to set
up a Chromium environment--this will be useful for you in developing V8. Don't
hesitate to ask if you have trouble setting it up.

[Benedikt Meurer, 2017-04-26 05:58:34]

LGTM on compiler