Fix returning negative i64 values on web target

[kajacx]

Description

Fixes #3773, which is a bug where returning a negative i64 from a module caused a crash when wasmer itself was running inside the browser using the js feature.

Looking at js-sys's implementation of TryFrom, it looks like BigInt is converted to JsValue anyway before converting it further, so converting BigInt to JsValue just so it is converted back seems like extra work.

To be honest, this needs an automated test, but I don't really know to make one. Can node load wasmer with the js feature? That is worth looking into.

[syrusakbary]

Can we add tests verifying that the issue is fixed?

[kajacx]

Can we add tests verifying that the issue is fixed?

Yes, I wanted to do that, but I do know where to begin. I am looking into the tests folder in the repo, but I don't see any tests for the js web target that I could easily add too.

The closest file I could find is types_functions.rs, but I do not know if the automated test suit already runs this with the js feature or not.

[kajacx]

Ok, I have tried adding this test. It is loading the WASM plugin from a wat text just like the other tests, which makes it simpler to work with.

The plugin exports a function that adds one, then calls the imported "add one" function and then adds one again to add three in total. This tests back and forth exchanging of values between the plugin and the host. Same idea as I did with fp-bindgen.

I am testing it with interesting values that might overflow (near 0, i64::MIN and i64::MAX).

I have verified that this test fails on the current master branch but succeeds with this fix.

But what I am missing is a way to run this automatically. It seems that node can run WASM, but I will have to try that.

[syrusakbary]

Cool, feel free to add the test inside lib/api/tests/import_function.rs (you will need to replace #[wasm_bindgen] with #[universal_test]). Also, try to make the test as generic as possible (no wasm bindgen or js specific stuff such as console_error_panic_hook, as that's done automatically by the universal test)

You can also that the test is working by running make test-js-api @kajacx

[kajacx]

Cool, feel free to add the test inside lib/api/tests/import_function.rs (you will need to replace #[wasm_bindgen] with #[universal_test]). Also, try to make the test as generic as possible (no wasm bindgen or js specific stuff such as console_error_panic_hook, as that's done automatically by the universal test)

You can also that the test is working by running make test-js-api @kajacx

Ok, I have tried that, but I cannot get it to work. There seems to be some problem with i64 values on node. I checked out the current master, and ran the make-test-api command, but it gave the following error: (full error message)

---- module::calling_host_functions_with_negative_values_works_js output ----
    error output:
        panicked at 'called `Result::unwrap()` on an `Err` value: "RuntimeError { source: Js(JsValue(TypeError: wasm function signature contains illegal type
TypeError: wasm function signature contains illegal type

After doing some searching, I have found this article about bigint support in node, but I have not figures out how to pass the --experimental-wasm-bigint flag to node. Any help @syrusakbary? Thanks.

[syrusakbary]

Tests passed in CI so I suspect that you may have an old version of Node installed in your system @kajacx

[kajacx]

I am ill right now, so I will not be doing anything on the computer for a few weeks. You can merge the PR if it looks good, I will re-check it to my satisfaction later either way.

[ptitSeb]

@syrusakbary it seems ready to be merged?

[syrusakbary]

Yup, good to merge!