Baldr: add WebAssembly.(Compile|Runtime)Error

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
... and switch over compile and runtime errors to use them.
(Assignee)

Comment 1

2 years ago
Depends on bug 1287220 to avoid conflicting with JSMSG_WASM_DECODE_FAIL change.
Depends on: 1287220
(Assignee)

Updated

2 years ago
Blocks: 1188259
(Assignee)

Comment 2

2 years ago
Created attachment 8791798 [details] [diff] [review]
fix-errors
Attachment #8791798 - Flags: review?(bbouvier)
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
Attachment #8791798 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 4

2 years ago
(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

2 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62999f48c433
Baldr: add WebAssembly.(Compile|Runtime)Error (r=bbouvier)
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
Flags: needinfo?(luke)
(Assignee)

Updated

2 years ago
Flags: needinfo?(luke)

Comment 7

2 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06baf9c96c5b
Baldr: add WebAssembly.(Compile|Runtime)Error (r=bbouvier)

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06baf9c96c5b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi :luke,
Will WebAssembly still be shipped in 51? If yes, do you think this patch is worth uplifting to 51?
Flags: needinfo?(luke)
(Assignee)

Comment 10

2 years ago
I know this changed since last time we talked, but the goal is now 52, so no.  Thanks for asking, though.
Flags: needinfo?(luke)
Mark 51 as won't fix as wasm will move to 52.
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.