1316447 - Baldr: add recent JS API additions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Luke Wagner [:luke]]

Namely WebAssembly.Module.(imports|exports) and WebAssembly.instantiate.

Comment 1

[Luke Wagner [:luke]]

Attachment: Module.imports and exports

Comment 2

[Luke Wagner [:luke]]

Attachment: hoist-sync-execute

Little bit of code motion to enable code sharing in next patch.

Comment 3

[Luke Wagner [:luke]]

Attachment: instantiate

WebAssembly.instantiate is also pretty easy; mostly just refactoring existing code so that it can be reused.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8809192 [details] [diff] [review]
Module.imports and exports

Review of attachment 8809192 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: js/src/wasm/WasmJS.cpp
@@ +526,5 @@
> +    }
> +
> +    *module = &unwrapped->as<WasmModuleObject>().module();
> +    return true;
> +

nit: blank line

@@ +532,5 @@
> +
> +struct KindNames
> +{
> +    RootedPropertyName kind;
> +    RootedPropertyName function;

I think this is unused (since you can use cx->names().function)?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8809276 [details] [diff] [review]
hoist-sync-execute

Review of attachment 8809276 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Promise.h
@@ +166,5 @@
>      // calling finishPromise().
>      virtual void execute() = 0;
> +
> +    // May be called in the absence of helper threads to synchronously execute
> +    // and finish a PromiseTask.

Is it called if and only if there are no helper threads (in which case we could assert)? (I guess I'll figure out in the next patch)

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8809277 [details] [diff] [review]
instantiate

Review of attachment 8809277 [details] [diff] [review]:
-----------------------------------------------------------------

Found one non-nit, I think: instantiate should return Instance *and* Module, per the design repo wording. This and the wrapped question have me want a second look. The Promise code is very clean and easy to understand!

::: js/src/wasm/WasmJS.cpp
@@ +1794,5 @@
> +        RootedWasmInstanceObject instanceObj(cx);
> +        if (!Instantiate(cx, *module, importObj, &instanceObj))
> +            return RejectWithPendingException(cx, promise);
> +
> +        RootedValue resolutionValue(cx, ObjectValue(*instanceObj));

This is not correct, per https://github.com/WebAssembly/design/blob/master/JS.md:

On success, the Promise is fulfilled with a plain JavaScript object pair {module, instance} containing the resulting WebAssembly.Module and WebAssembly.Instance

@@ +1822,5 @@
> +    RootedObject importObj(cx);
> +    if (!GetInstantiateArgs(cx, callArgs, &firstArg, &importObj))
> +        return RejectWithPendingException(cx, promise, callArgs);
> +
> +    if (firstArg->is<WasmModuleObject>()) {

It's a bit strange that the type checking of the first arg is made in two different functions. Could a wrapped Module be passed to this Instantiate? If so, I think it might be considered a bad arg in this impl.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8809277 [details] [diff] [review]
instantiate

Review of attachment 8809277 [details] [diff] [review]:
-----------------------------------------------------------------

(oops, so used to r+ your patches...)

Comment 8

[Hannes Verschore [:h4writer]]

I assume this is P1, based on the fact that we want WebAssembly in FF52.

Comment 9

[Luke Wagner [:luke]]

Attachment: instantiate

Phew, great catch!

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8809432 [details] [diff] [review]
instantiate

Review of attachment 8809432 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f290780813ad
Baldr: add Module.{imports,exports} (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b46d55b13bd
Baldr: hoist CanUseExtraThreads promise logic (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd4da2301e5
Baldr: add WebAssembly.instantiate (r=bbouvier)

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f290780813ad
https://hg.mozilla.org/mozilla-central/rev/4b46d55b13bd
https://hg.mozilla.org/mozilla-central/rev/bfd4da2301e5