If async preparing wasm fails, abort() to reject the module promise

[stephanreiter]

For example, if a global constructor performs an invalid memory access,
an exception will be thrown that bubbles up to receiveInstantiatedSource.
That causes a rejection of the promise returned from instantiateAsync,
which we should handle.

Fixes #12396

[kripken]

Thanks for the PR @stephanreiter !

The first CI error appears to be that this succeeds when it should fail:

    err = self.run_js('a.out.js', assert_returncode=NON_ZERO)

It does log the error message to the console, but looks like now the return code from the process is 0, which is not good.

[kripken]

If this is just an issue on node, I wonder if abort() could set the return code there, and/or immediately exit with an error. This seems related to the issue with NODEJS_CATCH_REJECTION that we don't really have a good answer for yet, but would be great to fix.

[kripken]

One thought I've had, but am not sure about: in the MODULARIZE case there is a Promise the user can do a .catch on. So we don't need to do anything there. But in the non-MODULARIZE case, we do want to turn a Promise rejection into a thrown exception, as there is nothing better to do, and we need to report the error up.

[stephanreiter]

Maybe like this?

#ifdef MODULARIZE
  instantiateAsync().catch(readyPromiseReject);
#else
  instantiateAsync();
#endif

[stephanreiter]

I just tested this for my use case and it does allow me to catch the error. 👍 Let me update the PR to see what CI tests think.

[kripken]

@stephanreiter Yes, and looks like it passes tests!

As I said I think that makes sense, but I'm not sure. The downside is adding a difference in MODULARIZE mode compared to normal mode. But, it does seem like the right thing to me..? If we do it, we'd need to add documentation and testing - in particular I doubt we have a test for MODULARIZE + failing to load the wasm, which we'd need.

But first, it might be good to get more eyes on this, I'll cc some people that may have thoughts or ideas: @curiousdannii @lourd @RReverser (you can start reading from #12408 (comment) and/or just read the current code in the PR)

[stephanreiter]

For MODULARIZE I also believe this to be right. An instantiation failure will reject the module ready promise, and my app code will be informed that something went wrong (and also gets the details via the error object; this is actually better than going through abort() where the error is lost).
Not sure about MODULARIZE=0. I've never used that.
Great to get more eyes on the PR. :-) I'll be happy to invest in tests on Monday.

[RReverser]

@kripken I think this makes sense too. To be fair, personally I'd prefer never abort-ing but instead rejecting / throwing JS issues that can be handled more idiomatically, but that would be a breaking change; meanwhile, adopting this at least for instantiation seems to make sense.

[stephanreiter]

Thanks for taking a look!

I added a test and rebased my PR.
For the test, I needed to make unhandled rejections in browser tests cause test failures. I'm quite sure this will affect other tests and it will be interesting to see if this uncovers other problems.

[stephanreiter]

Need to fix how to report unhandled rejections from browser tests.

[stephanreiter]

We now have two tests of handling of module instantiation failure due to a) a global constructor performing an invalid memory access, and b) download failure. We check that the module promise is rejected and that no promise rejection remains unhandled.

[kripken]

Thanks @stephanreiter ! Looks good to me with some test changes, + also please mention this in the Changelog.

[lourd]

Good changes, thanks @stephanreiter! LGTM.

This looks similar to my PR #11625 that got reviewed and then fell along the wayside. @kripken, let me know if you'd like to see that one finished. I think the async control flow between instantiation, fallback instantiation, and receiving the instantiated module is still confusing.

[kripken]

@kripken, let me know if you'd like to see that one finished.

I think refactoring that code would be great, sure!

[kripken]

Great!