[wasm] Transferrable modules

[wasm] Transferrable modules

We want to restrict structured cloning in Chrome to:
- postMessage senders and receivers that are co-located 
in the same process
- indexedDB (just https).

For context, on the Chrome side, we will achieve the postMessage part 
by using a mechanism similar to transferrables: the 
SerializedScriptValue will have a list of wasm modules, separate from 
the serialized data stream; and this list won't be copied cross 
process boundaries. The IDB part is achieved by explicitly opting in
reading/writing to the serialization stream. To block attack vectors 
in IPC cases, the default for deserialization will be to expect data 
in the wasm transfers list.

This change is the V8 side necessary to enabling this design. We 
introduce TransferrableModule, an opaque datatype exposed to the
embedder. Internally, TransferrableModules are just serialized data,
because we don't have a better mechanism, at the moment, for
de-contextualizing/re-contextualizing wasm modules (wrt Isolate and 
Context).

The chrome defaults will be implemented in the 
serialization/deserialization delegates on that side. For the v8 side 
of things, in the absence of a serialization delegate, the V8
serializer will write to serialization stream. In the absence of a 
deserialization delegate, the deserializer won't work. This asymmetry
is intentional - it communicates to the embedder the need to make a 
policy decision, otherwise wasm serialization/deserialization won't 
work "out of the box".

BUG=v8:6079
Review-Url: https://codereview.chromium.org/2748473004
Cr-Commit-Position: refs/heads/master@{#43955}
Committed: https://chromium.googlesource.com/v8/v8/+/99743ad460ea5b9795ba9d70a074e75d7362a3d1

[Mircea Trofin, 2017-03-11 17:58:51]

Patchset #1 (id:1) has been deleted

[Mircea Trofin, 2017-03-11 18:28:38]

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

[commit-bot: I haz the power, 2017-03-11 18:28: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-03-11 18:42:59]

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

[commit-bot: I haz the power, 2017-03-11 18:43:00]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Mircea Trofin, 2017-03-12 03:36:09]

Patchset #4 (id:80001) has been deleted

[Mircea Trofin, 2017-03-12 03:39:37]

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

[commit-bot: I haz the power, 2017-03-12 03:39: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, 2017-03-12 03:53:09]

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

[commit-bot: I haz the power, 2017-03-12 03:53:10]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Mircea Trofin, 2017-03-12 06:46:32]

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

[commit-bot: I haz the power, 2017-03-12 06:46:42]

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-03-12 07:08:28]

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

[commit-bot: I haz the power, 2017-03-12 07:08:29]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-03-12 16:12:33]

Description was changed from

==========
Transferrable modules
==========

to

==========
[wasm] Transferrable modules

We want to restrict structured cloning in Chrome to:
- postMessage senders and receivers that are co-located 
in the same process
- indexedDB (just https).

For context, on the chrome side, we will achieve the postMessage part 
by using a mechanism similar to transferrables: the 
SerializedScriptValue will have a list of wasm modules, separate from 
the serialized data stream; and this list won't be copied cross 
process boundaries. The IDB part is achieved by explicitly opting in
reading/writing to the serialization stream. To block attack vectors 
in IPC cases, the default for deserialization will be to expect data 
in the wasm transfers list.

This change is the V8 side necessary to enabling this design. We 
introduce TransferrableModule, an opaque datatype exposed to the
embedder. Internally, TransferrableModules are just serialized data,
because we don't have a better mechanism, at the moment, for
de-contextualizing/re-contextualizing wasm modules (wrt Isolate and 
Context)

The chrome defaults will be implemented in the 
serialization/deserialization delegates on that side. For the v8 side 
of things, in the absence of a serialization delegate, the V8
serializer will write to serialization stream. In the absence of a 
deserialization delegate, the deserializer won't work. This asymmetry
is intentional - it communicates to the embedder the need to make a 
policy decision, otherwise wasm serialization/deserialization won't 
work "out of the box".

BUG=v8:6079
==========

[Mircea Trofin, 2017-03-12 16:12:33]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org, jbroman@chromium.org

[Mircea Trofin, 2017-03-12 16:13:25]

Description was changed from

==========
[wasm] Transferrable modules

We want to restrict structured cloning in Chrome to:
- postMessage senders and receivers that are co-located 
in the same process
- indexedDB (just https).

For context, on the chrome side, we will achieve the postMessage part 
by using a mechanism similar to transferrables: the 
SerializedScriptValue will have a list of wasm modules, separate from 
the serialized data stream; and this list won't be copied cross 
process boundaries. The IDB part is achieved by explicitly opting in
reading/writing to the serialization stream. To block attack vectors 
in IPC cases, the default for deserialization will be to expect data 
in the wasm transfers list.

This change is the V8 side necessary to enabling this design. We 
introduce TransferrableModule, an opaque datatype exposed to the
embedder. Internally, TransferrableModules are just serialized data,
because we don't have a better mechanism, at the moment, for
de-contextualizing/re-contextualizing wasm modules (wrt Isolate and 
Context)

The chrome defaults will be implemented in the 
serialization/deserialization delegates on that side. For the v8 side 
of things, in the absence of a serialization delegate, the V8
serializer will write to serialization stream. In the absence of a 
deserialization delegate, the deserializer won't work. This asymmetry
is intentional - it communicates to the embedder the need to make a 
policy decision, otherwise wasm serialization/deserialization won't 
work "out of the box".

BUG=v8:6079
==========

to

==========
[wasm] Transferrable modules

We want to restrict structured cloning in Chrome to:
- postMessage senders and receivers that are co-located 
in the same process
- indexedDB (just https).

For context, on the Chrome side, we will achieve the postMessage part 
by using a mechanism similar to transferrables: the 
SerializedScriptValue will have a list of wasm modules, separate from 
the serialized data stream; and this list won't be copied cross 
process boundaries. The IDB part is achieved by explicitly opting in
reading/writing to the serialization stream. To block attack vectors 
in IPC cases, the default for deserialization will be to expect data 
in the wasm transfers list.

This change is the V8 side necessary to enabling this design. We 
introduce TransferrableModule, an opaque datatype exposed to the
embedder. Internally, TransferrableModules are just serialized data,
because we don't have a better mechanism, at the moment, for
de-contextualizing/re-contextualizing wasm modules (wrt Isolate and 
Context).

The chrome defaults will be implemented in the 
serialization/deserialization delegates on that side. For the v8 side 
of things, in the absence of a serialization delegate, the V8
serializer will write to serialization stream. In the absence of a 
deserialization delegate, the deserializer won't work. This asymmetry
is intentional - it communicates to the embedder the need to make a 
policy decision, otherwise wasm serialization/deserialization won't 
work "out of the box".

BUG=v8:6079
==========

[Mircea Trofin, 2017-03-12 16:14:57]

ptal - v8 side of structured cloning controls

[jbroman, 2017-03-13 18:53:38]

How settled is the issue of how wasm structured clone works (both from an impl
and spec perspective)?

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1718
include/v8.h:1718: virtual Maybe<uint32_t> GetWasmModuleTransferId(
Here and elsewhere: Before landing, add comments to new public API, like the
nearby methods?

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1831
include/v8.h:1831: uint32_t id);
Ditto: we've tried to reduce the number of virtual calls to the delegate. Could
this be implemented as something like:

void TransferWasmModule(uint32_t transfer_id, Local<WasmCompiledModule> module);

(analogous to how array buffers work)

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1832
include/v8.h:1832: virtual bool AllowInlineWasm() const { return false; }
Is there a reason this might vary over the lifetime of the serializer? If not,
it's one fewer virtual call if this is just a configured boolean, like
SetSupportsLegacyWireFormat.

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode3940
include/v8.h:3940: Isolate* isolate, TransferrableModule&);
(destructive) lvalue-ref parameter here is strange. I'd have expected
pass-by-value or rvalue-ref.

https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:765: Handle<Context> native_context =
isolate->native_context();
remove separate change (I assume debugging code)?

https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-objects....
src/wasm/wasm-objects.cc:225: Handle<Context> ctx = isolate->native_context();
remove separate change (I assume debugging code)?

[Mircea Trofin, 2017-03-15 18:02:05]

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

[commit-bot: I haz the power, 2017-03-15 18:02:14]

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

[Mircea Trofin, 2017-03-15 18:02:29]

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1718
include/v8.h:1718: virtual Maybe<uint32_t> GetWasmModuleTransferId(
On 2017/03/13 18:53:37, jbroman wrote:
> Here and elsewhere: Before landing, add comments to new public API, like the
> nearby methods?

Done.

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1831
include/v8.h:1831: uint32_t id);
On 2017/03/13 18:53:38, jbroman wrote:
> Ditto: we've tried to reduce the number of virtual calls to the delegate.
Could
> this be implemented as something like:
> 
> void TransferWasmModule(uint32_t transfer_id, Local<WasmCompiledModule>
module);
> 
> (analogous to how array buffers work)

This is more like transferring SharedArrayBuffers, hence a design similar to
GetSharedBufferId().

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1832
include/v8.h:1832: virtual bool AllowInlineWasm() const { return false; }
On 2017/03/13 18:53:37, jbroman wrote:
> Is there a reason this might vary over the lifetime of the serializer? If not,
> it's one fewer virtual call if this is just a configured boolean, like
> SetSupportsLegacyWireFormat.

Following the SetSupportsLegacyWireFormat would, actually,
allow it to vary over the lifetime of the serializer, wouldn't it?

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode3940
include/v8.h:3940: Isolate* isolate, TransferrableModule&);
On 2017/03/13 18:53:38, jbroman wrote:
> (destructive) lvalue-ref parameter here is strange. I'd have expected
> pass-by-value or rvalue-ref.

It's remnants from debugging something. What I want is const
TransferrableModule&. Pass-by-value doesn't work by design (no copy); and
rvalue-ref isn't what I want either, because the ownership stays with the
caller.

https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:765: Handle<Context> native_context =
isolate->native_context();
On 2017/03/13 18:53:38, jbroman wrote:
> remove separate change (I assume debugging code)?

yup! thanks for the catch.

https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-objects....
src/wasm/wasm-objects.cc:225: Handle<Context> ctx = isolate->native_context();
On 2017/03/13 18:53:38, jbroman wrote:
> remove separate change (I assume debugging code)?

Done.

[Mircea Trofin, 2017-03-15 18:04:24]

On 2017/03/15 18:02:29, Mircea Trofin wrote:
> https://codereview.chromium.org/2748473004/diff/120001/include/v8.h
> File include/v8.h (right):
> 
>
https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1718
> include/v8.h:1718: virtual Maybe<uint32_t> GetWasmModuleTransferId(
> On 2017/03/13 18:53:37, jbroman wrote:
> > Here and elsewhere: Before landing, add comments to new public API, like the
> > nearby methods?
> 
> Done.
> 
>
https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1831
> include/v8.h:1831: uint32_t id);
> On 2017/03/13 18:53:38, jbroman wrote:
> > Ditto: we've tried to reduce the number of virtual calls to the delegate.
> Could
> > this be implemented as something like:
> > 
> > void TransferWasmModule(uint32_t transfer_id, Local<WasmCompiledModule>
> module);
> > 
> > (analogous to how array buffers work)
> 
> This is more like transferring SharedArrayBuffers, hence a design similar to
> GetSharedBufferId().
> 
>
https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1832
> include/v8.h:1832: virtual bool AllowInlineWasm() const { return false; }
> On 2017/03/13 18:53:37, jbroman wrote:
> > Is there a reason this might vary over the lifetime of the serializer? If
not,
> > it's one fewer virtual call if this is just a configured boolean, like
> > SetSupportsLegacyWireFormat.
> 
> Following the SetSupportsLegacyWireFormat would, actually,
> allow it to vary over the lifetime of the serializer, wouldn't it?
> 
>
https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode3940
> include/v8.h:3940: Isolate* isolate, TransferrableModule&);
> On 2017/03/13 18:53:38, jbroman wrote:
> > (destructive) lvalue-ref parameter here is strange. I'd have expected
> > pass-by-value or rvalue-ref.
> 
> It's remnants from debugging something. What I want is const
> TransferrableModule&. Pass-by-value doesn't work by design (no copy); and
> rvalue-ref isn't what I want either, because the ownership stays with the
> caller.
> 
> https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-js.cc
> File src/wasm/wasm-js.cc (right):
> 
>
https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-js.cc#ne...
> src/wasm/wasm-js.cc:765: Handle<Context> native_context =
> isolate->native_context();
> On 2017/03/13 18:53:38, jbroman wrote:
> > remove separate change (I assume debugging code)?
> 
> yup! thanks for the catch.
> 
>
https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-objects.cc
> File src/wasm/wasm-objects.cc (right):
> 
>
https://codereview.chromium.org/2748473004/diff/120001/src/wasm/wasm-objects....
> src/wasm/wasm-objects.cc:225: Handle<Context> ctx = isolate->native_context();
> On 2017/03/13 18:53:38, jbroman wrote:
> > remove separate change (I assume debugging code)?
> 
> Done.

To answer the spec question, the remaining issue is how we want to signal error,
which shouldn't impact this part of the design.

[commit-bot: I haz the power, 2017-03-15 18:28:00]

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

[commit-bot: I haz the power, 2017-03-15 18:28:01]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-03-16 20:07:21]

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

[commit-bot: I haz the power, 2017-03-16 20:07:28]

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-03-16 20:14:17]

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

[commit-bot: I haz the power, 2017-03-16 20:14:18]

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

[Mircea Trofin, 2017-03-16 20:17:08]

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

[commit-bot: I haz the power, 2017-03-16 20:17:17]

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

[Mircea Trofin, 2017-03-16 20:17:27]

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1831
include/v8.h:1831: uint32_t id);
On 2017/03/15 18:02:28, Mircea Trofin wrote:
> On 2017/03/13 18:53:38, jbroman wrote:
> > Ditto: we've tried to reduce the number of virtual calls to the delegate.
> Could
> > this be implemented as something like:
> > 
> > void TransferWasmModule(uint32_t transfer_id, Local<WasmCompiledModule>
> module);
> > 
> > (analogous to how array buffers work)
> 
> This is more like transferring SharedArrayBuffers, hence a design similar to
> GetSharedBufferId().

As discussed offline - changed the design on deserializer side to match
TransferSharedArrayBuffer, for the sake of similarity. We don't believe virtual
calls matter much in this case, but we do care about reducing chattiness across
boundary between embedder and V8. We want to revisit the design on both SAB and
wasm side, and opened v8:6106 to track.

[commit-bot: I haz the power, 2017-03-16 20:38:43]

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

[commit-bot: I haz the power, 2017-03-16 20:38:44]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-03-17 14:44:37]

lgtm once a couple comments are resolved, and once you deal with
v8_linux_blink_rel (possibly just by suppressing the failures on the Blink side)

https://codereview.chromium.org/2748473004/diff/200001/src/value-serializer.h
File src/value-serializer.h (right):

https://codereview.chromium.org/2748473004/diff/200001/src/value-serializer.h...
src/value-serializer.h:306: std::vector<Handle<WasmModuleObject>>
transferred_wasm_modules_;
Hmm. This seems to require that the handles remain valid long after they are
passed in, which depends on HandleScope lifetime. The way
|array_buffer_transfer_map_| (and, for that matter, |id_map_|) works is that the
(optional) lookup table is held as a heap object by one global handle, and the
map then holds the objects.

Put another way, as a user I'd have expected this to be a legitimate use of the
API:

{
  v8::HandleScope handleScope(isolate);
  v8::Local<v8::WasmCompiledModule> module = ...;
  deserializer.TransferWasmCompiledModule(0, module);
}
deserializer.ReadValue(...);

https://codereview.chromium.org/2748473004/diff/200001/test/unittests/value-s...
File test/unittests/value-serializer-unittest.cc (right):

https://codereview.chromium.org/2748473004/diff/200001/test/unittests/value-s...
test/unittests/value-serializer-unittest.cc:2660: void
ThrowDataCloneError(Local<String> message) override {}
Are you expecting this to be unreached? (If so, NOTREACHED or
ASSERT_TRUE(false)?). If not, maybe add an implementation like the other
overrides of ThrowDataCloneError in this test?

[Mircea Trofin, 2017-03-17 16:21:07]

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

[commit-bot: I haz the power, 2017-03-17 16:21:17]

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

[Mircea Trofin, 2017-03-17 16:52:31]

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

[commit-bot: I haz the power, 2017-03-17 16:52:38]

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

[Mircea Trofin, 2017-03-17 16:55:32]

ptal - as agreed offline, went back to the delgate-based deserialization

https://codereview.chromium.org/2748473004/diff/200001/src/value-serializer.h
File src/value-serializer.h (right):

https://codereview.chromium.org/2748473004/diff/200001/src/value-serializer.h...
src/value-serializer.h:306: std::vector<Handle<WasmModuleObject>>
transferred_wasm_modules_;
On 2017/03/17 14:44:37, jbroman wrote:
> Hmm. This seems to require that the handles remain valid long after they are
> passed in, which depends on HandleScope lifetime. The way
> |array_buffer_transfer_map_| (and, for that matter, |id_map_|) works is that
the
> (optional) lookup table is held as a heap object by one global handle, and the
> map then holds the objects.
> 
> Put another way, as a user I'd have expected this to be a legitimate use of
the
> API:
> 
> {
>   v8::HandleScope handleScope(isolate);
>   v8::Local<v8::WasmCompiledModule> module = ...;
>   deserializer.TransferWasmCompiledModule(0, module);
> }
> deserializer.ReadValue(...);

As discussed offline - this alternative appears to introduce more complexity
than the previous solution, even considering the difference with SABs. We agreed
to go back to the delegate-based mechanism, and address quickly thereafter
v8:6106.

https://codereview.chromium.org/2748473004/diff/200001/test/unittests/value-s...
File test/unittests/value-serializer-unittest.cc (right):

https://codereview.chromium.org/2748473004/diff/200001/test/unittests/value-s...
test/unittests/value-serializer-unittest.cc:2660: void
ThrowDataCloneError(Local<String> message) override {}
On 2017/03/17 14:44:37, jbroman wrote:
> Are you expecting this to be unreached? (If so, NOTREACHED or
> ASSERT_TRUE(false)?). If not, maybe add an implementation like the other
> overrides of ThrowDataCloneError in this test?

Done.

[jbroman, 2017-03-17 17:08:32]

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

https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.c...
src/value-serializer.cc:809: Maybe<uint32_t> transfer_id =
delegate_->GetWasmModuleTransferId(
Do these calls into the delegate need to promote scheduled exceptions correctly,
e.g. using RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION?

https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.c...
src/value-serializer.cc:1623:
->GetWasmModuleFromId(reinterpret_cast<v8::Isolate*>(isolate_),
Do these calls into the delegate need to promote scheduled exceptions correctly,
e.g. using RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION? (like ReadHostObject does,
for instance)

[Mircea Trofin, 2017-03-17 17:16:11]

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

https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.c...
src/value-serializer.cc:809: Maybe<uint32_t> transfer_id =
delegate_->GetWasmModuleTransferId(
On 2017/03/17 17:08:32, jbroman wrote:
> Do these calls into the delegate need to promote scheduled exceptions
correctly,
> e.g. using RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION?

I don't think so, the transferring doesn't throw (unless a bug triggers
{D}CHECK, but that's different)

https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.c...
src/value-serializer.cc:1623:
->GetWasmModuleFromId(reinterpret_cast<v8::Isolate*>(isolate_),
On 2017/03/17 17:08:32, jbroman wrote:
> Do these calls into the delegate need to promote scheduled exceptions
correctly,
> e.g. using RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION? (like ReadHostObject does,
> for instance)

I don't think so, the transferring doesn't throw (unless a bug triggers
{D}CHECK, but that's different)

[commit-bot: I haz the power, 2017-03-17 17:17:40]

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

[commit-bot: I haz the power, 2017-03-17 17:17:41]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-03-17 17:28:24]

On 2017/03/17 at 17:16:11, mtrofin wrote:
> https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.cc
> File src/value-serializer.cc (right):
> 
>
https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.c...
> src/value-serializer.cc:809: Maybe<uint32_t> transfer_id =
delegate_->GetWasmModuleTransferId(
> On 2017/03/17 17:08:32, jbroman wrote:
> > Do these calls into the delegate need to promote scheduled exceptions
correctly,
> > e.g. using RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION?
> 
> I don't think so, the transferring doesn't throw (unless a bug triggers
{D}CHECK, but that's different)
> 
>
https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.c...
> src/value-serializer.cc:1623:
->GetWasmModuleFromId(reinterpret_cast<v8::Isolate*>(isolate_),
> On 2017/03/17 17:08:32, jbroman wrote:
> > Do these calls into the delegate need to promote scheduled exceptions
correctly,
> > e.g. using RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION? (like ReadHostObject
does,
> > for instance)
> 
> I don't think so, the transferring doesn't throw (unless a bug triggers
{D}CHECK, but that's different)

If you don't think delegates should be permitted to throw an exception here
(keeping in mind that Blink is not necessarily going to be the only
implementation of the delegate interface), please document that.

Given the signature, I'd expect clients to be permitted to schedule here. In
fact, your default implementations do. The rest of this code assumes that any
scheduled exception has been promoted.

[Mircea Trofin, 2017-03-17 18:33:05]

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

[commit-bot: I haz the power, 2017-03-17 18:33:10]

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

[Mircea Trofin, 2017-03-17 18:35:41]

On 2017/03/17 17:28:24, jbroman wrote:
> On 2017/03/17 at 17:16:11, mtrofin wrote:
> >
https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.cc
> > File src/value-serializer.cc (right):
> > 
> >
>
https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.c...
> > src/value-serializer.cc:809: Maybe<uint32_t> transfer_id =
> delegate_->GetWasmModuleTransferId(
> > On 2017/03/17 17:08:32, jbroman wrote:
> > > Do these calls into the delegate need to promote scheduled exceptions
> correctly,
> > > e.g. using RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION?
> > 
> > I don't think so, the transferring doesn't throw (unless a bug triggers
> {D}CHECK, but that's different)
> > 
> >
>
https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.c...
> > src/value-serializer.cc:1623:
> ->GetWasmModuleFromId(reinterpret_cast<v8::Isolate*>(isolate_),
> > On 2017/03/17 17:08:32, jbroman wrote:
> > > Do these calls into the delegate need to promote scheduled exceptions
> correctly,
> > > e.g. using RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION? (like ReadHostObject
> does,
> > > for instance)
> > 
> > I don't think so, the transferring doesn't throw (unless a bug triggers
> {D}CHECK, but that's different)
> 
> If you don't think delegates should be permitted to throw an exception here
> (keeping in mind that Blink is not necessarily going to be the only
> implementation of the delegate interface), please document that.
> 
> Given the signature, I'd expect clients to be permitted to schedule here. In
> fact, your default implementations do. The rest of this code assumes that any
> scheduled exception has been promoted.

I see - I agree. Changed as suggested + tests.

Also made the change that the default behavior of the serialization delegate is
akin to no delegate - i.e. we inline-serialize; then, the deserialization
delegate's 
default behavior is similar to no delegate, in that transfer won't work (and
inline
works only as opt-in, but that's not a delegate-controlled behavior); and the 
embedder must make an explicit decision re. transfers.

[commit-bot: I haz the power, 2017-03-17 18:41:20]

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

[commit-bot: I haz the power, 2017-03-17 18:41:21]

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

[jbroman, 2017-03-17 19:08:49]

lgtm with comments

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

https://codereview.chromium.org/2748473004/diff/260001/src/value-serializer.c...
src/value-serializer.cc:1627: RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate_,
JSObject);
This should be in the false path. If an exception is thrown, GetWasmModuleFromId
should return an empty MaybeLocal (and so we won't reach here.)

https://codereview.chromium.org/2748473004/diff/260001/src/value-serializer.c...
src/value-serializer.cc:1634: return MaybeHandle<JSObject>();
nit: this might be slightly clearer with early return:

if (!ReadVarint<uint32_t>().To(&transfer_id) || !delegate_ ||
      !delegate_
          ->GetWasmModuleFromId(reinterpret_cast<v8::Isolate*>(isolate_),
                                transfer_id)
          .ToLocal(&module_value)) {
  RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate_, JSObject);
  return MaybeHandle<JSObject>();
}

uint32_t id = next_id_++;
// ...

[Mircea Trofin, 2017-03-17 21:50:15]

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

[commit-bot: I haz the power, 2017-03-17 21:50:21]

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-03-17 22:16:10]

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

[commit-bot: I haz the power, 2017-03-17 22:16:11]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-03-17 22:16:35]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2017-03-17 22:16:36]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2748473004/#ps280001
(title: "feedback")

[commit-bot: I haz the power, 2017-03-17 22:16:41]

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-03-17 22:16:42]

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

[commit-bot: I haz the power, 2017-03-17 22:16:43]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[Mircea Trofin, 2017-03-17 22:16:48]

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

https://codereview.chromium.org/2748473004/diff/260001/src/value-serializer.c...
src/value-serializer.cc:1627: RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate_,
JSObject);
On 2017/03/17 19:08:48, jbroman wrote:
> This should be in the false path. If an exception is thrown,
GetWasmModuleFromId
> should return an empty MaybeLocal (and so we won't reach here.)

Acknowledged.

https://codereview.chromium.org/2748473004/diff/260001/src/value-serializer.c...
src/value-serializer.cc:1634: return MaybeHandle<JSObject>();
On 2017/03/17 19:08:48, jbroman wrote:
> nit: this might be slightly clearer with early return:
> 
> if (!ReadVarint<uint32_t>().To(&transfer_id) || !delegate_ ||
>       !delegate_
>           ->GetWasmModuleFromId(reinterpret_cast<v8::Isolate*>(isolate_),
>                                 transfer_id)
>           .ToLocal(&module_value)) {
>   RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate_, JSObject);
>   return MaybeHandle<JSObject>();
> }
> 
> uint32_t id = next_id_++;
> // ...

Done.

[bradnelson, 2017-03-20 18:23:50]

The CQ bit was checked by bradnelson@chromium.org

[bradnelson, 2017-03-20 18:23:51]

lgtm

[commit-bot: I haz the power, 2017-03-20 18:23:58]

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-03-20 19:03:25]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1490034230363510,
"parent_rev": "b1841eecb4d603e2f98f9464b4bef9d32b6e140b", "commit_rev":
"99743ad460ea5b9795ba9d70a074e75d7362a3d1"}

[commit-bot: I haz the power, 2017-03-20 19:03:31]

Description was changed from

==========
[wasm] Transferrable modules

We want to restrict structured cloning in Chrome to:
- postMessage senders and receivers that are co-located 
in the same process
- indexedDB (just https).

For context, on the Chrome side, we will achieve the postMessage part 
by using a mechanism similar to transferrables: the 
SerializedScriptValue will have a list of wasm modules, separate from 
the serialized data stream; and this list won't be copied cross 
process boundaries. The IDB part is achieved by explicitly opting in
reading/writing to the serialization stream. To block attack vectors 
in IPC cases, the default for deserialization will be to expect data 
in the wasm transfers list.

This change is the V8 side necessary to enabling this design. We 
introduce TransferrableModule, an opaque datatype exposed to the
embedder. Internally, TransferrableModules are just serialized data,
because we don't have a better mechanism, at the moment, for
de-contextualizing/re-contextualizing wasm modules (wrt Isolate and 
Context).

The chrome defaults will be implemented in the 
serialization/deserialization delegates on that side. For the v8 side 
of things, in the absence of a serialization delegate, the V8
serializer will write to serialization stream. In the absence of a 
deserialization delegate, the deserializer won't work. This asymmetry
is intentional - it communicates to the embedder the need to make a 
policy decision, otherwise wasm serialization/deserialization won't 
work "out of the box".

BUG=v8:6079
==========

to

==========
[wasm] Transferrable modules

We want to restrict structured cloning in Chrome to:
- postMessage senders and receivers that are co-located 
in the same process
- indexedDB (just https).

For context, on the Chrome side, we will achieve the postMessage part 
by using a mechanism similar to transferrables: the 
SerializedScriptValue will have a list of wasm modules, separate from 
the serialized data stream; and this list won't be copied cross 
process boundaries. The IDB part is achieved by explicitly opting in
reading/writing to the serialization stream. To block attack vectors 
in IPC cases, the default for deserialization will be to expect data 
in the wasm transfers list.

This change is the V8 side necessary to enabling this design. We 
introduce TransferrableModule, an opaque datatype exposed to the
embedder. Internally, TransferrableModules are just serialized data,
because we don't have a better mechanism, at the moment, for
de-contextualizing/re-contextualizing wasm modules (wrt Isolate and 
Context).

The chrome defaults will be implemented in the 
serialization/deserialization delegates on that side. For the v8 side 
of things, in the absence of a serialization delegate, the V8
serializer will write to serialization stream. In the absence of a 
deserialization delegate, the deserializer won't work. This asymmetry
is intentional - it communicates to the embedder the need to make a 
policy decision, otherwise wasm serialization/deserialization won't 
work "out of the box".

BUG=v8:6079

Review-Url: https://codereview.chromium.org/2748473004
Cr-Commit-Position: refs/heads/master@{#43955}
Committed:
https://chromium.googlesource.com/v8/v8/+/99743ad460ea5b9795ba9d70a074e75d736...
==========

[commit-bot: I haz the power, 2017-03-20 19:03:33]

Committed patchset #13 (id:280001) as
https://chromium.googlesource.com/v8/v8/+/99743ad460ea5b9795ba9d70a074e75d736...

[Michael Achenbach, 2017-03-21 13:54:00]

A revert of this CL (patchset #13 id:280001) has been created in
https://codereview.chromium.org/2762163002/ 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.