[wasm] instantiate expressed in terms of compile

[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

BUG=chromium:697028
Review-Url: https://codereview.chromium.org/2806073002
Cr-Original-Commit-Position: refs/heads/master@{#44592}
Committed: https://chromium.googlesource.com/v8/v8/+/7829af3275ff4644a2d0a1270abe1a1e4415e9fb
Review-Url: https://codereview.chromium.org/2806073002
Cr-Commit-Position: refs/heads/master@{#44669}
Committed: https://chromium.googlesource.com/v8/v8/+/71cf4890d0a2bc3ac47597d724676df871a572d5

[Mircea Trofin, 2017-04-10 18:55:37]

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

[commit-bot: I haz the power, 2017-04-10 18:55: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-04-10 18:57:10]

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

[commit-bot: I haz the power, 2017-04-10 18:57:11]

Dry run: Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/38884)

[Mircea Trofin, 2017-04-10 19:31:01]

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

[commit-bot: I haz the power, 2017-04-10 19:31:11]

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

[commit-bot: I haz the power, 2017-04-10 19:50:41]

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

[commit-bot: I haz the power, 2017-04-10 19:50:42]

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-04-11 00:38:22]

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

[commit-bot: I haz the power, 2017-04-11 00:38:24]

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

[commit-bot: I haz the power, 2017-04-11 00:58:14]

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

[commit-bot: I haz the power, 2017-04-11 00:58:14]

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

[Mircea Trofin, 2017-04-11 01:35:06]

Description was changed from

==========
[wasm] instantiate expressed in terms of compile

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

to

==========
[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

1 test fails throughout, it's the public js-api repo counterpart of 
the js-api tests. See https://github.com/WebAssembly/spec/pull/457.


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

[Mircea Trofin, 2017-04-11 01:35:06]

mtrofin@chromium.org changed reviewers:
+ adamk@chromium.org, ahaas@chromium.org, bradnelson@chromium.org

[Mircea Trofin, 2017-04-11 13:24:49]

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

[commit-bot: I haz the power, 2017-04-11 13:24:53]

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

[commit-bot: I haz the power, 2017-04-11 13:47:31]

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

[commit-bot: I haz the power, 2017-04-11 13:47:32]

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

[Mircea Trofin, 2017-04-11 21:06:44]

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

[commit-bot: I haz the power, 2017-04-11 21:06:59]

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

[commit-bot: I haz the power, 2017-04-11 21:08:30]

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

[commit-bot: I haz the power, 2017-04-11 21:08:31]

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_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/39029)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/25932)

[Mircea Trofin, 2017-04-11 22:35:42]

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

[commit-bot: I haz the power, 2017-04-11 22:35:58]

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-04-11 22:49:42]

Patchset #6 (id:100001) has been deleted

[Mircea Trofin, 2017-04-11 22:49:52]

Patchset #4 (id:60001) has been deleted

[Mircea Trofin, 2017-04-11 22:53:17]

Description was changed from

==========
[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

1 test fails throughout, it's the public js-api repo counterpart of 
the js-api tests. See https://github.com/WebAssembly/spec/pull/457.


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

to

==========
[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

BUG=chromium:697028
==========

[commit-bot: I haz the power, 2017-04-11 23:03:12]

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

[commit-bot: I haz the power, 2017-04-11 23:03:12]

Dry run: This issue passed the CQ dry run.

[bradnelson, 2017-04-11 23:18:41]

lgtm

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

https://codereview.chromium.org/2806073002/diff/120001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:340: Local<Value> data = args[1];
You sure args does that automatically?
Old code didn't assume that?

https://codereview.chromium.org/2806073002/diff/120001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:405: Local<Value> data = args[1];
Same

https://codereview.chromium.org/2806073002/diff/120001/test/mjsunit/wasm/inst...
File test/mjsunit/wasm/instantiate-run-basic.js (right):

https://codereview.chromium.org/2806073002/diff/120001/test/mjsunit/wasm/inst...
test/mjsunit/wasm/instantiate-run-basic.js:31: .then(pair =>
pair.instance.exports.main(), assertUnreachable)
>80

[Mircea Trofin, 2017-04-11 23:33:18]

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

https://codereview.chromium.org/2806073002/diff/120001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:340: Local<Value> data = args[1];
On 2017/04/11 23:18:41, bradnelson wrote:
> You sure args does that automatically?
> Old code didn't assume that?

Yes: https://cs.chromium.org/chromium/src/v8/include/v8.h?dr=CSs&l=9167

Old code cared about type of the argument. I just want to set it in the closure.

https://codereview.chromium.org/2806073002/diff/120001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:405: Local<Value> data = args[1];
On 2017/04/11 23:18:41, bradnelson wrote:
> Same

See above.

[Mircea Trofin, 2017-04-11 23:33:24]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2017-04-11 23:33:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2806073002/#ps140001
(title: "fixes")

[commit-bot: I haz the power, 2017-04-11 23:33:30]

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

[commit-bot: I haz the power, 2017-04-12 00:01:05]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491953604096340,
"parent_rev": "70ad23791a21c0dd7ecef8d4d8dd30ff6fc291f6", "commit_rev":
"7829af3275ff4644a2d0a1270abe1a1e4415e9fb"}

[commit-bot: I haz the power, 2017-04-12 00:01:12]

Description was changed from

==========
[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

BUG=chromium:697028
==========

to

==========
[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

BUG=chromium:697028

Review-Url: https://codereview.chromium.org/2806073002
Cr-Commit-Position: refs/heads/master@{#44592}
Committed:
https://chromium.googlesource.com/v8/v8/+/7829af3275ff4644a2d0a1270abe1a1e441...
==========

[commit-bot: I haz the power, 2017-04-12 00:01:13]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/7829af3275ff4644a2d0a1270abe1a1e441...

[Michael Hablich, 2017-04-12 13:27:36]

A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/2810203002/ by hablich@chromium.org.

The reason for reverting is: Roll blocker:
https://bugs.chromium.org/p/chromium/issues/detail?id=710824.

[Mircea Trofin, 2017-04-13 05:40:34]

Description was changed from

==========
[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

BUG=chromium:697028

Review-Url: https://codereview.chromium.org/2806073002
Cr-Commit-Position: refs/heads/master@{#44592}
Committed:
https://chromium.googlesource.com/v8/v8/+/7829af3275ff4644a2d0a1270abe1a1e441...
==========

to

==========
[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

BUG=chromium:697028

Review-Url: https://codereview.chromium.org/2806073002
Cr-Commit-Position: refs/heads/master@{#44592}
Committed:
https://chromium.googlesource.com/v8/v8/+/7829af3275ff4644a2d0a1270abe1a1e441...
==========

[Mircea Trofin, 2017-04-13 05:40:39]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2017-04-13 05:40:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2806073002/#ps180001
(title: "fix")

[commit-bot: I haz the power, 2017-04-13 05:40:53]

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

[commit-bot: I haz the power, 2017-04-13 06:00:00]

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

[commit-bot: I haz the power, 2017-04-13 06:00:01]

Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/26003)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[Mircea Trofin, 2017-04-17 20:23:49]

Patchset #9 (id:200001) has been deleted

[Mircea Trofin, 2017-04-17 21:27:38]

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

[commit-bot: I haz the power, 2017-04-17 21:27:40]

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-04-17 21:32:30]

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

[Mircea Trofin, 2017-04-17 21:33:00]

PTAL.

Dmitri, if you could look at src/wasm/wasm-js.cc, for the 
MicrotasksScope usage.

Thanks!

[commit-bot: I haz the power, 2017-04-17 22:04:10]

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

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

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...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Mircea Trofin, 2017-04-17 22:14:49]

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

[commit-bot: I haz the power, 2017-04-17 22:14:59]

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

[commit-bot: I haz the power, 2017-04-17 22:30:08]

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

[commit-bot: I haz the power, 2017-04-17 22:30: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...)

[dgozman, 2017-04-17 22:34:29]

On 2017/04/17 21:33:00, Mircea Trofin wrote:
> PTAL.
> 
> Dmitri, if you could look at src/wasm/wasm-js.cc, for the 
> MicrotasksScope usage.
> 
> Thanks!

Microtasks look good, thanks.

[Mircea Trofin, 2017-04-17 22:59:47]

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

[commit-bot: I haz the power, 2017-04-17 22:59:49]

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

[commit-bot: I haz the power, 2017-04-17 23:28:09]

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

[commit-bot: I haz the power, 2017-04-17 23:28:11]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-04-17 23:46:19]

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

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:35: #define ASSIGN(type, var, expr)      \
The name we use for this inside the API is ASSIGN_RETURN_ON_EXCEPTION, and it
also includes a DCHECK that an exception is pending. I'd rather this be more
similar to what we use inside.

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:320: vanilla_instantiate->Call(context, args.Holder(), 1,
&module));
I really feel like it's weird to create a v8::Function around a C++ function
just to immediately call it.

I would prefer there to be a slightly more fine-grained factoring here, where
there's WebAssemblyInstiantiateImpl(), which takes isolate, module and ffi
arguments, and a WebAssemblyInsantiateImplCallback, which is used for the one
case where the v8::Function is actually passed to .then (down in
WebAssemblyInstantiate()). I think that might even be less code overall. WDYT?

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:325: ASSIGN(String, module_name,
This and the above can't meaningfully fail (the API says that they'd only fail
if their length was huge). I'd rather see them use

Local<String> module_name = 
    String::NewFromOneByte(...).ToLocalChecked();

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:330: DO_BOOL(ret->CreateDataProperty(context, module_name,
module));
I don't believe that either of these could possibly throw. Can you wrap them in
CHECK() instead of your DO_BOOL macro? Like,

CHECK(ret->CreateDataProperty(...).FromJust());

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:405: DO_BOOL(resolver->Resolve(context, first_arg_value));
This would be the last use of DO_BOOL; given that, I think I'd rather see:

if (!resolver->Resolve(...).IsJust()) return;

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:420: Function::New(context, instantiator, data));
It's a little wasteful to create a new function each time this is called, but I
guess performance here is not a big concern?

[Mircea Trofin, 2017-04-18 01:03:11]

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

[commit-bot: I haz the power, 2017-04-18 01:03:23]

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-04-18 01:03:54]

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

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:35: #define ASSIGN(type, var, expr)      \
On 2017/04/17 23:46:19, adamk wrote:
> The name we use for this inside the API is ASSIGN_RETURN_ON_EXCEPTION, and it
> also includes a DCHECK that an exception is pending. I'd rather this be more
> similar to what we use inside.

Done.

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:320: vanilla_instantiate->Call(context, args.Holder(), 1,
&module));
On 2017/04/17 23:46:19, adamk wrote:
> I really feel like it's weird to create a v8::Function around a C++ function
> just to immediately call it.
> 
> I would prefer there to be a slightly more fine-grained factoring here, where
> there's WebAssemblyInstiantiateImpl(), which takes isolate, module and ffi
> arguments, and a WebAssemblyInsantiateImplCallback, which is used for the one
> case where the v8::Function is actually passed to .then (down in
> WebAssemblyInstantiate()). I think that might even be less code overall. WDYT?

Done.

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:325: ASSIGN(String, module_name,
On 2017/04/17 23:46:19, adamk wrote:
> This and the above can't meaningfully fail (the API says that they'd only fail
> if their length was huge). I'd rather see them use
> 
> Local<String> module_name = 
>     String::NewFromOneByte(...).ToLocalChecked();

Done.

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:330: DO_BOOL(ret->CreateDataProperty(context, module_name,
module));
On 2017/04/17 23:46:19, adamk wrote:
> I don't believe that either of these could possibly throw. Can you wrap them
in
> CHECK() instead of your DO_BOOL macro? Like,
> 
> CHECK(ret->CreateDataProperty(...).FromJust());

Done.

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:405: DO_BOOL(resolver->Resolve(context, first_arg_value));
On 2017/04/17 23:46:19, adamk wrote:
> This would be the last use of DO_BOOL; given that, I think I'd rather see:
> 
> if (!resolver->Resolve(...).IsJust()) return;

Done.

https://codereview.chromium.org/2806073002/diff/260001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:420: Function::New(context, instantiator, data));
On 2017/04/17 23:46:19, adamk wrote:
> It's a little wasteful to create a new function each time this is called, but
I
> guess performance here is not a big concern?

That, but also, we need a closure to hold on to the ffi until the promise is
resolved.

[commit-bot: I haz the power, 2017-04-18 01:27:14]

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

[commit-bot: I haz the power, 2017-04-18 01:27:15]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-04-18 01:29:29]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2017-04-18 01:29:29]

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

[commit-bot: I haz the power, 2017-04-18 01:29:34]

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

[commit-bot: I haz the power, 2017-04-18 01:31:17]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1492478969190060,
"parent_rev": "1236335551297ea54ae416c860bbd3ae89807993", "commit_rev":
"71cf4890d0a2bc3ac47597d724676df871a572d5"}

[commit-bot: I haz the power, 2017-04-18 01:31:25]

Description was changed from

==========
[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

BUG=chromium:697028

Review-Url: https://codereview.chromium.org/2806073002
Cr-Commit-Position: refs/heads/master@{#44592}
Committed:
https://chromium.googlesource.com/v8/v8/+/7829af3275ff4644a2d0a1270abe1a1e441...
==========

to

==========
[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed 
change uncovered bugs. 

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example. 
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using 
the compile -> new Instance alternative, in particular wrt promise 
creation. By reusing code paths, we uncover more bugs, and keep 
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly. 
Otherwise, we'd need that same implementation on the blink side. The 
negative is maintenance: imagine if the bugs I mentioned could only be 
found when running in Blink. 

BUG=chromium:697028

Review-Url: https://codereview.chromium.org/2806073002
Cr-Original-Commit-Position: refs/heads/master@{#44592}
Committed:
https://chromium.googlesource.com/v8/v8/+/7829af3275ff4644a2d0a1270abe1a1e441...
Review-Url: https://codereview.chromium.org/2806073002
Cr-Commit-Position: refs/heads/master@{#44669}
Committed:
https://chromium.googlesource.com/v8/v8/+/71cf4890d0a2bc3ac47597d724676df871a...
==========

[commit-bot: I haz the power, 2017-04-18 01:31:27]

Committed patchset #12 (id:280001) as
https://chromium.googlesource.com/v8/v8/+/71cf4890d0a2bc3ac47597d724676df871a...

[adamk, 2017-04-18 17:17:22]

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

https://codereview.chromium.org/2806073002/diff/280001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:39: DCHECK(i_isolate->has_pending_exception()); \
You still need to return here, otherwise we'll crash due to the empty var
farther down in the function.

https://codereview.chromium.org/2806073002/diff/280001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:128: i::MaybeHandle<i::JSReceiver> GetValueAsImports(const
Local<Value>& arg,
Style nit: we pass Locals and Handles by value (they're just pointers)

https://codereview.chromium.org/2806073002/diff/280001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:323: .ToLocal(&instance))
Style nit: please add { } around if-statement body if it doesn't all fit on one
line.