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

WASM bundles fail when loaded via file:// URLs #7918

Closed
bspeice opened this issue Aug 19, 2018 · 22 comments
Closed

WASM bundles fail when loaded via file:// URLs #7918

bspeice opened this issue Aug 19, 2018 · 22 comments

Comments

@bspeice
Copy link

bspeice commented Aug 19, 2018

Bug report

What is the current behavior?
Electron apps fail to load WASM code because WASM bundles served via file URL's aren't allowed to be loaded via instantiateStreaming (specifically because MIME application/wasm must be set.

If the current behavior is a bug, please provide the steps to reproduce.
No configuration is specifically necessary, but I've included a reproducing project at this git repo. To run:

yarn install
./build.sh
yarn start

Running will require a recent Rust nightly build, but I'm happy to add the bundle files if necessary to skip that step.

What is the expected behavior?
Webpack would detect that file URLs are not eligible for inclusion via instantiateStreaming and prioritize filling the arrayBuffer and instantiateing that instead.

Please note that I can specifically reproduce the correct behavior by snipping out the else if(...) WebAssembly.instantiateStreaming branch. I'm not sure what the right way of fixing this is, but I'm happy to work on a PR if someone can point me in the right direction.

Other relevant information:
webpack version: 4.16.5
Node.js version: 8.10
Operating System: Linux
Additional tools: Rust nightly


Alright, some updates after I spent more time looking at this: the actual URL request goes to ".wasm", so adding checks for "./" or "file://" don't fix this specific issue (though may not be a bad idea in general). Is this something that should potentially have a config value added instead, where we can disable streaming load?

@xtuc
Copy link
Member

xtuc commented Aug 20, 2018

I think that's something that browser should change. arrayBuffer + instantiate too has many disadvantages.

I'm not very familiar with Electron, could we load the wasm module using node and fs?

@xtuc
Copy link
Member

xtuc commented Aug 20, 2018

Btw here's a previous discussion: WebAssembly/design#1200

@bspeice
Copy link
Author

bspeice commented Aug 21, 2018

I'm admittedly totally unfamiliar with fs in node - my expectation is that you could load just fine, but you'd still have issues with the instantiateStreaming support. Some people I've talked with recommended effectively spinning up a static file server in-process to load the resource bundles (with appropriate MIME types) but this strikes me as a bit strange - I don't particularly want to host a server inside the electron app.

@schmidtk
Copy link

Emscripten's boilerplate to load WebAssembly modules handles this situation with a graceful fallback to ArrayBuffer instantiation. A similar approach could be applied here in webpack.

I tested this solution in the webpack output and it solved my problem in Electron. If anyone with more (any) webpack contribution experience would like to fix this via PR I would be forever grateful, otherwise I may be able to make time to take a crack at it.

@sokra
Copy link
Member

sokra commented Sep 2, 2018

Yep send a PR

@bspeice
Copy link
Author

bspeice commented Sep 3, 2018

I've started a PR, but it could use some assistance (I'm not sure what exactly is or would be failing in the integration tests). If anyone's able to help that would be great, otherwise feel free to use it as a starting point.

@xtuc
Copy link
Member

xtuc commented Sep 3, 2018

Thanks @bspeice for the PR.

Maybe we can override window.fetch to throw an error only the first call, so the fallback will still work.

@webpack-bot
Copy link
Contributor

This issue had no activity for at least half a year.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@webpack-bot
Copy link
Contributor

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

@bspeice
Copy link
Author

bspeice commented Mar 20, 2019

Apologies for not responding - I'm not a JS developer so would have no idea how to safely apply an override like that. If everyone's committed to that path, I'd need some help.

@webpack-bot
Copy link
Contributor

This issue had no activity for at least half a year.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@alexander-akait
Copy link
Member

bump

@webpack-bot
Copy link
Contributor

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@alexander-akait
Copy link
Member

bump

@webpack-bot
Copy link
Contributor

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

@alexander-akait
Copy link
Member

/cc @vankop fixed in webpack@5?

@vankop
Copy link
Member

vankop commented Jul 24, 2020

file://path/some.wasm should work.

Feel free to report a bug if does not work.

@bspeice
Copy link
Author

bspeice commented Jul 24, 2020

Can confirm, everything does seem to be fixed in webpack 5.

@alexander-akait
Copy link
Member

@bspeice thanks!

@bspeice
Copy link
Author

bspeice commented Jul 24, 2020

One quick clarification after I did some further testing: while Webpack 5 does contain fixes needed to load WASM blobs asynchronously, it doesn't change the underlying issue (Content-Type unset for file:// URLs). I'm having issues trying to find when this was changed in Chrome (and thus Electron as well), but when using window.fetch('file://*.wasm'), the Content-Type now comes back as application/wasm.

@alexander-akait
Copy link
Member

@bspeice Can you open a new issue with reproducible steps?

@bspeice
Copy link
Author

bspeice commented Jul 25, 2020

@evilebottnawi: no need for a new issue. My intention was to clarify that the underlying issue was Chrome-related, and thus not something webpack could fix. The combination of browser and webpack changes that have come since this issue was opened make it so that everything works correctly. No further work needed, thanks all for the assistance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants