[JSPI] Wrong order of execution when calling JSPI'ed function from deferred module

[Mintyboi]

Please include the following in your bug report:

Version of emscripten/emsdk:
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.65 (7f8a05d)
clang version 20.0.0git (https:/github.com/llvm/llvm-project 547917aebd1e79a8929b53f0ddf3b5185ee4df74)

Issue
I have a code like this

#include <stdio.h>
#include <emscripten.h>

extern "C" {
    extern void jsPrintSomething(char *);
    extern void saveProfileData();
}

void callJsFunc()
{
    printf(" ********* Calling JS async function from deferred module ********* \n");
    jsPrintSomething("from callJsFunc");
}

void foo() { callJsFunc(); }

extern "C" {
    EMSCRIPTEN_KEEPALIVE int main()
    {
        printf(" ********* Calling JS async function in primary module ********* \n");
        jsPrintSomething("from primary module");
        saveProfileData();
        
        foo();
        
        printf(" End of main function \n");
        return 0;
    }
}

and the js library as follows:

mergeInto(LibraryManager.library, {
    jsPrintSomething__sig: "vi",
    jsPrintSomething: function(ptr) {
        var ptrStr = UTF8ToString(ptr);
        return new Promise((resolve, reject) => {
            setTimeout(() => {
                console.log(`Printing ${ptrStr} after 1000ms`);
              resolve();
            }, 1000);
        });
    }
});

Build command:
emcc main.cpp -g -sMAIN_MODULE=2 -o index.js --js-library=jslibrary.js -sINCLUDE_FULL_LIBRARY=1 -sSPLIT_MODULE=1 -sJSPI
-sJSPI_IMPORTS=jsPrintSomething

After doing a split module and patching #22487, the Actual result I'm getting is

 ********* Calling JS async function in primary module ********* 
Printing from primary module after 1000ms
Profile data written to local disk
 ********* Calling JS async function from deferred module ********* 
 End of main function 
Printing from callJsFunc after 1000ms

Expected result:

 ********* Calling JS async function in primary module ********* 
Printing from primary module after 1000ms
Profile data written to local disk
 ********* Calling JS async function from deferred module ********* 
Printing from callJsFunc after 1000ms            <------ Application should pause here and wait for this line to get printed first
 End of main function 

Reproducer:
jspi_wrong_execution.zip

[tlively]

Hmm, I guess we're correctly waiting for the secondary module to be loaded, but then we're failing to wait again when the secondary function returns a Promise. @brendandahl, how complicated do you think this would be to fix?

[brendandahl]

I was thinking it would be as simple as adding suspending wrappers around the imports that are passed to the secondary module. e.g. in __load_secondary_module

wasmExports['%jsPrintSomething'] = new WebAssembly.Suspending(wasmExports['%jsPrintSomething']);

However, that doesn't seem to work and now that I think about it, I'm not sure if JSPI is supposed to work across wasm module calls, or do we need to have JSPI promising/suspending wrappers in between the module calls. Right now we have:

JS->mainPromisingWrapper->mainInPrimaryWasm->fooInSecondaryWasm->sleepSuspendingWrapper->sleepInJs

but do we instead need:

JS->mainPromisingWrapper->mainInPrimaryWasm->fooSuspendingWrapper->fooPromisingWrapper-> fooInSecondaryWasm->sleepSuspendingWrapper->sleepInJs

[tlively]

There should be no need for the wrappers in the wasm-to-wasm cross-module calls, but perhaps this is not working correctly in the current implementation. @fgmccabe, do you know if this is supposed to work correctly right now?

[fgmccabe]

plain (without jspi) wasm-to-wasm cross module calls are working (we have tests for that case).

[fgmccabe]

This:
wasmExports['%jsPrintSomething'] = new WebAssembly.Suspending(wasmExports['%jsPrintSomething']);

should cause an exception. Because new WebAssembly.Suspending is expecting a JS function, not a WebAssembly function.

[fgmccabe]

This:
JS->mainPromisingWrapper->mainInPrimaryWasm->fooInSecondaryWasm->sleepSuspendingWrapper->sleepInJs

is what we expect to work.

[brendandahl]

This: wasmExports['%jsPrintSomething'] = new WebAssembly.Suspending(wasmExports['%jsPrintSomething']);

should cause an exception. Because new WebAssembly.Suspending is expecting a JS function, not a WebAssembly function.

I had a bit more time to look into this and this is kind of the issue. wasmExports['%jsPrintSomething'] comes from the main wasm module, but it is the imported JS function that is also exported. If I wrap wasmExports['%jsPrintSomething'] with suspending, I don't get an error, but it doesn't work as expected. However, if I replace the export with the original JS function then it does work as expected e.g. wasmExports['%jsPrintSomething'] = new WebAssembly.Suspending(_jsPrintSomething);

Two options to fix:

1. Change V8 to support doing WebAssembly.Suspending(aWasmExportThatIsActuallyJS);
2. Change emscripten/binaryen to use the original JS import and wrap that. This option seems challening with how things are currently structured.

[brendandahl]

One thing to clarify above where naming is confusing, wasmExports is passed into the secondary module as the imports.

[fgmccabe]

There is a third option:

3. Wrap functions that are exported in a JS lambda:

WebAssembly.Suspending((X)=>wasmExport(X))

That will force the function argument to Suspending to be JS. And it would be a local change.

[tlively]

I talked to @brendandahl offline about this to understand the problem more, and I think this situation should not require any additional wrapping at all.

My understanding is that one instance imports new WebAssembly.Suspending(someJSFunc), then re-exports it directly as "%someJSFunc". The second instance then imports and calls "%someJSFunc", which suspends. This should not require any additional wrappers on the boundary between the first instance and the second instance because that is a Wasm-to-Wasm boundary, even though the call is to a directly re-exported JS function rather than a normal Wasm function. It's important to maintain the property that re-exporting an imported function has the same behavior as exporting a trampoline that simply calls the imported functoin.

@brendandahl, can you confirm that something goes wrong when there is no additional wrapping between the two modules? Can you also confirm that the proxy handler that loads the secondary module is not on the stack for this interaction? As we discussed, we need to add more wrappers in the proxy handler, but I'd like to confirm that this is a separate issue.

[fgmccabe]

Part of this is simplicity of specification. The Suspending spec is written to only take JS functions as arguments. This was 'part of the bargain' when the JSPI was simplified to not need an explicit Suspender object.

Requiring an exported wasm function to be recognized as a JS function would add extra corners to the specification for marginal gain.

Using the lambda wrapper is cheap and likely inlineable in most cases.

[tlively]

Neither wrapping of Wasm functions nor using a lambda should be necessary, since there should not need to be any wrappers between the two Wasm modules.