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

Instance access after start function fails #1118

Closed
jfbastien opened this issue Jul 27, 2017 · 10 comments
Closed

Instance access after start function fails #1118

jfbastien opened this issue Jul 27, 2017 · 10 comments

Comments

@jfbastien
Copy link
Member

This isn't explicit in the JS API: if the start function fails then the instance is still accessible through the table if its element section set at least one non-imported function. That seems fine to me, but I'd rather make sure everyone agrees, and we don't want to e.g. null out elements that were set in case of failure.

@RyanLamansky
Copy link

Interesting scenario. My assumption was that a failure of the start function would make the instance unusable.

@binji
Copy link
Member

binji commented Jul 28, 2017

This seems fine to me too. There's nothing special about the start function except that it is called immediately after instantiation, right? Just because it traps doesn't necessarily mean there is anything wrong with the instance, the same as when any other function traps.

@jfbastien
Copy link
Member Author

jfbastien commented Jul 28, 2017

It's not special, except the instance is otherwise inaccessible when the start function traps (because you can't have assigned from WebAssembly.instantiate's resolved promise but you can still access the WebAssembly.Table if it was an import).

I wonder if this has any implication with dynamic JavaScript module import and <script type=module>, when integrated with WebAssembly. @domenic do you have thoughts on this?

@rossberg
Copy link
Member

rossberg commented Jul 28, 2017 via email

@jfbastien
Copy link
Member Author

Seems fine to me as well. Having special behaviour with implicit table
modifications after a failure would seem much weirder to me.

Agreed, I was surprised when I realized this, and then thought it was fine. I'd rather we explicitly agree it's fine though! Especially w.r.t. modules (I'm still not 100% sure, hence the @domenic ping).

@domenic
Copy link
Member

domenic commented Jul 29, 2017

I don't think I have the context to understand what's going on here, but I can give some potentially-relevant information about modules/HTML integration. Apologies if it ends up not being relevant and this is just off-topic.

What we've done there is that there is a module map (per realm), URL -> "module script". A module script is a spec-level structure. It can progress between several states:

  • fetching
  • uninstantiated
  • instantiated
  • evaluated
  • errored

The normal flow is fetching -> uninstantiated -> instantiated -> evaluated, but any of these can fail, moving you to the errored state.

If a module script is in the errored state, it stays in the module map like that forever, and that failure is remembered. Any future modules that depend on it statically will automatically become errored, and its exports are inaccessible to the rest of the module graph. (But if the failure occurs during evaluation, any side effects still occurred, of course.)

It seems to me like there might be something analogous going on here, where the start function is kind of like the evaluation proccess. In which case making the instance unusable sounds like it would be analogous. So maybe that is the way to go?

In terms of eventual JS module integration, the important thing is that we're able to create some mapping from wasm modules (module instances, I guess?) to the "module script" concept. In this case it sounds like the important part is knowing when the instance should be exposed as an errored module script. It's OK if we decide that start failure does not make the wasm module script errored, as long as we can expose a useful module namespace object for the non-errored result.

I think we'll be able to create such a mapping regardless, but I wanted to give us much background as possible in case it helps those who understand this system better make a decision.

@jfbastien
Copy link
Member Author

It seems to me like there might be something analogous going on here, where the start function is kind of like the evaluation proccess. In which case making the instance unusable sounds like it would be analogous. So maybe that is the way to go?

Yes, I think what you describe is what I had in mind as the process for ES6 modules. In our implementation (where we reuse our ES6 module machinery for WebAssembly) we initialize memory / table and call the start function while linking:

https://github.com/WebKit/webkit/blob/f230fb6c2d3263473c4d350262a7fdd3f20225c3/Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp#L89

We don't invalidate the table when it fails, which is what this bug is about. Tables don't really have any analogous thing to ES6 modules though: WebAssembly has imports and exports which map very well to ES6 modules. Tables are just another way to access a WebAssembly instance's exports (which can themselves be re-exported imports).

In terms of eventual JS module integration, the important thing is that we're able to create some mapping from wasm modules (module instances, I guess?) to the "module script" concept. In this case it sounds like the important part is knowing when the instance should be exposed as an errored module script. It's OK if we decide that start failure does not make the wasm module script errored, as long as we can expose a useful module namespace object for the non-errored result.

Yeah I think what we have now works because tables are just this Other Thing we have.

I think we'll be able to create such a mapping regardless, but I wanted to give us much background as possible in case it helps those who understand this system better make a decision.

Agreed 😄

@rossberg
Copy link
Member

Right, the only JS-side objects that are still accessible at all after a trap from the start function are newly created JS function objects put into an (external) table during instantiation. The actual WebAssembly.Instance object representing the module will not be reachable or otherwise observable (so depending on implementation details, can become garbage immediately or isn't even allocated yet).

@jfbastien
Copy link
Member Author

Ah, @bterlson just touched the issue I had in the back of my mind when I filed this: tc39/ecma262#916

@sunfishcode
Copy link
Member

The consensus here seems to be that the current behavior is ok. Please re-open or file a new issue if there are any further concerns!

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

No branches or pull requests

6 participants