1303079 - Baldr: add WebAssembly.(Compile|Runtime)Error

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

... and switch over compile and runtime errors to use them.

Comment 1

[Luke Wagner [:luke]]

Depends on bug 1287220 to avoid conflicting with JSMSG_WASM_DECODE_FAIL change.

Comment 2

[Luke Wagner [:luke]]

Attachment: fix-errors

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8791798 [details] [diff] [review]
fix-errors

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

Thank you for the mass conversion to wasmFailValidateText!

::: js/src/builtin/TestingFunctions.cpp
@@ +608,5 @@
> +        ok = wasm::BinaryToText(cx, bytes, length, buffer);
> +    if (!ok) {
> +        if (!cx->isExceptionPending())
> +            JS_ReportError(cx, "wasm binary to text print error");
> +        return false;

In the else branch, should we report OOM?

::: js/src/jit-test/tests/wasm/binary.js
@@ +104,5 @@
>  const U32MAX_LEB = [255, 255, 255, 255, 15];
>  
>  const wasmEval = (code, imports) => new WebAssembly.Instance(new WebAssembly.Module(code), imports).exports;
>  
> +assertErrorMessage(() => wasmEval(toU8([])), CompileError, magicError);

This file can also use wasmFailValidateText, right?

::: js/src/jit-test/tests/wasm/jsapi.js
@@ +68,5 @@
>  assertErrorMessage(() => new Module(undefined), TypeError, "first argument must be an ArrayBuffer or typed array object");
>  assertErrorMessage(() => new Module(1), TypeError, "first argument must be an ArrayBuffer or typed array object");
>  assertErrorMessage(() => new Module({}), TypeError, "first argument must be an ArrayBuffer or typed array object");
> +assertErrorMessage(() => new Module(new Uint8Array()), CompileError, /failed to match magic number/);
> +assertErrorMessage(() => new Module(new ArrayBuffer()), CompileError, /failed to match magic number/);

\o/

::: js/src/jit-test/tests/wasm/memory.js
@@ +101,5 @@
>      assertErrorMessage(() => storeModule(type, ext, offset, align).store(base, value), Error, /invalid or out-of-range index/);
>  }
>  
>  function badLoadModule(type, ext) {
> +    return wasmFailValidateText( `(module (func (param i32) (${type}.load${ext} (get_local 0))) (export "" 0))`, /can't touch memory/);

no need to return anymore

@@ +106,4 @@
>  }
>  
>  function badStoreModule(type, ext) {
> +    return wasmFailValidateText(`(module (func (param i32) (${type}.store${ext} (get_local 0) (${type}.const 0))) (export "" 0))`, /can't touch memory/);

ditto

Comment 4

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
Thanks!

> > +        if (!cx->isExceptionPending())
> > +            JS_ReportError(cx, "wasm binary to text print error");
> > +        return false;
> 
> In the else branch, should we report OOM?

No, I don't think so since, by test, there is already an exception pending.

> > +assertErrorMessage(() => wasmEval(toU8([])), CompileError, magicError);
> 
> This file can also use wasmFailValidateText, right?

Almost, but no, b/c it's eval'ing binary, not text.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62999f48c433
Baldr: add WebAssembly.(Compile|Runtime)Error (r=bbouvier)

Comment 6

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

Jit tests were frequently failing like https://treeherder.mozilla.org/logviewer.html#?job_id=36396696&repo=mozilla-inbound when this push landed.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/625573d37c8b

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06baf9c96c5b
Baldr: add WebAssembly.(Compile|Runtime)Error (r=bbouvier)

Comment 8

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/06baf9c96c5b

Comment 9

[Gerry Chang [:gchang]]

Hi :luke,
Will WebAssembly still be shipped in 51? If yes, do you think this patch is worth uplifting to 51?

Comment 10

[Luke Wagner [:luke]]

I know this changed since last time we talked, but the goal is now 52, so no.  Thanks for asking, though.

Comment 11

[Gerry Chang [:gchang]]

Mark 51 as won't fix as wasm will move to 52.