[wasm] enable wasm structured cloning in specific cases

[wasm] enable wasm structured cloning in specific cases

Using the mechanism introduced in V8 with https://codereview.chromium.org/2748473004, we restrict wasm structured cloning 
to: 
1) senders and receivers in the same process. This is achieved, similar to shared 
array buffers, by defaulting wasm serialization to using a transfer list internal to
SerializedScriptValue. The transfer list is not IPC-ed, which achieves the desired 
restriction

2) IndexedDB, when the origin is secure. This is achieved by explicitly opting in, on 
both serialization and deserialization side, into inline (i.e. in the stream) 
(de)serialization, if the origin is secure. This last check is done on the serialization 
side, which should be sufficient, since we trust the IDB layer.

In the process of adding tests for in-process iframe, I found crbug.com/700788. Left
the test in the CL, but currently disabled. Also, didn't add a cross-origin iframe test 
because, currently, that wouldn't add value (because of this bug)
 
BUG=chromium:703650
Review-Url: https://codereview.chromium.org/2749503002
Cr-Commit-Position: refs/heads/master@{#460328}
Committed: https://chromium.googlesource.com/chromium/src/+/110b0ecc2600e97fdc7496fb99fb309cab806242

[Mircea Trofin, 2017-03-13 05:14:35]

Description was changed from

==========
[wasm] enable wasm structured cloning in specific cases

BUG=
==========

to

==========
[wasm] enable wasm structured cloning in specific cases

BUG=
==========

[Mircea Trofin, 2017-03-13 05:14:36]

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

[Mircea Trofin, 2017-03-22 06:02:37]

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

[commit-bot: I haz the power, 2017-03-22 06:03:13]

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-22 07:20:03]

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

[commit-bot: I haz the power, 2017-03-22 07:20:04]

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

[Mircea Trofin, 2017-03-22 07:26:19]

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

[commit-bot: I haz the power, 2017-03-22 07:26:30]

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-22 09:34:40]

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

[commit-bot: I haz the power, 2017-03-22 09:34:41]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-03-22 15:25:00]

Description was changed from

==========
[wasm] enable wasm structured cloning in specific cases

BUG=
==========

to

==========
[wasm] enable wasm structured cloning in specific cases

Using the mechanism introduced in V8 with
https://codereview.chromium.org/2748473004, we restrict wasm structured cloning 
to: 
1) senders and receivers in the same process. This is achieved, similar to
shared 
array buffers, by defaulting wasm serialization to using a transfer list
internal to
SerializedScriptValue. The transfer list is not IPC-ed, which achieves the
desired 
restriction

2) IndexedDB, when the origin is secure. This is achieved by explicitly opting
in, on 
both serialization and deserialization side, into inline (i.e. in the stream) 
(de)serialization, if the origin is secure. This last check is done on the
serialization 
side, which should be sufficient, since we trust the IDB layer.

In the process of adding tests for in-process iframe, I found crbug.com/700788.
Left
the test in the CL, but currently disabled. Also, didn't add a cross-origin
iframe test 
because, currently, that wouldn't add value (because of this bug)
 
BUG=chromium:703650
==========

[Mircea Trofin, 2017-03-22 15:25:00]

mtrofin@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2017-03-22 17:41:07]

I'm confused about the intended behavior when attempting to store modules to
Indexed DB in non-secure contexts. It looks like the value is silently ignored
with no error, and on retrieval is a type that isn't tested?

(The serialization implementation in the non-secure looks like it's done in such
a way that the data is stored out-of-line and just dropped at some point.)

I would expect a DataCloneError to be thrown in this case, as we do for other
uncloneable types. Do we have a specification for desired behavior here?

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js
(right):

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:10: return
new Promise( (resolve, reject) => {
nit: remove space between ( (

(not new in this CL)

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:12: .then
(mod => {
nit: remove space between then (

(not new in this CL)

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:24:
store.put(mod, module_key);
Hrm... I would have expected this to throw DataCloneError in the insecure case
rather than silently failing to serialize the data.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:25:
tx.oncomplete = function() {
not new in this CL, but maybe add: 

tx.onabort = function() { reject(transaction.error); };

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:49: var
instance = new WebAssembly.Instance(mod);
Since the constructor may throw, maybe wrap in a try/catch block where catch
calls reject() ?

(Not critical - it's just a difference between the test timing out or getting a
useful message. It'll fail either way.)

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:52:
reject(new Error(kErrorMsg));
We should be asserting something about what `mod` is in this case, at the very
least.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:65:
.then((res) => assert_equals(res, 2))
I don't think you want to use .then().catch() here because if the callback for
.then() throws it will propagate to the .catch().

You probably want the two-argument form of then(), i.e.

.then(
  result => assert_equals(result, 2),
  error => assert_unreached(error)
);

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:75:
.then(assert_unreached)
same as above re: two argument form of then()

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:429:
// we don't expect scenarios with numerous wasm modules being transferred
Nit: capitalize. Also...  I'm not sure what this comment is telling the reader.
Is it a hint that something is not optimized?

[blink-reviews, 2017-03-22 18:08:41]

We don't yet have a spec, Domenic is ironing that one out.

Happy to throw in the IDB case - Domenic?


On Wed, Mar 22, 2017 at 10:41 AM <jsbell@chromium.org> wrote:

> I'm confused about the intended behavior when attempting to store modules
> to
> Indexed DB in non-secure contexts. It looks like the value is silently
> ignored
> with no error, and on retrieval is a type that isn't tested?
>
> (The serialization implementation in the non-secure looks like it's done
> in such
> a way that the data is stored out-of-line and just dropped at some point.)
>
> I would expect a DataCloneError to be thrown in this case, as we do for
> other
> uncloneable types. Do we have a specification for desired behavior here?
>
>
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> File
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js
> (right):
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:10:
> return new Promise( (resolve, reject) => {
> nit: remove space between ( (
>
> (not new in this CL)
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:12:
> .then (mod => {
> nit: remove space between then (
>
> (not new in this CL)
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:24:
> store.put(mod, module_key);
> Hrm... I would have expected this to throw DataCloneError in the
> insecure case rather than silently failing to serialize the data.
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:25:
> tx.oncomplete = function() {
> not new in this CL, but maybe add:
>
> tx.onabort = function() { reject(transaction.error); };
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:49:
> var instance = new WebAssembly.Instance(mod);
> Since the constructor may throw, maybe wrap in a try/catch block where
> catch calls reject() ?
>
> (Not critical - it's just a difference between the test timing out or
> getting a useful message. It'll fail either way.)
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:52:
> reject(new Error(kErrorMsg));
> We should be asserting something about what `mod` is in this case, at
> the very least.
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:65:
> .then((res) => assert_equals(res, 2))
> I don't think you want to use .then().catch() here because if the
> callback for .then() throws it will propagate to the .catch().
>
> You probably want the two-argument form of then(), i.e.
>
> .then(
> result => assert_equals(result, 2),
> error => assert_unreached(error)
> );
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:75:
> .then(assert_unreached)
> same as above re: two argument form of then()
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Sour...
> File
>
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
> (right):
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Sour...
>
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:429:
> // we don't expect scenarios with numerous wasm modules being
> transferred
> Nit: capitalize. Also... I'm not sure what this comment is telling the
> reader. Is it a hint that something is not optimized?
>
> https://codereview.chromium.org/2749503002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-03-22 18:08:41]

We don't yet have a spec, Domenic is ironing that one out.

Happy to throw in the IDB case - Domenic?


On Wed, Mar 22, 2017 at 10:41 AM <jsbell@chromium.org> wrote:

> I'm confused about the intended behavior when attempting to store modules
> to
> Indexed DB in non-secure contexts. It looks like the value is silently
> ignored
> with no error, and on retrieval is a type that isn't tested?
>
> (The serialization implementation in the non-secure looks like it's done
> in such
> a way that the data is stored out-of-line and just dropped at some point.)
>
> I would expect a DataCloneError to be thrown in this case, as we do for
> other
> uncloneable types. Do we have a specification for desired behavior here?
>
>
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> File
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js
> (right):
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:10:
> return new Promise( (resolve, reject) => {
> nit: remove space between ( (
>
> (not new in this CL)
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:12:
> .then (mod => {
> nit: remove space between then (
>
> (not new in this CL)
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:24:
> store.put(mod, module_key);
> Hrm... I would have expected this to throw DataCloneError in the
> insecure case rather than silently failing to serialize the data.
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:25:
> tx.oncomplete = function() {
> not new in this CL, but maybe add:
>
> tx.onabort = function() { reject(transaction.error); };
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:49:
> var instance = new WebAssembly.Instance(mod);
> Since the constructor may throw, maybe wrap in a try/catch block where
> catch calls reject() ?
>
> (Not critical - it's just a difference between the test timing out or
> getting a useful message. It'll fail either way.)
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:52:
> reject(new Error(kErrorMsg));
> We should be asserting something about what `mod` is in this case, at
> the very least.
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:65:
> .then((res) => assert_equals(res, 2))
> I don't think you want to use .then().catch() here because if the
> callback for .then() throws it will propagate to the .catch().
>
> You probably want the two-argument form of then(), i.e.
>
> .then(
> result => assert_equals(result, 2),
> error => assert_unreached(error)
> );
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:75:
> .then(assert_unreached)
> same as above re: two argument form of then()
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Sour...
> File
>
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
> (right):
>
>
>
https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Sour...
>
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:429:
> // we don't expect scenarios with numerous wasm modules being
> transferred
> Nit: capitalize. Also... I'm not sure what this comment is telling the
> reader. Is it a hint that something is not optimized?
>
> https://codereview.chromium.org/2749503002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jsbell, 2017-03-22 19:04:46]

On 2017/03/22 18:08:41, chromium-reviews wrote:
> We don't yet have a spec, Domenic is ironing that one out.
> 
> Happy to throw in the IDB case - Domenic?
> 

Heh, some of the formal spec work is on my plate too. :)

To be clear, I don't think we should block work on getting precise text in all
the relevant specs. Just a rough sketch somewhere so we can agree code === tests
=== intent is good enough for me for now.

[jsbell, 2017-03-22 21:05:47]

I believe the relevant spec language would go into WASM itself. e.g.

Per
https://html.spec.whatwg.org/multipage/infrastructure.html#serializable-objects

* WebAssembly.Module would need [Serializable]
* It would define serialization steps and deserialization steps
* The serialization steps would include something like "If the current context
is not a secure context, throw a "DataCloneError" DOMException."

... but we need Domenic to weigh in here.

[jbroman, 2017-03-22 21:09:41]

On 2017/03/22 at 21:05:47, jsbell wrote:
> I believe the relevant spec language would go into WASM itself. e.g.
> 
> Per
https://html.spec.whatwg.org/multipage/infrastructure.html#serializable-objects
> 
> * WebAssembly.Module would need [Serializable]
> * It would define serialization steps and deserialization steps
> * The serialization steps would include something like "If the current context
is not a secure context, throw a "DataCloneError" DOMException."
> 
> ... but we need Domenic to weigh in here.

FYI, that will make it impossible to send postMessage wasm from an insecure
context, put it in history.pushState, etc. May or may not be what you want.

[Mircea Trofin, 2017-03-22 23:30:20]

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

[commit-bot: I haz the power, 2017-03-22 23:31:15]

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-22 23:31:21]

incorporated feedback, except for the error behavior for IDB, where 
we're waiting for final conclusion. (same goes for postMessage).

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js
(right):

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:10: return
new Promise( (resolve, reject) => {
On 2017/03/22 17:41:07, jsbell wrote:
> nit: remove space between ( (
> 
> (not new in this CL)

Done.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:12: .then
(mod => {
On 2017/03/22 17:41:07, jsbell wrote:
> nit: remove space between then (
> 
> (not new in this CL)

Done.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:24:
store.put(mod, module_key);
On 2017/03/22 17:41:06, jsbell wrote:
> Hrm... I would have expected this to throw DataCloneError in the insecure case
> rather than silently failing to serialize the data.

Acknowledged. Will update once we get clarity over the desired behavior, if
that's OK.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:25:
tx.oncomplete = function() {
On 2017/03/22 17:41:07, jsbell wrote:
> not new in this CL, but maybe add: 
> 
> tx.onabort = function() { reject(transaction.error); };
> 

Done.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:49: var
instance = new WebAssembly.Instance(mod);
On 2017/03/22 17:41:06, jsbell wrote:
> Since the constructor may throw, maybe wrap in a try/catch block where catch
> calls reject() ?
> 
> (Not critical - it's just a difference between the test timing out or getting
a
> useful message. It'll fail either way.)

Done.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:52:
reject(new Error(kErrorMsg));
On 2017/03/22 17:41:07, jsbell wrote:
> We should be asserting something about what `mod` is in this case, at the very
> least.

Done.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:65:
.then((res) => assert_equals(res, 2))
On 2017/03/22 17:41:07, jsbell wrote:
> I don't think you want to use .then().catch() here because if the callback for
> .then() throws it will propagate to the .catch().
> 
> You probably want the two-argument form of then(), i.e.
> 
> .then(
>   result => assert_equals(result, 2),
>   error => assert_unreached(error)
> );

Done.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:75:
.then(assert_unreached)
On 2017/03/22 17:41:07, jsbell wrote:
> same as above re: two argument form of then()

Done.

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2749503002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:429:
// we don't expect scenarios with numerous wasm modules being transferred
On 2017/03/22 17:41:07, jsbell wrote:
> Nit: capitalize. Also...  I'm not sure what this comment is telling the
reader.
> Is it a hint that something is not optimized?

Done.

[domenic, 2017-03-22 23:45:08]

On 2017/03/22 at 21:09:41, jbroman wrote:
> On 2017/03/22 at 21:05:47, jsbell wrote:
> > I believe the relevant spec language would go into WASM itself. e.g.
> > 
> > Per
https://html.spec.whatwg.org/multipage/infrastructure.html#serializable-objects
> > 
> > * WebAssembly.Module would need [Serializable]
> > * It would define serialization steps and deserialization steps
> > * The serialization steps would include something like "If the current
context is not a secure context, throw a "DataCloneError" DOMException."
> > 
> > ... but we need Domenic to weigh in here.
> 
> FYI, that will make it impossible to send postMessage wasm from an insecure
context, put it in history.pushState, etc. May or may not be what you want.

The spec here really depends on whether we also want to block postMessage. Whose
responsibility is it to make that decision?

If we do, then jsbell's outline is about right. (It's a bit complicated since
WebAssembly.Module is not defined in Web IDL, so it's technically not a platform
object. I can work around that.)

If we don't, then the spec work is entirely in IndexedDB, where it defines the
procedure for turning a serialized Record into bytes-on-disk.

[Mircea Trofin, 2017-03-23 00:53:44]

On 2017/03/22 23:45:08, domenic wrote:
> On 2017/03/22 at 21:09:41, jbroman wrote:
> > On 2017/03/22 at 21:05:47, jsbell wrote:
> > > I believe the relevant spec language would go into WASM itself. e.g.
> > > 
> > > Per
>
https://html.spec.whatwg.org/multipage/infrastructure.html#serializable-objects
> > > 
> > > * WebAssembly.Module would need [Serializable]
> > > * It would define serialization steps and deserialization steps
> > > * The serialization steps would include something like "If the current
> context is not a secure context, throw a "DataCloneError" DOMException."
> > > 
> > > ... but we need Domenic to weigh in here.
> > 
> > FYI, that will make it impossible to send postMessage wasm from an insecure
> context, put it in history.pushState, etc. May or may not be what you want.
> 
> The spec here really depends on whether we also want to block postMessage.
Whose
> responsibility is it to make that decision?

From the discussions with esprehn and ikilpatrick, I was under the assumption
that decision was made as part of the wasm ship in 57 discussion, and the
conclusion was that we wanted the behavior I'm describing in this CL (where
postMessage is allowed in certain conditions).

What's left to spec, afaik, is the error cases in the 2 situations:
- what if I try and postMessage a module in a disallowed case
- what if I try and put in IDB a module when I'm in an insecure context.

I'm gathering the spec-ing is 2-part: one for what happens in the IDB case, the
other in the postMessage case.

> 
> If we do, then jsbell's outline is about right. (It's a bit complicated since
> WebAssembly.Module is not defined in Web IDL, so it's technically not a
platform
> object. I can work around that.)
> 
> If we don't, then the spec work is entirely in IndexedDB, where it defines the
> procedure for turning a serialized Record into bytes-on-disk.

Should I understand, then, that we can close on the IDB part of things given
jsbell's spec? I also assume the postMessage spec part is to come from Domenic.

[Mircea Trofin, 2017-03-23 00:54:22]

mtrofin@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[commit-bot: I haz the power, 2017-03-23 00:57:14]

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

[commit-bot: I haz the power, 2017-03-23 00:57:15]

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

[domenic, 2017-03-23 00:57:39]

On 2017/03/23 at 00:53:44, mtrofin wrote:

> Should I understand, then, that we can close on the IDB part of things given
jsbell's spec? I also assume the postMessage spec part is to come from Domenic.

Well, I think we need jsbell to spec it (in a different form than he did above,
since we want it to apply to IDB not postMessage). My guess is that he'll spec
it to throw a "DataCloneError" DOMException.

The postMessage spec is indeed to come from me---a major blocker just got
resolved so it's coming soon (TM) ^_^

[Mircea Trofin, 2017-03-23 01:04:47]

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

[commit-bot: I haz the power, 2017-03-23 01:05:12]

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-23 02:55:05]

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

[commit-bot: I haz the power, 2017-03-23 02:55:06]

Dry run: This issue passed the CQ dry run.

[jsbell, 2017-03-23 16:16:58]

On 2017/03/23 00:57:39, domenic wrote: 
> Well, I think we need jsbell to spec it (in a different form than he did
above,
> since we want it to apply to IDB not postMessage). My guess is that he'll spec
> it to throw a "DataCloneError" DOMException.

Sounds plausible. This will be somewhat strange in that the Serialize will
succeed (meaning, the entire object will be traversed/serialized), then there
will be a post-serialization pass to validate. This difference is observable
(i.e. getters later in the object will not be call if there's a truly
non-serializable object encountered first). Weird but the alternative is passing
some optional context into the serialization steps.

In terms of implementation here, is it sufficient to have IDBObjectStore check
the SSV's wasmModules() is empty(), then throw? That would match such a
"post-serialization" pass.

...

While I'm here, shouldn't TransferredWasmModulesArray be a WTF::Vector not a
std::vector?

[Mircea Trofin, 2017-03-23 18:07:16]

On 2017/03/23 16:16:58, jsbell wrote:
> On 2017/03/23 00:57:39, domenic wrote: 
> > Well, I think we need jsbell to spec it (in a different form than he did
> above,
> > since we want it to apply to IDB not postMessage). My guess is that he'll
spec
> > it to throw a "DataCloneError" DOMException.
> 
> Sounds plausible. This will be somewhat strange in that the Serialize will
> succeed (meaning, the entire object will be traversed/serialized), then there
> will be a post-serialization pass to validate. This difference is observable
> (i.e. getters later in the object will not be call if there's a truly
> non-serializable object encountered first). Weird but the alternative is
passing
> some optional context into the serialization steps.
> 

Would it be preferable we have a mode where attempt to serialize wasm failed to
start with?

> In terms of implementation here, is it sufficient to have IDBObjectStore check
> the SSV's wasmModules() is empty(), then throw? That would match such a
> "post-serialization" pass.
> 
> ...
> 
> While I'm here, shouldn't TransferredWasmModulesArray be a WTF::Vector not a
> std::vector?

What would the motivation be?

[jbroman, 2017-03-23 18:08:35]

On 2017/03/23 at 18:07:16, mtrofin wrote:
> On 2017/03/23 16:16:58, jsbell wrote:
> > On 2017/03/23 00:57:39, domenic wrote: 
> > > Well, I think we need jsbell to spec it (in a different form than he did
> > above,
> > > since we want it to apply to IDB not postMessage). My guess is that he'll
spec
> > > it to throw a "DataCloneError" DOMException.
> > 
> > Sounds plausible. This will be somewhat strange in that the Serialize will
> > succeed (meaning, the entire object will be traversed/serialized), then
there
> > will be a post-serialization pass to validate. This difference is observable
> > (i.e. getters later in the object will not be call if there's a truly
> > non-serializable object encountered first). Weird but the alternative is
passing
> > some optional context into the serialization steps.
> > 
> 
> Would it be preferable we have a mode where attempt to serialize wasm failed
to
> start with?
> 
> > In terms of implementation here, is it sufficient to have IDBObjectStore
check
> > the SSV's wasmModules() is empty(), then throw? That would match such a
> > "post-serialization" pass.
> > 
> > ...
> > 
> > While I'm here, shouldn't TransferredWasmModulesArray be a WTF::Vector not a
> > std::vector?
> 
> What would the motivation be?

As a matter of policy Blink prefers WTF containers and string types to STL ones.

[jsbell, 2017-03-23 19:44:45]

On 2017/03/23 18:07:16, Mircea Trofin wrote:
> Would it be preferable we have a mode where attempt to serialize wasm failed
to
> start with?

I'd definitely prefer that, but since it would be observable to script we'd need
to mirror it in the specs (i.e. StructuredSerialize() and [Serializable]
definitions). The benefit is that it allows for future serializable types to
have similar behavior and future storage APIs to enforce the same requirements
without duplicating logic, but maybe I'm being overly pedantic.

I'll defer to others here.

[Mircea Trofin, 2017-03-23 21:53:59]

On 2017/03/23 19:44:45, jsbell wrote:
> On 2017/03/23 18:07:16, Mircea Trofin wrote:
> > Would it be preferable we have a mode where attempt to serialize wasm failed
> to
> > start with?
> 
> I'd definitely prefer that, but since it would be observable to script we'd
need
> to mirror it in the specs (i.e. StructuredSerialize() and [Serializable]
> definitions). The benefit is that it allows for future serializable types to
> have similar behavior and future storage APIs to enforce the same requirements
> without duplicating logic, but maybe I'm being overly pedantic.
> 
> I'll defer to others here.

Would it be OK to move forward with this CL in the meantime - to avoid getting
it too stale?

We would still need to close for 59 on both error case issues.

[jsbell, 2017-03-24 16:51:13]

Yeah, I'm fine with moving forward, assuming we address before the branch.

Indexed DB tests and code change lgtm (with one test nit).

(I haven't re-reviewed the other files, though, since they'll need an owner
anyway)

https://codereview.chromium.org/2749503002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js
(right):

https://codereview.chromium.org/2749503002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:55:
reject(e);
Should return here, or move the resolve into the try{} block

[Mircea Trofin, 2017-03-24 22:51:22]

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

[commit-bot: I haz the power, 2017-03-24 22:51:54]

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-24 22:52:07]

Jeremy - are you OK to move forward with this CL?

Thanks!

https://codereview.chromium.org/2749503002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js
(right):

https://codereview.chromium.org/2749503002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:55:
reject(e);
On 2017/03/24 16:51:13, jsbell wrote:
> Should return here, or move the resolve into the try{} block

Done.

[commit-bot: I haz the power, 2017-03-24 23:47:14]

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

[commit-bot: I haz the power, 2017-03-24 23:47:15]

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

[Mircea Trofin, 2017-03-27 18:21:13]

On 2017/03/24 22:52:07, Mircea Trofin wrote:
> Jeremy - are you OK to move forward with this CL?
> 
> Thanks!
> 
>
https://codereview.chromium.org/2749503002/diff/80001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js
> (right):
> 
>
https://codereview.chromium.org/2749503002/diff/80001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:55:
> reject(e);
> On 2017/03/24 16:51:13, jsbell wrote:
> > Should return here, or move the resolve into the try{} block
> 
> Done.

Gentle nudge.

Thanks!

[jbroman, 2017-03-27 18:50:52]

I don't see the logic for (1) in this CL. Are you just relying on the fact that
we it gets dropped at the IPC boundary? Do you intend to throw a more helpful
DataCloneError in the future?

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:71:
window.location = get_host_info().AUTHENTICATED_ORIGIN +
window.location.pathname;
Doing this inside the test seems not to match what we do elsewhere (where we do
this check outside the test case). WDYT of that?

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html:7:
function TestSendToIFrame() {
As a matter of style, some of these console.log can probably be removed, as
they're implied by the test results. It's also implied that a promise failure
fails the test, so assert_unreached for that is unnecessary.

Finally, the control flow is slightly easier to read with async functions, but I
don't feel as strongly about that:

promise_test(async function() {
  var mod = await createWasmModule();
  assert_true(mod instanceof WebAssembly.Module);
  var ans = await new Promise((resolve, reject) => {
    var iframe = document.getElementById("iframe").contentWindow;
    iframe.postMessage(mod, '*');
    iframe.onmessage = event => resolve(event.data);
  });
  assert_equals(ans, 43);
}, "send wasm module to iframe");

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html:25:
promise_test(TestSendToIFrame, "send wasm module to worker");
to iframe, not worker, right?

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_service_worker_test.html
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_service_worker_test.html:7:
promise_test(t => {
Similarly, here (plus the use of global variables here is confusing):

promise_test(async test => {
  var registration = await service_worker_unregister_and_register(
      test, 'resources/service-worker.js', 'resources/blank.html');
  add_completion_callback(() => registration.unregister());
  var worker = registration.installing;
  var data = await new Promise((resolve, reject) => {
    var messageChannel = new MessageChannel();
    worker.postMessage({port: messageChannel.port2}, [messageChannel.port2]);
    worker.postMessage({compile: true});
    messageChannel.port1.onmessage = event => resolve(event.data);
  });
  assert_equals(data, undefined);
}, 'postMessaging wasm from a service worker should fail');

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:64:
std::vector<v8::WasmCompiledModule::TransferrableModule>;
Vector, not std::vector, in Blink

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:417:
return v8::MaybeLocal<v8::WasmCompiledModule>();
nit: The usual expectation is that functions returning MaybeLocal across the API
boundary throw an exception when they return nothing.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:428:
return v8::Nothing<uint32_t>();
nit: The usual expectation is that functions returning Maybe across the API
boundary throw an exception when they return nothing.

Also, should we NOTREACHED() in this case? Should v8 ever be calling this in the
case where we are writing wasm inline?

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:264: [CallWith=ScriptState]
any deserializeBufferContainingWasm(ArrayBuffer buffer);
Easy to change later, but if we want this, maybe give the existing methods a
dictionary parameter to configure this, rather than potentially adding variants
here? Something like:

any deserializeBuffer(ArrayBuffer buffer, optional
InternalsDeserializeBufferOptions);
ArrayBuffer serializeObject(any obj, optional InternalsSerializeObjectOptions);

[Mircea Trofin, 2017-03-27 20:48:33]

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

[commit-bot: I haz the power, 2017-03-27 20:49: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-27 20:52:33]

The logic for (1) is, as you pointed out, done by dropping 
the transfer list at IPC. I hoped my blurb in the CL would
amount to that - I can reword it perhaps?

The error cases aren't spec-ed yet for IDB (nor for postMessage). There's some
mention of this earlier in the review. This needs to be closed in the 59
timeframe. I'm waiting for Joshua and Domenic to close that part (the spec), and
will implement here, too. In the meantime, I'd like to make progress with the
CL, so it doesn't get stale.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_indexeddb_test.js:71:
window.location = get_host_info().AUTHENTICATED_ORIGIN +
window.location.pathname;
On 2017/03/27 18:50:52, jbroman wrote:
> Doing this inside the test seems not to match what we do elsewhere (where we
do
> this check outside the test case). WDYT of that?

Done.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html:7:
function TestSendToIFrame() {
On 2017/03/27 18:50:52, jbroman wrote:
> As a matter of style, some of these console.log can probably be removed, as
> they're implied by the test results. It's also implied that a promise failure
> fails the test, so assert_unreached for that is unnecessary.
> 
> Finally, the control flow is slightly easier to read with async functions, but
I
> don't feel as strongly about that:
> 
> promise_test(async function() {
>   var mod = await createWasmModule();
>   assert_true(mod instanceof WebAssembly.Module);
>   var ans = await new Promise((resolve, reject) => {
>     var iframe = document.getElementById("iframe").contentWindow;
>     iframe.postMessage(mod, '*');
>     iframe.onmessage = event => resolve(event.data);
>   });
>   assert_equals(ans, 43);
> }, "send wasm module to iframe");

The console were there for my debugging, thanks for catching.

I thought "Promises" were more "modern, the way forward", hence the sticking
with that pattern.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html:25:
promise_test(TestSendToIFrame, "send wasm module to worker");
On 2017/03/27 18:50:52, jbroman wrote:
> to iframe, not worker, right?

Done.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_service_worker_test.html
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_service_worker_test.html:7:
promise_test(t => {
On 2017/03/27 18:50:52, jbroman wrote:
> Similarly, here (plus the use of global variables here is confusing):
> 
> promise_test(async test => {
>   var registration = await service_worker_unregister_and_register(
>       test, 'resources/service-worker.js', 'resources/blank.html');
>   add_completion_callback(() => registration.unregister());
>   var worker = registration.installing;
>   var data = await new Promise((resolve, reject) => {
>     var messageChannel = new MessageChannel();
>     worker.postMessage({port: messageChannel.port2}, [messageChannel.port2]);
>     worker.postMessage({compile: true});
>     messageChannel.port1.onmessage = event => resolve(event.data);
>   });
>   assert_equals(data, undefined);
> }, 'postMessaging wasm from a service worker should fail');

Which vars are global?

Otherwise, same answer re. async vs promise - please let me know if I am
operating on an incorrect assumption, I'll be hapy to switch to your code sample
instead.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:64:
std::vector<v8::WasmCompiledModule::TransferrableModule>;
On 2017/03/27 18:50:52, jbroman wrote:
> Vector, not std::vector, in Blink

Done.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:417:
return v8::MaybeLocal<v8::WasmCompiledModule>();
On 2017/03/27 18:50:52, jbroman wrote:
> nit: The usual expectation is that functions returning MaybeLocal across the
API
> boundary throw an exception when they return nothing.

OK to do so, but I'd wait first for Domenic & Joshua to wrap up the spec.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:428:
return v8::Nothing<uint32_t>();
On 2017/03/27 18:50:52, jbroman wrote:
> nit: The usual expectation is that functions returning Maybe across the API
> boundary throw an exception when they return nothing.
> 
> Also, should we NOTREACHED() in this case? Should v8 ever be calling this in
the
> case where we are writing wasm inline?

See prev comment re. exceptions.

On the serialization side, v8 will try first to transfer, and if the delegate
provides Nothing<bool>, then it goes the inline path. So we can't NOTREACHED()
here.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:264: [CallWith=ScriptState]
any deserializeBufferContainingWasm(ArrayBuffer buffer);
On 2017/03/27 18:50:52, jbroman wrote:
> Easy to change later, but if we want this, maybe give the existing methods a
> dictionary parameter to configure this, rather than potentially adding
variants
> here? Something like:
> 
> any deserializeBuffer(ArrayBuffer buffer, optional
> InternalsDeserializeBufferOptions);
> ArrayBuffer serializeObject(any obj, optional
InternalsSerializeObjectOptions);

We don't have a scenario for configurations - let's do when we have a reason.

[commit-bot: I haz the power, 2017-03-27 22:16:32]

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

[commit-bot: I haz the power, 2017-03-27 22:16:33]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-03-28 15:27:25]

ok lgtm

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_local_iframe_test.html:7:
function TestSendToIFrame() {
On 2017/03/27 at 20:52:32, Mircea Trofin wrote:
> On 2017/03/27 18:50:52, jbroman wrote:
> > As a matter of style, some of these console.log can probably be removed, as
> > they're implied by the test results. It's also implied that a promise
failure
> > fails the test, so assert_unreached for that is unnecessary.
> > 
> > Finally, the control flow is slightly easier to read with async functions,
but I
> > don't feel as strongly about that:
> > 
> > promise_test(async function() {
> >   var mod = await createWasmModule();
> >   assert_true(mod instanceof WebAssembly.Module);
> >   var ans = await new Promise((resolve, reject) => {
> >     var iframe = document.getElementById("iframe").contentWindow;
> >     iframe.postMessage(mod, '*');
> >     iframe.onmessage = event => resolve(event.data);
> >   });
> >   assert_equals(ans, 43);
> > }, "send wasm module to iframe");
> 
> The console were there for my debugging, thanks for catching.
> 
> I thought "Promises" were more "modern, the way forward", hence the sticking
with that pattern.

async/await is the more recent (ES 2017) syntactic sugar for promises. "await"
causes the function to close over its scope and resume execution when the
promise is resolved (using |then| implicitly). You don't have to use it, but to
me it makes the code easier to read while still using (and hence having the
advantages of) promises.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_service_worker_test.html
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/wasm/wasm_service_worker_test.html:7:
promise_test(t => {
On 2017/03/27 at 20:52:33, Mircea Trofin wrote:
> On 2017/03/27 18:50:52, jbroman wrote:
> > Similarly, here (plus the use of global variables here is confusing):
> > 
> > promise_test(async test => {
> >   var registration = await service_worker_unregister_and_register(
> >       test, 'resources/service-worker.js', 'resources/blank.html');
> >   add_completion_callback(() => registration.unregister());
> >   var worker = registration.installing;
> >   var data = await new Promise((resolve, reject) => {
> >     var messageChannel = new MessageChannel();
> >     worker.postMessage({port: messageChannel.port2},
[messageChannel.port2]);
> >     worker.postMessage({compile: true});
> >     messageChannel.port1.onmessage = event => resolve(event.data);
> >   });
> >   assert_equals(data, undefined);
> > }, 'postMessaging wasm from a service worker should fail');
> 
> Which vars are global?

registration, worker and port are global (since they have no "var", "let" or
"const", nor are they a function parameter, they are implicitly properties of
the global |window| object).

> Otherwise, same answer re. async vs promise - please let me know if I am
operating on an incorrect assumption, I'll be hapy to switch to your code sample
instead.

Answered above. No objection to promises, though the language now allows a new
syntax that's essentially sugar on top.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:417:
return v8::MaybeLocal<v8::WasmCompiledModule>();
On 2017/03/27 at 20:52:33, Mircea Trofin wrote:
> On 2017/03/27 18:50:52, jbroman wrote:
> > nit: The usual expectation is that functions returning MaybeLocal across the
API
> > boundary throw an exception when they return nothing.
> 
> OK to do so, but I'd wait first for Domenic & Joshua to wrap up the spec.

Please leave a TODO, at least? (Especially since this is more a V8 API than web
API issue.)

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:428:
return v8::Nothing<uint32_t>();
On 2017/03/27 at 20:52:33, Mircea Trofin wrote:
> On 2017/03/27 18:50:52, jbroman wrote:
> > nit: The usual expectation is that functions returning Maybe across the API
> > boundary throw an exception when they return nothing.
> > 
> > Also, should we NOTREACHED() in this case? Should v8 ever be calling this in
the
> > case where we are writing wasm inline?
> 
> See prev comment re. exceptions.
> 
> On the serialization side, v8 will try first to transfer, and if the delegate
provides Nothing<bool>, then it goes the inline path. So we can't NOTREACHED()
here.

OK. Please add this to the documentation in include/v8.h (actually, I don't
think there's a comment there at all right now), because this is surprising
behaviour to me.

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2749503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:264: [CallWith=ScriptState]
any deserializeBufferContainingWasm(ArrayBuffer buffer);
On 2017/03/27 at 20:52:33, Mircea Trofin wrote:
> On 2017/03/27 18:50:52, jbroman wrote:
> > Easy to change later, but if we want this, maybe give the existing methods a
> > dictionary parameter to configure this, rather than potentially adding
variants
> > here? Something like:
> > 
> > any deserializeBuffer(ArrayBuffer buffer, optional
> > InternalsDeserializeBufferOptions);
> > ArrayBuffer serializeObject(any obj, optional
InternalsSerializeObjectOptions);
> 
> We don't have a scenario for configurations - let's do when we have a reason.

Acknowledged.

[Mircea Trofin, 2017-03-29 06:40:36]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2017-03-29 06:40:37]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2749503002/#ps160001
(title: "async")

[commit-bot: I haz the power, 2017-03-29 06:41: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-03-29 08:27:32]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490769636535890,
"parent_rev": "a0e931e8c3cf56801cad5148f99938e6f611f94a", "commit_rev":
"110b0ecc2600e97fdc7496fb99fb309cab806242"}

[commit-bot: I haz the power, 2017-03-29 08:28:16]

Description was changed from

==========
[wasm] enable wasm structured cloning in specific cases

Using the mechanism introduced in V8 with
https://codereview.chromium.org/2748473004, we restrict wasm structured cloning 
to: 
1) senders and receivers in the same process. This is achieved, similar to
shared 
array buffers, by defaulting wasm serialization to using a transfer list
internal to
SerializedScriptValue. The transfer list is not IPC-ed, which achieves the
desired 
restriction

2) IndexedDB, when the origin is secure. This is achieved by explicitly opting
in, on 
both serialization and deserialization side, into inline (i.e. in the stream) 
(de)serialization, if the origin is secure. This last check is done on the
serialization 
side, which should be sufficient, since we trust the IDB layer.

In the process of adding tests for in-process iframe, I found crbug.com/700788.
Left
the test in the CL, but currently disabled. Also, didn't add a cross-origin
iframe test 
because, currently, that wouldn't add value (because of this bug)
 
BUG=chromium:703650
==========

to

==========
[wasm] enable wasm structured cloning in specific cases

Using the mechanism introduced in V8 with
https://codereview.chromium.org/2748473004, we restrict wasm structured cloning 
to: 
1) senders and receivers in the same process. This is achieved, similar to
shared 
array buffers, by defaulting wasm serialization to using a transfer list
internal to
SerializedScriptValue. The transfer list is not IPC-ed, which achieves the
desired 
restriction

2) IndexedDB, when the origin is secure. This is achieved by explicitly opting
in, on 
both serialization and deserialization side, into inline (i.e. in the stream) 
(de)serialization, if the origin is secure. This last check is done on the
serialization 
side, which should be sufficient, since we trust the IDB layer.

In the process of adding tests for in-process iframe, I found crbug.com/700788.
Left
the test in the CL, but currently disabled. Also, didn't add a cross-origin
iframe test 
because, currently, that wouldn't add value (because of this bug)
 
BUG=chromium:703650

Review-Url: https://codereview.chromium.org/2749503002
Cr-Commit-Position: refs/heads/master@{#460328}
Committed:
https://chromium.googlesource.com/chromium/src/+/110b0ecc2600e97fdc7496fb99fb...
==========

[commit-bot: I haz the power, 2017-03-29 08:28:18]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/110b0ecc2600e97fdc7496fb99fb...