Make 'instantiateWasm' callback resolve the result return from 'recei… #23781

gogogoghost · 2025-02-27T10:14:57Z

In js output content.

When ASYNCIFY enabled, wasmExports modified by Asyncify.instrumentWasmExports. source

But when Module['instantiateWasm'] used. The createWasm function will always resolve the original wasmExports. source.

It will case crash when async C++ function rewind.

So I make instantiateWasm callback resolve the result return from receiveInstance.

…veInstance'.

sbc100

Tanks for the fix!

Do you think we can update one of the existing tests to cover this? Maybe test_instantiate_wasm in test_other.py

…enabled

gogogoghost · 2025-02-28T04:29:38Z

I have just updated the test unit. And this issue only affects when EXPORT_ES6 enabled.

When EXPORT_ES6 disabled. createWasm called after wasmExports defined.

var wasmExports;
createWasm();

wasmExports can be updated in createWasm.

When EXPORT_ES6 enabled. wasmExports assigned by the return value of createWasm.

var wasmExports = await createWasm();

sbc100 · 2025-02-28T16:06:14Z

test/test_manual_wasm_instantiate.c

-  assert(result);
+  printf("main: before getResult\n");
+  int result = getResult();
+  printf("testWasmInstantiationSucceeded: %d\n",result);
  return 0;


Are the changes in this file necessary? It looks like you could revert this file probably.

sbc100 · 2025-02-28T16:10:05Z

test/test_other.py

@@ -15688,7 +15688,12 @@ def test_instantiate_wasm(self):
        });
        return {}; // Compiling asynchronously, no exports.
      }''')
-    self.do_runf('test_manual_wasm_instantiate.c', emcc_args=['--pre-js=pre.js'])
+    self.run_process([EMCC, test_file('test_manual_wasm_instantiate.c'), '--pre-js=pre.js','-sASYNCIFY','-sEXPORT_ES6=1','-o','manual_init_module.mjs'])


I don't think you need to add '-sASYNCIFY' here do you?

sbc100 · 2025-02-28T16:11:22Z

test/test_other.py

@@ -15688,7 +15688,12 @@ def test_instantiate_wasm(self):
        });
        return {}; // Compiling asynchronously, no exports.
      }''')
-    self.do_runf('test_manual_wasm_instantiate.c', emcc_args=['--pre-js=pre.js'])
+    self.run_process([EMCC, test_file('test_manual_wasm_instantiate.c'), '--pre-js=pre.js','-sASYNCIFY','-sEXPORT_ES6=1','-o','manual_init_module.mjs'])


This test is already decorated with also_with_modularize so it should already run both with and without -sMODULARIZE.

Do this bug depened on -sEXPORT_ES6 , no just -sMODULARIZE somehow?

Did I misunderstand it? I need to add a new test unit to cover this situation not edit existing one.

This bug just depened on -sASYNCIFY and -sEXPORT_ES6.

Ah I see.. the bug only occurs with -sEXPORT_ES6.

I don't see how it depends on -sASYNCIFY though, maybe that flag can be removed?

Sorry, I think I understand now. -sASYNCIFY is needed because in that mode we wrap the wasm exports.

However, I think -sMODULARIZE should be enough (no need for -sEXPORT_ES6). So I think if you run test_instantiate_wasm_modularize (the modularized version) with -sASYNCIFY that should be enough to repro the issue without creating manual_init.mjs.

Sorry for the confusion.

source here.

Module['instantiateWasm'](info, (mod, inst) => { // When -sASYNCIFY. This function will wrap the original exports and return. receiveInstance(mod, inst); // But here always resolve the original exports. resolve(mod.exports); });

And when EXPORT_ES6 enabled. wasmExports assigned by the return value of createWasm.

var wasmExports = await createWasm();

So the wasmExports will always be the original exports object. Not the wrapped one.
It will case crash when a C++ function rewind.
If -sASYNCIFY removed. It will always success. because:

var wasmExports; // return value not used. createWasm();

And source here updated the global wasmExports.

And when EXPORT_ES6 enabled. wasmExports assigned by the return value of createWasm.

But this is also true for just -sMODULARIZE. i.e. i think this bug will show up with either -sMODULARIZE or -sEXPORT_ES6.. so it should be enough to test with the existing test_instantiate_wasm_modularize

test/test_other.py

sbc100 · 2025-03-03T20:10:21Z

The test failures look like they are because test_browser.py run the same test and needs -sASYNCIFY too.

gogogoghost · 2025-03-19T03:50:12Z

The test failures look like they are because test_browser.py run the same test and needs -sASYNCIFY too.

When -sASYNCIFY added into test_manual_wasm_instantiate in test_browser.py.

Then main function in test_manual_wasm_instantiate.c have to call REPORT_RESULT(0); and change reporting=Reporting.JS_ONLY to reporting=Reporting.FULL to report the result.

But other.test_instantiate_wasm doesn't link report_result.c that will case compile failed.

Please give me some advice. Do I need to create a separate .c file for browser.test_manual_wasm_instantiate?

caiiiycuk · 2025-04-02T15:08:23Z

Hi, what is the status of this PR?

--
P. S. Last version where it works a expected is 4.0.2

caiiiycuk · 2025-04-02T15:13:37Z

Maybe adding a new test for this specific case wouldn’t be such a bad idea?

sbc100 · 2025-04-02T15:21:46Z

I think its almost ready.. just needs the tests to pass.

sbc100 · 2025-04-08T15:22:29Z

test/test_browser.py

    shutil.copy(test_file('test_manual_wasm_instantiate.html'), '.')
-    self.run_browser('test_manual_wasm_instantiate.html', '/report_result?exit:0')
+    self.run_browser('test_manual_wasm_instantiate.html', '/report_result?1')


Why is this change needed?

Make 'instantiateWasm' callback resolve the result return from 'recei…

feabf26

…veInstance'.

sbc100 reviewed Feb 27, 2025

View reviewed changes

Update 'test_instantiate_wasm' to cover when ASYNCIFY and EXPORT_ES6 …

532dff3

…enabled

Merge branch 'main' into main

9d91469

FelixNumworks mentioned this pull request Feb 28, 2025

Provide a way to reject Promise in instantiateWasm #23780

Open

sbc100 reviewed Feb 28, 2025

View reviewed changes

test/test_other.py Show resolved Hide resolved

gogogoghost and others added 2 commits March 1, 2025 15:26

Remove '-sEXPORT_ES6' from 'test_instantiate_wasm' and add a comment

4ddca57

Merge branch 'main' into main

9528492

sbc100 reviewed Mar 1, 2025

View reviewed changes

test/test_other.py Show resolved Hide resolved

gogogoghost and others added 4 commits March 2, 2025 19:52

Remove unnecessary parameters and calls

fe1b791

Merge branch 'main' of github.com:gogogoghost/emscripten

b4ad166

Remove unnecessary output check

a73404f

Merge branch 'main' into main

900d8d1

sbc100 reviewed Mar 3, 2025

View reviewed changes

test/test_other.py Show resolved Hide resolved

Merge branch 'emscripten-core:main' into main

6775dbf

caiiiycuk mentioned this pull request Apr 2, 2025

Fix regression with instantiateWasm + MODULARIZE + Asyncify #24032

Closed

gogogoghost and others added 3 commits April 8, 2025 10:12

Merge branch 'main' into main

06de16e

Fix test browser.test_manual_wasm_instantiate

c76d934

Remove useless parameter

698f5ac

sbc100 reviewed Apr 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make 'instantiateWasm' callback resolve the result return from 'recei… #23781

Make 'instantiateWasm' callback resolve the result return from 'recei… #23781

gogogoghost commented Feb 27, 2025 •

edited

Loading

sbc100 left a comment

gogogoghost commented Feb 28, 2025 •

edited

Loading

sbc100 Feb 28, 2025

sbc100 Feb 28, 2025

sbc100 Feb 28, 2025

gogogoghost Feb 28, 2025

sbc100 Feb 28, 2025

sbc100 Feb 28, 2025

gogogoghost Feb 28, 2025

sbc100 Feb 28, 2025

sbc100 commented Mar 3, 2025

gogogoghost commented Mar 19, 2025

caiiiycuk commented Apr 2, 2025 •

edited

Loading

caiiiycuk commented Apr 2, 2025

sbc100 commented Apr 2, 2025

sbc100 Apr 8, 2025

Make 'instantiateWasm' callback resolve the result return from 'recei… #23781

Are you sure you want to change the base?

Make 'instantiateWasm' callback resolve the result return from 'recei… #23781

Conversation

gogogoghost commented Feb 27, 2025 • edited Loading

sbc100 left a comment

Choose a reason for hiding this comment

gogogoghost commented Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented Mar 3, 2025

gogogoghost commented Mar 19, 2025

caiiiycuk commented Apr 2, 2025 • edited Loading

caiiiycuk commented Apr 2, 2025

sbc100 commented Apr 2, 2025

Choose a reason for hiding this comment

gogogoghost commented Feb 27, 2025 •

edited

Loading

gogogoghost commented Feb 28, 2025 •

edited

Loading

caiiiycuk commented Apr 2, 2025 •

edited

Loading