Keep materialized ArrayBuffer and ImageBitmap objects in SerializedScriptValue.

Keep materialized ArrayBuffer and ImageBitmap objects in SerializedScriptValue.

This allows a message to be received in multiple worlds, even though the
ArrayBufferContents can only be consumed once.

BUG=623355
Review-Url: https://codereview.chromium.org/2878053002
Cr-Commit-Position: refs/heads/master@{#471868}
Committed: https://chromium.googlesource.com/chromium/src/+/4bcb9954c02c7dd9c7cb2a0756e4a3002ec66e5d

[jbroman, 2017-05-12 17:34:56]

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

[commit-bot: I haz the power, 2017-05-12 17:36:08]

Dry run: CQ is trying da patch.

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

[jbroman, 2017-05-12 17:41:49]

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

[commit-bot: I haz the power, 2017-05-12 17:43:01]

Dry run: CQ is trying da patch.

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

[jbroman, 2017-05-12 19:39:18]

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

[commit-bot: I haz the power, 2017-05-12 19:39:39]

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-05-13 00:08:01]

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

[commit-bot: I haz the power, 2017-05-13 00:08:01]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-05-13 05:48:12]

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

[commit-bot: I haz the power, 2017-05-13 05:48:27]

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-05-13 09:08:44]

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

[commit-bot: I haz the power, 2017-05-13 09:08:45]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-05-15 14:56:48]

jbroman@chromium.org changed reviewers:
+ haraken@chromium.org

[jbroman, 2017-05-15 14:56:49]

This seems better than crashing, at least.

It's still slightly weird (the worlds end up sharing the same wrapped array
buffer, and so can do reads/writes that are visible to one another), which could
conceivably cause trouble, but I'm not sure what would be better.

[haraken, 2017-05-15 17:13:33]

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(left):

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:167:
// Transfer image bitmaps.

Why can we remove this code? (Maybe I'm failing at understanding the point of
this CL :-)

[haraken, 2017-05-15 17:18:44]

LGTM

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h
(right):

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h:211:
return received_->array_buffers;

Can we add a dcheck to verify that this is called after the data was received?

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h:214:
return received_->image_bitmaps;

Ditto.

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(left):

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:167:
// Transfer image bitmaps.
On 2017/05/15 17:13:33, haraken wrote:
> 
> Why can we remove this code? (Maybe I'm failing at understanding the point of
> this CL :-)

Ah, sorry. I now understand. transferred_image_bitmaps_ is no longer needed
after this CL.

[jbroman, 2017-05-15 17:19:26]

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(left):

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:167:
// Transfer image bitmaps.
On 2017/05/15 at 17:13:33, haraken wrote:
> Why can we remove this code? (Maybe I'm failing at understanding the point of
this CL :-)

Because ReceiveTransfer exists to create these objects (but also to avoid trying
to create them a second time if the SSV is deserialized again in a second world
-- which notably failed for ArrayBuffer in the attached bug).

So both here and above, the work of creating the objects is factored out and
moved there. For ImageBitmap, that was all the work being done here.

[jbroman, 2017-05-15 17:20:51]

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h
(right):

https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h:211:
return received_->array_buffers;
On 2017/05/15 at 17:18:44, haraken wrote:
> Can we add a dcheck to verify that this is called after the data was received?

Could do, but base::Optional already DCHECKs it (twice, in fact). Seemed
overkill to add a third DCHECK in a row.

[haraken, 2017-05-15 17:23:24]

On 2017/05/15 17:20:51, jbroman wrote:
>
https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h
> (right):
> 
>
https://codereview.chromium.org/2878053002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h:211:
> return received_->array_buffers;
> On 2017/05/15 at 17:18:44, haraken wrote:
> > Can we add a dcheck to verify that this is called after the data was
received?
> 
> Could do, but base::Optional already DCHECKs it (twice, in fact). Seemed
> overkill to add a third DCHECK in a row.

Makes sense.

[jbroman, 2017-05-15 17:30:26]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2017-05-15 17:31:05]

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-05-15 19:37:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494869426436670,
"parent_rev": "468b6062369eb147e6907c8c2fba82f78370aafa", "commit_rev":
"4bcb9954c02c7dd9c7cb2a0756e4a3002ec66e5d"}

[commit-bot: I haz the power, 2017-05-15 19:37:17]

Description was changed from

==========
Keep materialized ArrayBuffer and ImageBitmap objects in SerializedScriptValue.

This allows a message to be received in multiple worlds, even though the
ArrayBufferContents can only be consumed once.

BUG=623355
==========

to

==========
Keep materialized ArrayBuffer and ImageBitmap objects in SerializedScriptValue.

This allows a message to be received in multiple worlds, even though the
ArrayBufferContents can only be consumed once.

BUG=623355

Review-Url: https://codereview.chromium.org/2878053002
Cr-Commit-Position: refs/heads/master@{#471868}
Committed:
https://chromium.googlesource.com/chromium/src/+/4bcb9954c02c7dd9c7cb2a0756e4...
==========

[commit-bot: I haz the power, 2017-05-15 19:37:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4bcb9954c02c7dd9c7cb2a0756e4...