Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS API for traps #89

Closed
aheejin opened this issue Oct 31, 2019 · 5 comments · Fixed by #93
Closed

JS API for traps #89

aheejin opened this issue Oct 31, 2019 · 5 comments · Fixed by #93

Comments

@aheejin
Copy link
Member

aheejin commented Oct 31, 2019

I proposed not to catch traps with wasm catch instruction in #1 (comment). Even though it has not been decided yet, in this issue I'd like to discuss JS API changes in case we decide not to catch traps as I proposed.

To make this behavior consistent, traps should not be catchable both before and after they hit a JS frame. To this end, it might be convenient if we have JS API to create a WebAssembly trap, like

trap = new WebAssembly.Trap()

So that we can maintain its trap identity in both JS and wasm.

But the current JS API specifies if an exported function call traps, the JS frame that called the exported function should throw a WebAssembly.RuntimeError.

So I'm wondering, should we preserve this behavior? If we should, we may have to make catch not catch RuntimeError exception. RuntimeError contains various error conditions other than traps. I think most cases of RuntimeError are unrecoverable errors, but would there be any case of non-trap RuntimeError that should be caught by catch instruction?

Or, would it be better to create WebAssembly.Trap and change the JS API so that in case of a wasm trap, its embedding JS frame throws not RuntimeError but Trap instead, and catch instruction catches all other JS exceptions except for Trap?

cc @dschuff @mstarzinger @Ms2ger @lukewagner @rossberg
@backes (In case this has implementation issues)

This was referenced Oct 31, 2019
@rossberg
Copy link
Member

AFAICT, RuntimeErrors correspond to traps and only traps in Wasm. What other possible conditions are you referring to (except a user manually creating one)? There shouldn't be any need for introducing a new Error class for traps -- and that would break existing code anyway.

So, I believe that filtering RuntimeError from catch is the only backwards compatible way in which we can prevent Wasm from catching traps after an intermediate JS frame.

If we do that, then we probably also want to filter stack overflow and out of memory exceptions, which are implementation dependent, but could originate from Wasm code as well.

@aheejin
Copy link
Member Author

aheejin commented Oct 31, 2019

I think you're right... there are many conditions that can cause an error but it looks like traps are the only case that can case an error during func_invoke. Would there be implementation implication to v8? @mstarzinger

As you said, stack overflow and OOM can throw any errors depending on the implementations. What would be a good way to make them not catchable? We can start with some frequent errors (such as RangeError), but I'm not sure if we can prevent all of them from being caught, given that they can be anything.

@dschuff
Copy link
Member

dschuff commented Oct 31, 2019

Just to be clear, I think @rossberg is saying that the wasm catch instruction shouldn't catch traps or stack overflow or OOM exceptions, which makes sense. For the RangeErrors which arise from e.g. OOMs, I think the engine will have to track internally whether it was originally a wasm OOM or something else, (so that when it unwinds from a wasm frame, through a JS frame, and back into another wasm frame, it can tell the difference).

I think once any of those propagates into a JS frame it would have to be just a regular JS exception though, right? Uncatchable JS exceptions aren't a thing AFAIK, and I guess changing the JS type thrown by a trap would be a breaking change?

@aheejin
Copy link
Member Author

aheejin commented Oct 31, 2019

I think once any of those propagates into a JS frame it would have to be just a regular JS exception though, right? Uncatchable JS exceptions aren't a thing AFAIK, and I guess changing the JS type thrown by a trap would be a breaking change?

Yes, given that we can just not catch RuntimeError, I don't think we need to introduce a new Trap type after all, so I think we don't need a breaking change.

@mstarzinger
Copy link

I think you're right... there are many conditions that can cause an error but it looks like traps are the only case that can case an error during func_invoke. Would there be implementation implication to v8? @mstarzinger

Yes, it should be doable for V8 to filter RuntimeError objects that represent an error originating from func_invoke during stack unwinding. During unwinding we would make sure that such exceptions cannot be caught by a wasm catch, and hence can only be caught by the embedder (i.e. a JavaScript try {} catch(e) {} statement). This works independently of how many JS frames and wasm frames are interleaved on the stack or in which order they are interleaved.

It is important however to base this distinction between such a RuntimeError exception and any other arbitrary exception on a predicate that is not observable by JavaScript. What I mean by that is we should make sure this distinction is not based on the instanceof operator, because that predicate can be intercepted in JS by setting the @@hasInstance property on RuntimeError. This in turn would make VM internals of how exception handling and stack unwinding is implemented in different VMs observable. Using an internal slot not visible to JavaScript on the other hand should be safe.

Just to be clear, I think @rossberg is saying that the wasm catch instruction shouldn't catch traps or stack overflow or OOM exceptions, which makes sense. For the RangeErrors which arise from e.g. OOMs, I think the engine will have to track internally whether it was originally a wasm OOM or something else, (so that when it unwinds from a wasm frame, through a JS frame, and back into another wasm frame, it can tell the difference).

Good point. The internal slot I mentioned above could also be used to distinguish a e.g. RangeError that originated from within wasm from one that originated from somewhere else.

I think once any of those propagates into a JS frame it would have to be just a regular JS exception though, right? Uncatchable JS exceptions aren't a thing AFAIK, and I guess changing the JS type thrown by a trap would be a breaking change?

Agreed, once any exception propagates into JS it should be represented as a regular JS exception (modulo the internal slot I mentioned above). This should be doable without changing the JS type of any existing exception.

aheejin added a commit to aheejin/exception-handling that referenced this issue Dec 30, 2019
This adds overview on traps not being caught by the `catch` instruction
and its relationship with the JS API.
Closes WebAssembly#1 and closes WebAssembly#89.
aheejin added a commit that referenced this issue Jan 10, 2020
This adds overview on traps not being caught by the `catch` instruction
and its relationship with the JS API.
Closes #1 and closes #89.
ioannad pushed a commit to ioannad/exception-handling that referenced this issue Jun 6, 2020
* Upgrade to latest Sphinx release (2.4.4) (#1171)

Fixes #1157

* Support 4GB of memory both in initial and max.

* [interpreter] Strictify and specify .bin.wast format (#1173)

* Merge nontrapping-float-to-int proposal into spec (#1143)

See the non-trapping-float-to-int-conversions proposal here:

https://github.com/WebAssembly/nontrapping-float-to-int-conversions

* Merge sign-extension-ops proposal into spec (#1144)

See the sign-extension-ops proposal here:

https://github.com/WebAssembly/sign-extension-ops

This PR is built on top of #1143 (merge nontrapping-float-to-int).

* Merge multi-value proposal into spec (#1145)

See the multi-value proposal here:

https://github.com/WebAssembly/multi-value

This PR is built on top of the following PRs:

* #1143 (merge nontrapping-float-to-int)
* #1144 (merge sign-extension-ops)

* [interpreter] Remove junk in README

* [interpreter] Remove junk in README

* [spec] Fix grammar for fractions (#1178)

* [spec] Add missing i64.extend32_s syntax (#1179)

* [js-api][web-api] Fix some markup errors.

* Add a README to the proposals directory.

* Add more address overflow tests (#1188)

There are already tests for effective address overflow, but those have a
large value baked into the offset. These tests all use `1` as the
immediate offset, and use `-1` for the address on the stack, which may
be compiled differently.

* Add a test for non-treelike behavior of stack (#961)

We've recently found a bug in a WebAssembly library we've been working
with where we're mapping WebAssembly to a tree-like IR internally. The
way we parse into this representation, however, has a bug when the
function isn't itself tree-like but rather exibits properties that
exploit a stack machine. For example this isn't so straightforward to
represent in a tree-like fashion:

    (import "" "a" (func $foo))
    (import "" "b" (func $foo (result i32)))
    (func (result i32)
      call $b
      call $b
      call $a
      i32.xor)

The extra `call $a` in the middle is valid `WebAssembly` but needs
special treatment when converting to a more tree-like IR format. I
figured it'd be good to ensure there's a spec test covering this case as
we currently pass the suite of spec tests but still contain this bug!

* [js-api] Various editorial improvements.

* [js-api] Replace pseudo-ASCII characters by normal ones.

This also required disambiguating the references to "module", as there are now
two definitions by that name.

* [js-api] Improve prose in 'run a host function'.

* [js-api] Improve some of the multi-value prose.

* Synchronize js-api tests.

* Add script to synchronize js-api tests.

Co-authored-by: Ng Zhi An <[email protected]>
Co-authored-by: Alon Zakai <[email protected]>
Co-authored-by: Ben Smith <[email protected]>
Co-authored-by: Ms2ger <[email protected]>
Co-authored-by: Alex Crichton <[email protected]>
ioannad pushed a commit to ioannad/exception-handling that referenced this issue Jun 6, 2020
This adds overview on traps not being caught by the `catch` instruction
and its relationship with the JS API.
Closes WebAssembly#1 and closes WebAssembly#89.
ioannad pushed a commit to ioannad/exception-handling that referenced this issue Feb 23, 2021
This adds overview on traps not being caught by the `catch` instruction
and its relationship with the JS API.
Closes WebAssembly#1 and closes WebAssembly#89.
aheejin added a commit to aheejin/emscripten that referenced this issue May 7, 2022
We used to implement `abort()` with throwing a JS exception. emscripten-core#9730
changed it to `RuntimeError`, because it is the class a trap becomes
once it hits a JS frame.

But this turned out to be not working, because Wasm EH currently does
not assume all `RuntimeError`s are from traps; it decides whether a
`RuntimeError` is from a trap or not based on a hidden field within the
object. Relevant past discussion:
WebAssembly/exception-handling#89 (comment)

So at the moment we don't have a way of throwing a trap from JS, which
is inconvenient. I think we eventually want to make JS API for this, but
for the moment, this PR resorts to a wasm function that traps. This at
least makes calling `std::terminate` work. So far calling it exhausted
the call stack and aborted.

Fixes emscripten-core#16407.
aheejin added a commit to aheejin/emscripten that referenced this issue May 7, 2022
We used to implement `abort()` with throwing a JS exception. emscripten-core#9730
changed it to `RuntimeError`, because it is the class a trap becomes
once it hits a JS frame.

But this turned out to be not working, because Wasm EH currently does
not assume all `RuntimeError`s are from traps; it decides whether a
`RuntimeError` is from a trap or not based on a hidden field within the
object. Relevant past discussion:
WebAssembly/exception-handling#89 (comment)

So at the moment we don't have a way of throwing a trap from JS, which
is inconvenient. I think we eventually want to make JS API for this, but
for the moment, this PR resorts to a wasm function that traps. This at
least makes calling `std::terminate` work. So far calling it exhausted
the call stack and aborted.

Fixes emscripten-core#16407.
aheejin added a commit to aheejin/emscripten that referenced this issue May 10, 2022
`noexcept` function shouldn't throw, so `noexcept` function code
generation is to `invoke` every function call in those functions and in
case they throw, call `std::terminate`. This codegen comes from clang
and native platforms do this too. So in wasm, they become something like
```wasm
try
  function body
catch_all
  call std::terminate
end
```

`std::terminate` calls `std::__terminate`. Both of `std::terminate` and
`std::__terminate` are `noexcept` now. So that means their code is
structured like that, which sounds like self-calling, but normally no
function calls in those functions should ever throw, so that's fine. But
in our case, `abort` ends up throwing, which is a problem.

The function body of `__terminate` eventually calls JS `abort`, and ends
up here:
https://github.com/emscripten-core/emscripten/blob/970998b2670a9bcf39d31e2b01db571089955add/src/preamble.js#L605-L623

This ends up throwing a JS exception. This is basically just a foreign
exception from the wasm perspective, and is caught by `catch_all`, and
calls `std::terminate` again. And the whole process continues until the
call stack is exhausted.

What emscripten-core#9730 tried to do was throwing a trap, because Wasm
`catch`/`catch_all` don't catch traps. Traps become `RuntimeError`s
after they hit a JS frame. To be consistent, we decided
`catch`/`catch_all` shouldn't catch them after they become
`RuntimeError`s. That's the reason emscripten-core#9730 changed the code to throw not
just any random thing but `RuntimeError`. But somehow we decided that we
make that trap distinction not based on `RuntimeError` class but some
hidden field
(WebAssembly/exception-handling#89 (comment)).

This PR removes `noexcept` from `std::terminate` and
`std::__terminate`'s signatures so that the cleanup that contains
`catch_all` is not generated for those two functions. So now the JS
exception thrown by `abort()` will unwind the stack, which is different
from native, but that can be considered OK because I don't think users
expect `abort` to preserve the stack intact?

Fixes emscripten-core#16407.
aheejin added a commit to emscripten-core/emscripten that referenced this issue May 11, 2022
We used to implement `abort()` with throwing a JS exception. #9730
changed it to `RuntimeError`, because it is the class a trap becomes
once it hits a JS frame.

But this turned out to be not working, because Wasm EH currently does
not assume all `RuntimeError`s are from traps; it decides whether a
`RuntimeError` is from a trap or not based on a hidden field within the
object. Relevant past discussion:
WebAssembly/exception-handling#89 (comment)

So at the moment we don't have a way of throwing a trap from JS, which
is inconvenient. I think we eventually want to make JS API for this, but
for the moment, this PR resorts to a wasm function that traps. This at
least makes calling `std::terminate` work. So far calling it exhausted
the call stack and aborted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants