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

Broken linked modules in no-modules target if shim is not parsed in document #3277

Closed
daxpedda opened this issue Feb 2, 2023 · 5 comments · Fixed by #3279
Closed

Broken linked modules in no-modules target if shim is not parsed in document #3277

daxpedda opened this issue Feb 2, 2023 · 5 comments · Fixed by #3279
Labels

Comments

@daxpedda
Copy link
Collaborator

daxpedda commented Feb 2, 2023

Describe the Bug

If the no-modules target is used and the shim is not parsed in a document, like for example in any kind of worker, any linked module will break.

This problem partially stems from #3069, which now uses new URL('path/to/linked/module', script_src).

When using the no-modules target, script_src can not possibly be correct when not used in a document, which was an oversight from this PR: #3169.

The problem is that if you are in a worker and using importScripts(), location.href can never return the path to the imported script, the shim, which is what we want, instead location.href will return the path to the script that starts the worker, which is not useful in this context and in fact will break any path created for linked modules.

Steps to Reproduce

I can't really get the examples working because wasm-pack doesn't work for me, but the simplest example is using the Atomics.waitAsync polyfill in Firefox inside a worker, which currently uses linked modules and will try to create a nested worker, which will fail because new URL() will report an invalid base, for example if the base is a blob.

Solution

I think the solution is two-fold here:

  • Remove the fallback to location.href if document is undefined. It is simply impossible to get the path to the shim with the no-modules target, at least as far as I have research.
  • Disable --split-linked-modules by default when using the no-modules target.
    --split-linked-modules is already disabled by default, but not when using wasm-bindgen-cli-support, e.g. wasm-bindgen-test-runner.
@daxpedda daxpedda added the bug label Feb 2, 2023
@daxpedda
Copy link
Collaborator Author

daxpedda commented Feb 2, 2023

I tracked the location.href issue down to #1938, which is also me from 3 years ago.

@daxpedda
Copy link
Collaborator Author

daxpedda commented Feb 2, 2023

I was quiet confused how --split-linked-modules was somehow enabled by default.

Eventually I discovered what I think was an oversight in #3069, --split-linked-modules is not enabled by default, but when using wasm-bindgen-cli-support directly it is, so tools like wasm-bindgen-test-runner were enabling it by default.

@daxpedda
Copy link
Collaborator Author

daxpedda commented Feb 2, 2023

I made a PR to address this issue: #3279.

@Liamolucko
Copy link
Collaborator

Oh, that's pretty bad. I'm particularly concerned about the Atomics.waitAsync polyfill - in summary, attempting to use wasm-bindgen-futures in a worker in Firefox will fail if you enable --split-linked-modules when using --target no-modules.

I think that may be worth yanking the new release of wasm-bindgen-futures over, particularly since it's possible for someone to completely miss the bug if they don't test in Firefox, and I really don't want wasm-bindgen to be the culprit for sites suddenly becoming incompatible with Firefox. Cc @alexcrichton.

To be clear, I'm not suggesting we yank the whole thing, just wasm-bindgen-futures. I don't think the bug itself is yank-worthy, it's just that the way it manifests in wasm-bindgen-futures could so easily slip by unnoticed.

  • Remove the fallback to location.href if document is undefined. It is simply impossible to get the path to the shim with the no-modules target, at least as far as I have research.

👍

  • Disable --split-linked-modules by default when using the no-modules target.

It's already disabled by default.

@daxpedda
Copy link
Collaborator Author

daxpedda commented Feb 2, 2023

I believe as long as it's documented that --split-linked-modules and the no-modules target don't play well together it might not be a big deal.
Some documentation was added in #3279.

  • Disable --split-linked-modules by default when using the no-modules target.

It's already disabled by default.

Yes, but not in wasm-bindgen-test-runner, which did break a lot of things.

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