Use `fetch` rather than `XMLHttpRequest` for downloading Wasm files #22015

seanmorris · 2024-05-30T01:15:31Z

This PR switches requests for Wasm files to be loaded with fetch() rather than XMLHttpRequest().

This fixes running in service workers where XMLHttpRequest is not available.

Partially fixes #22003, only applies to Wasm files.

sbc100 · 2024-05-30T14:28:41Z

I believe readAsync is also used to load the wasm module itself.. so this effects all users, not just those who have shared libraries. Maybe update the title and description to refer to wasm file loading?

sbc100

lgtm with a few comments

src/web_or_worker_shell_read.js

sbc100 · 2024-05-30T14:33:04Z

src/web_or_worker_shell_read.js

-      if (xhr.status == 200 || (xhr.status == 0 && xhr.response)) { // file URLs can return 0
-        onload(xhr.response);
-        return;
+    fetch(url)


Can you also update the above two uses of XMLHttpRequest ?

Looks like the two earlier calls are synchronous, it might take some more re-engineering to get those converted.

test/test_browser.py

sbc100 · 2024-05-30T14:40:49Z

test/test_browser.py

@@ -5549,6 +5554,69 @@ def test_webpack(self):
    shutil.copyfile('webpack/src/hello.wasm', 'webpack/dist/hello.wasm')
    self.run_browser('webpack/dist/index.html', '/report_result?exit:0')

+  def test_fetch_polyfill_shared_lib(self):


I wonder if we even need to involve shared libraries here since the polyfill is needed even to load the base wasm file (I think).

Lgtm either way though since you've already gone to the trouble to writing the test.

I think its good to have this test either way, that stuff could be changed in the future as the code becomes more unified. We should make sure the code works in all cases.

sbc100

I updated the PR title and description.. I hope thats ok

sbc100 · 2024-05-30T14:49:43Z

Actually I was wrong.. it looks like the core wasm file already has a fetch-by-default setup (we can remove this as a followup to this change I hope):

emscripten/src/preamble.js

Line 660 in cb99414

    
           return fetch(binaryFile, {{{ makeModuleReceiveExpr('fetchSettings', "{ credentials: 'same-origin' }") }}}).then((response) => {

test/common.py

test/test_browser.py

sbc100

Looks good!

Just need to figure out what is going on with that common.py change. Otherwise this looks ready to land.

Thanks for you patience on this one.

test/test_browser.py

Inspired by emscripten-core#22015

…owserCore.compile_btest

Inspired by emscripten-core#22015

In emscripten-core#22015 we switched `readAsync` to be based on fetch rather than XHR. However that broke the assumption in `getBinaryenPromise` that was assuming `readAsync` was XHR-based. This change reinstates the XHR fallback in the case of webviews where loading from `file://` URLs is not only possible but (apparently) common. Sadly we don't have any webview tests so I have no way to actually test this within our current frameworks.

sbc100 · 2024-05-30T23:46:43Z

Oops, I think we overlooked the webview + file-url case. See #22026

In emscripten-core#22015 we switched `readAsync` to be based on fetch rather than XHR. However that broke the assumption in `getBinaryenPromise` that was assuming `readAsync` was XHR-based. This change reinstates the XHR fallback in the case of webviews where loading from `file://` URLs is not only possible but (apparently) common. Sadly we don't have any webview tests so I have no way to actually test this within our current frameworks.

Inspired by emscripten-core#22015

…22018) Inspired by #22015

In emscripten-core#22015 we switched `readAsync` to be based on fetch rather than XHR. However that broke the assumption in `getBinaryenPromise` that was assuming `readAsync` was XHR-based. This change reinstates the XHR fallback in the case of webviews where loading from `file://` URLs is not only possible but (apparently) common. Sadly we don't have any webview tests so I have no way to actually test this within our current frameworks.

In #22015 we switched `readAsync` to be based on fetch rather than XHR. However that broke the assumption in `getBinaryenPromise` that was assuming `readAsync` was XHR-based. This change reinstates the XHR fallback in the case of webviews where loading from `file://` URLs is not only possible but (apparently) common. Sadly we don't have any webview tests so I have no way to actually test this within our current frameworks.

…mscripten-core#22015) This PR switches requests for Wasm files to be loaded with `fetch()` rather than `XMLHttpRequest()`. This fixes running in service workers where `XMLHttpRequest` is not available. Partially fixes emscripten-core#22003, only applies to Wasm files.

…mscripten-core#22018) Inspired by emscripten-core#22015

In emscripten-core#22015 we switched `readAsync` to be based on fetch rather than XHR. However that broke the assumption in `getBinaryenPromise` that was assuming `readAsync` was XHR-based. This change reinstates the XHR fallback in the case of webviews where loading from `file://` URLs is not only possible but (apparently) common. Sadly we don't have any webview tests so I have no way to actually test this within our current frameworks.

seanmorris added 2 commits May 29, 2024 21:13

Separating readAsync fetch from loadPackage fetch.

710e300

Tweaks.

bcdaaa8

seanmorris changed the title ~~Separating readAsync fetch from loadPackage fetch.~~ Use fetch() for loading shared libraries May 30, 2024

seanmorris mentioned this pull request May 30, 2024

Use fetch() for loading packages & shared libraries #22002

Closed

seanmorris added 3 commits May 29, 2024 22:46

Fixing exception detection in test_missing_data_throws_error

d882d42

Fixing exception detection in test_missing_data_throws_error

24fa0cd

AUTHORS update

8b9355b

seanmorris changed the title ~~Use fetch() for loading shared libraries~~ Use fetch() for loading shared libraries May 30, 2024

sbc100 changed the title ~~Use fetch() for loading shared libraries~~ Use fetch rather than XMLHttpRequest for download Wasm files May 30, 2024

sbc100 reviewed May 30, 2024

View reviewed changes

sbc100 changed the title ~~Use fetch rather than XMLHttpRequest for download Wasm files~~ Use fetch rather than XMLHttpRequest for downloading Wasm files May 30, 2024

seanmorris added 2 commits May 30, 2024 11:39

PR comment tweaks.

d4a2fa0

PR comment tweaks.

a83c297

seanmorris requested a review from sbc100 May 30, 2024 15:44

seanmorris added 2 commits May 30, 2024 11:57

PR comment tweaks.

61132ca

Spacing.

6165987

sbc100 reviewed May 30, 2024

View reviewed changes

test/common.py Outdated Show resolved Hide resolved

sbc100 reviewed May 30, 2024

View reviewed changes

test/test_browser.py Show resolved Hide resolved

sbc100 reviewed May 30, 2024

View reviewed changes

test/test_browser.py Outdated Show resolved Hide resolved

sbc100 reviewed May 30, 2024

View reviewed changes

test/test_browser.py Outdated Show resolved Hide resolved

sbc100 reviewed May 30, 2024

View reviewed changes

test/test_browser.py Outdated Show resolved Hide resolved

sbc100 reviewed May 30, 2024

View reviewed changes

test/test_browser.py Outdated Show resolved Hide resolved

seanmorris added 3 commits May 30, 2024 12:16

More PR comment tweaks.

fbe17c1

More PR comment tweaks.

68023c8

Whitespace.

459df06

sbc100 reviewed May 30, 2024

View reviewed changes

test/test_browser.py Outdated Show resolved Hide resolved

test/test_browser.py Outdated Show resolved Hide resolved

seanmorris added 3 commits May 30, 2024 12:28

Comments.

72d6a2b

Removing debug.

ea85f33

tweak

b4a9154

sbc100 reviewed May 30, 2024

View reviewed changes

test/test_browser.py Outdated Show resolved Hide resolved

sbc100 mentioned this pull request May 30, 2024

[test] Use fetch over XMLHttpRequest for browser test reporting. NFC #22018

Merged

tweak

3974389

sbc100 added a commit to sbc100/emscripten that referenced this pull request May 30, 2024

[test] Use fetch over XMLHttpRequest for browser test reporting. NFC

81ce805

Inspired by emscripten-core#22015

Merge branch 'main' into sm-worker-shared-fetch

16559db

sbc100 added a commit to sbc100/emscripten that referenced this pull request May 30, 2024

[test] Use fetch over XMLHttpRequest for browser test reporting. NFC

2c0cc36

Inspired by emscripten-core#22015

seanmorris added 3 commits May 30, 2024 16:54

Passing -fPIC from args (if present) to compile report_result.c in Br…

1b07be5

…owserCore.compile_btest

Passing -fPIC from args (if present) to compile report_result.c in Br…

feab2d6

…owserCore.compile_btest

Merge branch 'main' into sm-worker-shared-fetch

bbff4bf

sbc100 approved these changes May 30, 2024

View reviewed changes

sbc100 enabled auto-merge (squash) May 30, 2024 22:20

sbc100 added a commit to sbc100/emscripten that referenced this pull request May 30, 2024

[test] Use fetch over XMLHttpRequest for browser test reporting. NFC

a11c95f

Inspired by emscripten-core#22015

sbc100 merged commit 7e79633 into emscripten-core:main May 30, 2024
29 checks passed

sbc100 mentioned this pull request May 30, 2024

Re-enable XHR as fallback for fetch on webview #22026

Merged

sbc100 added a commit to sbc100/emscripten that referenced this pull request May 31, 2024

[test] Use fetch over XMLHttpRequest for browser test reporting. NFC

582cc24

Inspired by emscripten-core#22015

sbc100 added a commit to sbc100/emscripten that referenced this pull request May 31, 2024

[test] Use fetch over XMLHttpRequest for browser test reporting. NFC

c21c198

Inspired by emscripten-core#22015

sbc100 added a commit that referenced this pull request May 31, 2024

[test] Use fetch over XMLHttpRequest for browser test reporting. NFC (#…

28c10a1

…22018) Inspired by #22015

msorvig pushed a commit to msorvig/emscripten that referenced this pull request Jun 4, 2024

[test] Use fetch over XMLHttpRequest for browser test reporting. NFC (e…

1ecdf5a

…mscripten-core#22018) Inspired by emscripten-core#22015

Use fetch rather than XMLHttpRequest for downloading Wasm files #22015

Use fetch rather than XMLHttpRequest for downloading Wasm files #22015

Uh oh!

Conversation

seanmorris commented May 30, 2024 • edited by sbc100 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented May 30, 2024

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbc100 May 30, 2024

Choose a reason for hiding this comment

Uh oh!

seanmorris May 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbc100 May 30, 2024

Choose a reason for hiding this comment

Uh oh!

sbc100 May 30, 2024

Choose a reason for hiding this comment

Uh oh!

seanmorris May 30, 2024

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 commented May 30, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbc100 commented May 30, 2024

Uh oh!

Uh oh!

Use `fetch` rather than `XMLHttpRequest` for downloading Wasm files #22015

Use `fetch` rather than `XMLHttpRequest` for downloading Wasm files #22015

seanmorris commented May 30, 2024 •

edited by sbc100

Loading