-
Notifications
You must be signed in to change notification settings - Fork 33
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
Expose the JavaScript Exception tag and allow importing it #269
Conversation
This is a basic start of what we'll need to add to the embedder spec. I'm not sure if we also need a way to allow exceptions to bubble from the embedder into wasm ; I don't see any reference to what happens when the instance wants to call an import, so maybe not. Also the references aren't quite right, but I may need help to fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So basically this wraps JS exceptions when entering wasm and unwraps them when exiting wasm, right? If we do this, I guess we don't need a special handling for catch
..?
document/js-api/index.bs
Outdated
@@ -1383,10 +1388,14 @@ The <dfn attribute for="Exception">stack</dfn> getter steps are: | |||
<h4 id="js-exceptions">JavaScript exceptions</h4> | |||
|
|||
The <dfn>JavaScript exception tag</dfn> is a [=tag address=] reserved by this | |||
specification to distinguish exceptions originating from JavaScript. | |||
specification to distinguish exceptions originating from JavaScript. It is exposed | |||
to JavaScript code via {{WebAssembly}}.{{JSTag}} a global pre-initailized (TODO: what's ths spec-y way to say this?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to JavaScript code via {{WebAssembly}}.{{JSTag}} a global pre-initailized (TODO: what's ths spec-y way to say this?) | |
to JavaScript code via {{WebAssembly}}.{{JSTag}} a global pre-initialized (TODO: what's ths spec-y way to say this?) |
Not sure about more spec-y way to say this though.
I don't have any particular concerns about the implementation here. We could pack/unpack the exception in a |
Yes, this basically wraps/unwraps as you said. Because of that I don't think we need special handling for catch, because it would be the same as catching an exception with any other imported tag with an externref type. |
I think this can be treated as internal implementation / optimization details, and this should be still compliant with what @dschuff wrote. Right? |
On second thought I think my suggestion doesn't work. If JS explicitly creates and throws a |
document/js-api/index.bs
Outdated
|
||
For any [=associated store=] |store|, the result of | ||
[=tag_parameters=](|store|, [=JavaScript exception tag=]) must be « ». | ||
[=tag_parameters=](|store|, [=JavaScript exception tag=]) must be « [=externref=] ». | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what we should be doing (as the "spec-y" thing, and in order to preerve abstraction boundaries) is to invoke tag_alloc
from the core spec's embedding interface to create that tag, with the respective tag type. That could happen lazily, i.e., the JS spec does the allocation the first time the tag address is needed and then memoizes it.
Btw, I think the embedding spec should also provide a tag_type
function, analogous to the other *_type
functions.
Co-authored-by: Andreas Rossberg <[email protected]>
This is a special tag which matches any JS exception not wrapped in a WebAssembly.Exception object. See: WebAssembly/exception-handling#269 This only adds the JSTag object for now. The special behavior of this tag with respect to JS exceptions will be implemented in a separate CL. [email protected] Bug: v8:8091 Change-Id: Ie663791deaf60ac178d7f366ddd007ddfe0e383e Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4508921 Commit-Queue: Thibaud Michaud <[email protected]> Reviewed-by: Clemens Backes <[email protected]> Cr-Commit-Position: refs/heads/main@{#87479}
@dschuff @thibaudmichaud As discussed earlier, we should rebase this to the new EH API because this is a requirement for our partners. |
Rebasing this on the new proposal has some implications so I would like to clarify a few points. In the old proposal, the implicit wrapping/unwrapping of the JS exception into a WA.Exception at the boundary was only observable through the With the new proposal, it sounds like the WA.Exception could also be observed using And is it correct that without this PR, |
Logically, the exnref of a JS exception is a wrapped value. Internally, it could be the unwrapped JS value itself, depending on implementation choices. That is not observable in Wasm. In particular, there is no way to compare the exnref against the externref that logically represents the actual JS value. From the Wasm perspective, the exnref is opaque. Moreover, this exnref cannot be passed pack to JS, so JS cannot observe its relation to the JS value either. (That's why we need the type error at the boundary.) Taken together, this should imply that the previous special-casing still works. An engine is free to represent a JS-exception exnref in whatever way it sees fit, whether wrapped or unwrapped. The only requirement is that throw_ref handles it accordingly. |
I've rebased this against #301 to make it easier to review. One aspect becomes trivial because #301 already handles allocating the JS tag. So this just becomes about the right way to put it on the WebAssembly namespace. In that sense it becomes just like the wasm-specific exception classes, so I refactored that part. It looks like @Ms2ger authored that, WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't have time to follow all the changes in detail, but some comments below.
Agreed; mostly I was wondering how the "create a namespace" bit that we have for the exceptions got to be the way it was in the first place. And I also don't see why the attribute description shouldn't work; byt what exactly do we mean by "work" here? If we write the attribute that way and write a getter, it seems like this is sufficient to describe the desired behavior but I don't have much experience with this kind of spec. |
The reason for the custom algorithm is that the exception classes are modeled after the ones that exist in JavaScript itself, and those couldn't be specified in WebIDL. The attribute should be fine, though; I was just wondering if you discovered an issue that lead you to put in the custom algorithm instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All that remains of this PR is JS/WebIDL stuff, so I'll defer to @Ms2ger. :)
Got it. No, I didn't discover any issues; the attribute is now back, with a getter. |
Continuing the discussion from #301 (comment)
It would not be unwrapped on entry (with the optimization I was discussing), so it would come out as the WebAssembly.Exception + payload. We would have to unwrap it either on entry or on exit to ensure that only the payload comes out. |
@thibaudmichaud @sjrd, continuing the discussion from #301 (comment)
Ok, I don't mind adding this restriction. We can always relax it later if we want. |
SGTM. For the same reason (and to be consistent) the restriction should also apply to |
I could imagine that being actually useful though? If you have a JS object stored in an externref in wasm, I can imagine that you might want to throw it, and have the result come out as an unwrapped JS exception. Especially if it's already an Error object or something. |
I disagree. On the Wasm side, this is the only way to throw an exception that will be caught as-is on the JS side. This is important for interoperability with JS, if the JS thing you want to talk to expects specific shapes of exceptions. The way I see it, by disallowing
This is good in terms of implementation because there is a unique representation of exceptions with (by spec) Currently in V8, the Note that if we do disallow it on both sides, it is still kind-of possible for Wasm to throw a bare JS exception, but it requires cooperation from JS. You can write a JS function Edit: also I'm not only saying this in the abstract. I would definitely use |
Ok, special-casing
That will work, but that does not require any special case because |
Any more comments on this? The current version just uses the WebIDL attribute with a getter, and also contains the restriction that creating a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, looks good to me.
I believe this is what we discussed in #202
The JS tag has type externref and is exposed on the WebAssembly object, allowing it to be imported.
When such exceptions are caught in wasm, the thrown JS object is passed to the catch block as an externref.
When such exceptions propagate from wasm to JS, the payload value is thrown directly, rather than being
wrapped in a new Exception.