[wasm] Compile and Instantiation

[wasm] Compile and Instantiation

Implemented the WebAssembly.Module and WebAssembly.Instance
in terms of the WasmModule::CompileFunctions and
WasmModule::Instantiate APIs.

Added negative tests - for invalid module object.

BUG=

Committed: https://crrev.com/bd03c6429739592cd587a1803b8dcca271838fe6
Cr-Commit-Position: refs/heads/master@{#37775}

[Mircea Trofin, 2016-07-04 06:16:28]

Description was changed from

==========
[wasm] Compile and Instantiation

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

to

==========
[wasm] Compile and Instantiation

Implemented the WebAssembly.Module and WebAssembly.Instance
in terms of the WasmModule::CompileFunctions and 
WasmModule::Instantiate APIs.

Added negative tests - for invalid module object.

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

[Mircea Trofin, 2016-07-04 06:16:28]

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

[Mircea Trofin, 2016-07-04 06:29:09]

ptal

[rossberg, 2016-07-04 10:04:00]

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

https://codereview.chromium.org/2121593002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:322: if (compiled_module.is_null()) return nothing;
It seems like CompileFunctions eagerly raises exceptions in the error case, but
that breaks the asynchronous WebAssembly.compile method, which should capture
all exceptions in a rejected promise.

To make that work, CompileFunctions will need to take an ErrorThrower and report
all failures through that (and then use the thrower-Reify method in
WebAssemblyComile to get a handle on the exception).

As a temporary solution, WebAssemblyCompile could also implement a catch-handler
that reifies the exceptions retroactively. However, that won't fly any longer
once compilation will actually be performed asynchronously, e.g. when off-loaded
to another thread.

https://codereview.chromium.org/2121593002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:326: i::Handle<i::Map> map = i_isolate->factory()->NewMap(
It isn't desirable to create a new map creation for each module object. Can you
move this to the Install function and store the map in the native context?

https://codereview.chromium.org/2121593002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:388: if (args.Length() < 1 || !args[0]->IsObject()) {
This check is not sufficient (nor necessary). You should be checking that the
value has the module_sym property, like in the previous code, which brands it as
a module object.

https://codereview.chromium.org/2121593002/diff/20001/test/mjsunit/wasm/insta...
File test/mjsunit/wasm/instantiate-module-basic.js (right):

https://codereview.chromium.org/2121593002/diff/20001/test/mjsunit/wasm/insta...
test/mjsunit/wasm/instantiate-module-basic.js:62: // Negative tests.
We should probably have a few new positive tests, too. For example, testing that
instantiating the same module several times works correctly.

[Mircea Trofin, 2016-07-05 06:02:43]

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

https://codereview.chromium.org/2121593002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:388: if (args.Length() < 1 || !args[0]->IsObject()) {
On 2016/07/04 10:04:00, rossberg wrote:
> This check is not sufficient (nor necessary). You should be checking that the
> value has the module_sym property, like in the previous code, which brands it
as
> a module object.

Done.

https://codereview.chromium.org/2121593002/diff/20001/test/mjsunit/wasm/insta...
File test/mjsunit/wasm/instantiate-module-basic.js (right):

https://codereview.chromium.org/2121593002/diff/20001/test/mjsunit/wasm/insta...
test/mjsunit/wasm/instantiate-module-basic.js:62: // Negative tests.
On 2016/07/04 10:04:00, rossberg wrote:
> We should probably have a few new positive tests, too. For example, testing
that
> instantiating the same module several times works correctly.

True - actually, for that to work, we'll have to clone the compiled code.

[Mircea Trofin, 2016-07-12 02:36:52]

Patchset #3 (id:40001) has been deleted

[Mircea Trofin, 2016-07-12 02:51:50]

FYI - post-landing the cloning CL, multi-instantiation tests should pass, as
seen here.

Note that I haven't added globals tests, we don't have that support
in the test infra yet. I'll add that separately.

[Mircea Trofin, 2016-07-12 21:38:25]

On 2016/07/12 02:51:50, Mircea Trofin wrote:
> FYI - post-landing the cloning CL, multi-instantiation tests should pass, as
> seen here.
> 
> Note that I haven't added globals tests, we don't have that support
> in the test infra yet. I'll add that separately.

PTAL - the cloning CL is in, so this one is unblocked.

[rossberg, 2016-07-13 10:28:51]

On 2016/07/12 21:38:25, Mircea Trofin wrote:
> On 2016/07/12 02:51:50, Mircea Trofin wrote:
> > FYI - post-landing the cloning CL, multi-instantiation tests should pass, as
> > seen here.
> > 
> > Note that I haven't added globals tests, we don't have that support
> > in the test infra yet. I'll add that separately.
> 
> PTAL - the cloning CL is in, so this one is unblocked.

Hm, did you forget to upload some patch set? I don't see my other comments
addressed.

[Mircea Trofin, 2016-07-13 16:34:49]

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

[commit-bot: I haz the power, 2016-07-13 16:34:55]

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

[commit-bot: I haz the power, 2016-07-13 17:14:16]

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

[commit-bot: I haz the power, 2016-07-13 17:14:17]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-07-13 17:57:27]

On 2016/07/13 10:28:51, rossberg wrote:
> On 2016/07/12 21:38:25, Mircea Trofin wrote:
> > On 2016/07/12 02:51:50, Mircea Trofin wrote:
> > > FYI - post-landing the cloning CL, multi-instantiation tests should pass,
as
> > > seen here.
> > > 
> > > Note that I haven't added globals tests, we don't have that support
> > > in the test infra yet. I'll add that separately.
> > 
> > PTAL - the cloning CL is in, so this one is unblocked.
> 
> Hm, did you forget to upload some patch set? I don't see my other comments
> addressed.

Likely - I don't know what happened. Ptal - thanks!

[rossberg, 2016-07-14 13:30:28]

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

https://codereview.chromium.org/2121593002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:388: if (args.Length() < 1 || !args[0]->IsObject()) {
On 2016/07/05 06:02:42, Mircea Trofin wrote:
> On 2016/07/04 10:04:00, rossberg wrote:
> > This check is not sufficient (nor necessary). You should be checking that
the
> > value has the module_sym property, like in the previous code, which brands
it
> as
> > a module object.
> 
> Done.

Hm, I still don't see the code that addresses this comment. In its current form,
the CL is unsafe, because you could be looking at another kind of object that
just happens to have a FixedArray in its internal field 0 as well. You need the
private symbol to identify the object as a module unambiguously.

[Mircea Trofin, 2016-07-14 15:59:57]

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

[commit-bot: I haz the power, 2016-07-14 16:00:00]

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

[Mircea Trofin, 2016-07-14 16:02:58]

PTAL - thanks for your patience, and sorry for the back-and-forth. It 
looks like I accidentally deleted the patch where a bunch of these things
were done. 

Re. the module_sym, I also needed to check that the source is not 
undefined. I believe that's what we get for undefined arg[0].

Thanks!

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

https://codereview.chromium.org/2121593002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:322: if (compiled_module.is_null()) return nothing;
On 2016/07/04 10:04:00, rossberg wrote:
> It seems like CompileFunctions eagerly raises exceptions in the error case,
but
> that breaks the asynchronous WebAssembly.compile method, which should capture
> all exceptions in a rejected promise.
> 
> To make that work, CompileFunctions will need to take an ErrorThrower and
report
> all failures through that (and then use the thrower-Reify method in
> WebAssemblyComile to get a handle on the exception).
> 
> As a temporary solution, WebAssemblyCompile could also implement a
catch-handler
> that reifies the exceptions retroactively. However, that won't fly any longer
> once compilation will actually be performed asynchronously, e.g. when
off-loaded
> to another thread.

Done, + test

https://codereview.chromium.org/2121593002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:326: i::Handle<i::Map> map = i_isolate->factory()->NewMap(
On 2016/07/04 10:04:00, rossberg wrote:
> It isn't desirable to create a new map creation for each module object. Can
you
> move this to the Install function and store the map in the native context?

Done.

https://codereview.chromium.org/2121593002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:388: if (args.Length() < 1 || !args[0]->IsObject()) {
On 2016/07/14 13:30:28, rossberg wrote:
> On 2016/07/05 06:02:42, Mircea Trofin wrote:
> > On 2016/07/04 10:04:00, rossberg wrote:
> > > This check is not sufficient (nor necessary). You should be checking that
> the
> > > value has the module_sym property, like in the previous code, which brands
> it
> > as
> > > a module object.
> > 
> > Done.
> 
> Hm, I still don't see the code that addresses this comment. In its current
form,
> the CL is unsafe, because you could be looking at another kind of object that
> just happens to have a FixedArray in its internal field 0 as well. You need
the
> private symbol to identify the object as a module unambiguously.

Done (hopefully this time I won't delete my own patch again)

[commit-bot: I haz the power, 2016-07-14 16:23:54]

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

[commit-bot: I haz the power, 2016-07-14 16:23:55]

Dry run: This issue passed the CQ dry run.

[rossberg, 2016-07-14 16:23:59]

LGTM

> Re. the module_sym, I also needed to check that the source is not 
> undefined. I believe that's what we get for undefined arg[0].

Wouldn't CreateModuleObject have failed in that case? At least that should be
the invariant. I.e., if module_sym exists, it contains a valid buffer source.

However, this is JavaScript, so GetProperty will just return undefined (not a
null handle) if the property does not exist. That's probably what you were
seeing. So your fix is indeed needed.

[Mircea Trofin, 2016-07-14 16:25:25]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2016-07-14 16:25:36]

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

[commit-bot: I haz the power, 2016-07-14 16:27:56]

Description was changed from

==========
[wasm] Compile and Instantiation

Implemented the WebAssembly.Module and WebAssembly.Instance
in terms of the WasmModule::CompileFunctions and 
WasmModule::Instantiate APIs.

Added negative tests - for invalid module object.

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

to

==========
[wasm] Compile and Instantiation

Implemented the WebAssembly.Module and WebAssembly.Instance
in terms of the WasmModule::CompileFunctions and 
WasmModule::Instantiate APIs.

Added negative tests - for invalid module object.

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

[commit-bot: I haz the power, 2016-07-14 16:27:57]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-07-14 16:31:19]

Description was changed from

==========
[wasm] Compile and Instantiation

Implemented the WebAssembly.Module and WebAssembly.Instance
in terms of the WasmModule::CompileFunctions and 
WasmModule::Instantiate APIs.

Added negative tests - for invalid module object.

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

to

==========
[wasm] Compile and Instantiation

Implemented the WebAssembly.Module and WebAssembly.Instance
in terms of the WasmModule::CompileFunctions and
WasmModule::Instantiate APIs.

Added negative tests - for invalid module object.

BUG=

Committed: https://crrev.com/bd03c6429739592cd587a1803b8dcca271838fe6
Cr-Commit-Position: refs/heads/master@{#37775}
==========

[commit-bot: I haz the power, 2016-07-14 16:31:20]

Patchset 5 (id:??) landed as
https://crrev.com/bd03c6429739592cd587a1803b8dcca271838fe6
Cr-Commit-Position: refs/heads/master@{#37775}