1343579 - Baldr: make Pause-on-Exception break in wasm code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Luke Wagner [:luke]]

Right now, if the "Pause on Exception" button is enabled and you trap in wasm, the exception will propagate until the first JS caller and that's where the Debugger will break.  Ideally, we'd break at the trapping instruction.  I think this would be a *super* useful option for anyone debugging out-of-bounds or div-by-zero etc traps.

Attached is .html/.wasm test case.

Comment 1

[Luke Wagner [:luke]]

Attachment: trap.tar.gz

Comment 2

[Yury Delendik (:yury)]

Attachment: Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

Review commit: https://reviewboard.mozilla.org/r/116950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116950/

Comment 3

[Yury Delendik (:yury)]

Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116950/diff/1-2/

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

https://reviewboard.mozilla.org/r/116950/#review120766

Is implementing isInCatchScope the only thing needed to get this functionality in the frontend, or are there more patches?

::: js/src/vm/Debugger.cpp:6903
(Diff revision 2)
> -    size_t offset;
> -    if (!ScriptOffset(cx, script, args[0], &offset))
> +  public:
> +    explicit DebuggerScriptIsInCatchScopeMatcher(JSContext* cx, size_t offset) : cx_(cx), offset_(offset) { }
> +    using ReturnType = bool;
> +
> +    ReturnType match(HandleScript script) {
> +        if (!EnsureScriptOffsetIsValid(cx_, script, offset_))

This will not propagate errors correctly. The matcher returns true/false to mean "is in catch scope", not fallibility.

Comment 5

[Yury Delendik (:yury)]

Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116950/diff/2-3/

Comment 6

[Yury Delendik (:yury)]

Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

https://reviewboard.mozilla.org/r/116950/#review120766

That's correct. Implementing isInCatchScope fixes the test case provided in comment 0.

> This will not propagate errors correctly. The matcher returns true/false to mean "is in catch scope", not fallibility.

Fixed. Thanks for spotting it.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

https://reviewboard.mozilla.org/r/116950/#review120800

The implementation of isInCatchScope looks fine, though I don't really understand how this enables the frontend debugger to pause on wasm exceptions.

Comment 8

[Yury Delendik (:yury)]

Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116950/diff/3-4/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/014cd346dff3
Implements Debugger.Script.isInCatchScope for wasm scripts. r=shu

Comment 10

[Yury Delendik (:yury)]

Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

https://reviewboard.mozilla.org/r/116950/#review120800

We implemented most of the logic in previous bugs (e.g. bug 1286948). The devtools server part was breaking on isInCatchScope at https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.js#1832, after fixing Debugger API for wasm script everything started working.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/014cd346dff3