Closed
Bug 1303079
Opened 8 years ago
Closed 8 years ago
Baldr: add WebAssembly.(Compile|Runtime)Error
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(1 file)
(deleted),
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
... and switch over compile and runtime errors to use them.
Assignee | ||
Comment 1•8 years ago
|
||
Depends on bug 1287220 to avoid conflicting with JSMSG_WASM_DECODE_FAIL change.
Depends on: 1287220
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8791798 -
Flags: review?(bbouvier)
Comment 3•8 years ago
|
||
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•8 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.
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•8 years ago
|
Flags: needinfo?(luke)
Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/06baf9c96c5b Baldr: add WebAssembly.(Compile|Runtime)Error (r=bbouvier)
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06baf9c96c5b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 9•8 years ago
|
||
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•8 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)
Comment 11•8 years ago
|
||
Mark 51 as won't fix as wasm will move to 52.
You need to log in
before you can comment on or make changes to this bug.
Description
•