Disallow passing a SharedArrayBuffer in the transfer list.

Disallow passing a SharedArrayBuffer in the transfer list.

This behavior changed recently. SharedArrayBuffers should not be put in the
transfer list, because they are not detached, and that is the meaning of being
in the transfer list.

This is the V8 side of the change, the Blink side will come next.

Review-Url: https://codereview.chromium.org/2570433005
Cr-Commit-Position: refs/heads/master@{#41740}
Committed: https://chromium.googlesource.com/v8/v8/+/1c5e1504e0305363cd262f1706cbd63f9a62ae46

[binji, 2016-12-12 23:59:17]

binji@chromium.org changed reviewers:
+ jbroman@chromium.org, jkummerow@chromium.org

[Jakob Kummerow, 2016-12-13 02:34:58]

lgtm

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc
File src/value-serializer.cc (right):

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:738: ThrowDataCloneError(
Would it make more sense to enforce this with a
DCHECK(!array_buffer->is_shared()) in ValueSerializer::TransferArrayBuffer()
(and/or a DCHECK here)? Then you could move the
array_buffer_transfer_map_.Find() call into the branch below where
|transfer_entry| is actually required.

[jbroman, 2016-12-13 20:56:00]

lgtm with nits

https://codereview.chromium.org/2570433005/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2570433005/diff/1/include/v8.h#newcode1722
include/v8.h:1722: virtual Maybe<uint32_t> TransferSharedArrayBuffer(Isolate*
isolate,
Do we want to use the word "transfer" here, given we're removing it from the
transfer list? (I'm admittedly not up to date on context behind this decision.)

https://codereview.chromium.org/2570433005/diff/1/include/v8.h#newcode1723
include/v8.h:1723: Local<Object> object);
I'd expect the argument to be Local<SharedArrayBuffer> here.

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc
File src/value-serializer.cc (right):

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:738: ThrowDataCloneError(
On 2016/12/13 at 02:34:58, Jakob Kummerow wrote:
> Would it make more sense to enforce this with a
DCHECK(!array_buffer->is_shared()) in ValueSerializer::TransferArrayBuffer()
(and/or a DCHECK here)? Then you could move the
array_buffer_transfer_map_.Find() call into the branch below where
|transfer_entry| is actually required.

+1

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:749:
WriteTag(SerializationTag::kSharedArrayBufferTransfer);
nit: Should this be renamed to just kSharedArrayBuffer, since it's not a
transfer anymore?

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:752: } else {
nit: From my perspective, having to match this else with the corresponding if
makes this harder to read. Since the other branch always returns anyhow, could
this be formatted as:

if (array_buffer->is_shared()) {
  // shared array buffers
  return Just(true);
}

uint32_t* transfer_entry = array_buffer_transfer_map_.Find(array_buffer);
if (transfer_entry) {
  // array buffer transfer
  return Just(true);
}

// remaining code

https://codereview.chromium.org/2570433005/diff/1/test/unittests/value-serial...
File test/unittests/value-serializer-unittest.cc (right):

https://codereview.chromium.org/2570433005/diff/1/test/unittests/value-serial...
test/unittests/value-serializer-unittest.cc:2144:
.WillRepeatedly(Invoke([this](Isolate*, Local<Object> object) {
nit: Since this just validates an argument and returns a constant, you might not
need to use testing::Invoke. Does this work?

EXPECT_CALL(serializer_delegate_, TransferSharedArrayBuffer(isolate(),
input_buffer()))
    .WillOnce(Return(Just(0U)));

[binji, 2016-12-14 23:58:30]

https://codereview.chromium.org/2570433005/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2570433005/diff/1/include/v8.h#newcode1722
include/v8.h:1722: virtual Maybe<uint32_t> TransferSharedArrayBuffer(Isolate*
isolate,
On 2016/12/13 20:56:00, jbroman wrote:
> Do we want to use the word "transfer" here, given we're removing it from the
> transfer list? (I'm admittedly not up to date on context behind this
decision.)

AIUI, the thought is that transferring is defined by the original value being
detached more than whether the value is serialized or not. Makes some sense I
guess, but it unfortunately complicates the code.

So good point, though I'm not sure what else to call it. Maybe
"ShareSharedArrayBuffer"?

https://codereview.chromium.org/2570433005/diff/1/include/v8.h#newcode1723
include/v8.h:1723: Local<Object> object);
On 2016/12/13 20:56:00, jbroman wrote:
> I'd expect the argument to be Local<SharedArrayBuffer> here.

Done.

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc
File src/value-serializer.cc (right):

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:738: ThrowDataCloneError(
On 2016/12/13 20:56:00, jbroman wrote:
> On 2016/12/13 at 02:34:58, Jakob Kummerow wrote:
> > Would it make more sense to enforce this with a
> DCHECK(!array_buffer->is_shared()) in ValueSerializer::TransferArrayBuffer()
> (and/or a DCHECK here)? Then you could move the
> array_buffer_transfer_map_.Find() call into the branch below where
> |transfer_entry| is actually required.
> 
> +1

Done.

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:749:
WriteTag(SerializationTag::kSharedArrayBufferTransfer);
On 2016/12/13 20:56:00, jbroman wrote:
> nit: Should this be renamed to just kSharedArrayBuffer, since it's not a
> transfer anymore?

Done.

https://codereview.chromium.org/2570433005/diff/1/src/value-serializer.cc#new...
src/value-serializer.cc:752: } else {
On 2016/12/13 20:56:00, jbroman wrote:
> nit: From my perspective, having to match this else with the corresponding if
> makes this harder to read. Since the other branch always returns anyhow, could
> this be formatted as:
> 
> if (array_buffer->is_shared()) {
>   // shared array buffers
>   return Just(true);
> }
> 
> uint32_t* transfer_entry = array_buffer_transfer_map_.Find(array_buffer);
> if (transfer_entry) {
>   // array buffer transfer
>   return Just(true);
> }
> 
> // remaining code

Done.

https://codereview.chromium.org/2570433005/diff/1/test/unittests/value-serial...
File test/unittests/value-serializer-unittest.cc (right):

https://codereview.chromium.org/2570433005/diff/1/test/unittests/value-serial...
test/unittests/value-serializer-unittest.cc:2144:
.WillRepeatedly(Invoke([this](Isolate*, Local<Object> object) {
On 2016/12/13 20:56:00, jbroman wrote:
> nit: Since this just validates an argument and returns a constant, you might
not
> need to use testing::Invoke. Does this work?
> 
> EXPECT_CALL(serializer_delegate_, TransferSharedArrayBuffer(isolate(),
> input_buffer()))
>     .WillOnce(Return(Just(0U)));

Done.

[binji, 2016-12-14 23:59:35]

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

[commit-bot: I haz the power, 2016-12-14 23:59:40]

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-12-15 00:06:53]

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

[commit-bot: I haz the power, 2016-12-15 00:06:54]

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/14211)

[jbroman, 2016-12-15 16:15:33]

https://codereview.chromium.org/2570433005/diff/20001/src/value-serializer.cc
File src/value-serializer.cc (right):

https://codereview.chromium.org/2570433005/diff/20001/src/value-serializer.cc...
src/value-serializer.cc:740: DCHECK(!index.IsNothing());
super-nit: index.FromJust() will already crash in this case; is there value
gained by the additional DCHECK?

https://codereview.chromium.org/2570433005/diff/20001/test/unittests/value-se...
File test/unittests/value-serializer-unittest.cc (right):

https://codereview.chromium.org/2570433005/diff/20001/test/unittests/value-se...
test/unittests/value-serializer-unittest.cc:2130: bool
should_transfer_shared_array_buffer_;
This seems to only ever be set to false. Should it be set to true somewhere, or
is the code guarded by this dead?

[binji, 2016-12-15 19:31:21]

https://codereview.chromium.org/2570433005/diff/20001/src/value-serializer.cc
File src/value-serializer.cc (right):

https://codereview.chromium.org/2570433005/diff/20001/src/value-serializer.cc...
src/value-serializer.cc:740: DCHECK(!index.IsNothing());
On 2016/12/15 16:15:33, jbroman wrote:
> super-nit: index.FromJust() will already crash in this case; is there value
> gained by the additional DCHECK?

Done.

https://codereview.chromium.org/2570433005/diff/20001/test/unittests/value-se...
File test/unittests/value-serializer-unittest.cc (right):

https://codereview.chromium.org/2570433005/diff/20001/test/unittests/value-se...
test/unittests/value-serializer-unittest.cc:2130: bool
should_transfer_shared_array_buffer_;
On 2016/12/15 16:15:33, jbroman wrote:
> This seems to only ever be set to false. Should it be set to true somewhere,
or
> is the code guarded by this dead?

Done, forgot to remove after removing the test that used it!

[binji, 2016-12-15 19:31:49]

The CQ bit was checked by binji@chromium.org

[binji, 2016-12-15 19:31:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbroman@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/2570433005/#ps40001
(title: "fixes")

[commit-bot: I haz the power, 2016-12-15 19:31:56]

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

[binji, 2016-12-15 19:32:33]

The CQ bit was checked by binji@chromium.org

[binji, 2016-12-15 19:32:34]

The CQ bit was unchecked by binji@chromium.org

[binji, 2016-12-15 23:24:45]

https://codereview.chromium.org/2570433005/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2570433005/diff/1/include/v8.h#newcode1722
include/v8.h:1722: virtual Maybe<uint32_t> TransferSharedArrayBuffer(Isolate*
isolate,
On 2016/12/14 23:58:30, binji wrote:
> On 2016/12/13 20:56:00, jbroman wrote:
> > Do we want to use the word "transfer" here, given we're removing it from the
> > transfer list? (I'm admittedly not up to date on context behind this
> decision.)
> 
> AIUI, the thought is that transferring is defined by the original value being
> detached more than whether the value is serialized or not. Makes some sense I
> guess, but it unfortunately complicates the code.
> 
> So good point, though I'm not sure what else to call it. Maybe
> "ShareSharedArrayBuffer"?

I went with GetSharedArrayBufferId, I think that's pretty clear.

[binji, 2016-12-15 23:24:49]

The CQ bit was checked by binji@chromium.org

[binji, 2016-12-15 23:24:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbroman@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/2570433005/#ps60001
(title: "TransferSharedArrayBuffer -> GetSharedArrayBufferId")

[commit-bot: I haz the power, 2016-12-15 23:24:54]

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-12-15 23:55:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481844289460390,
"parent_rev": "635edafaa6dc23dac3f28e18fcd914db667dfdbf", "commit_rev":
"1c5e1504e0305363cd262f1706cbd63f9a62ae46"}

[commit-bot: I haz the power, 2016-12-15 23:55:07]

Description was changed from

==========
Disallow passing a SharedArrayBuffer in the transfer list.

This behavior changed recently. SharedArrayBuffers should not be put in the
transfer list, because they are not detached, and that is the meaning of being
in the transfer list.

This is the V8 side of the change, the Blink side will come next.
==========

to

==========
Disallow passing a SharedArrayBuffer in the transfer list.

This behavior changed recently. SharedArrayBuffers should not be put in the
transfer list, because they are not detached, and that is the meaning of being
in the transfer list.

This is the V8 side of the change, the Blink side will come next.

Review-Url: https://codereview.chromium.org/2570433005
Cr-Commit-Position: refs/heads/master@{#41740}
Committed:
https://chromium.googlesource.com/v8/v8/+/1c5e1504e0305363cd262f1706cbd63f9a6...
==========

[commit-bot: I haz the power, 2016-12-15 23:55:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/1c5e1504e0305363cd262f1706cbd63f9a6...

[Michael Achenbach, 2016-12-16 07:34:26]

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

The reason for reverting is: Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...

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