wasm InvokeJS return value is broken

[kg]

Description

The transition to using a custom closure instead of eval broke the return value of InvokeJS.

Reproduction Steps

        var res = Interop.Runtime.InvokeJS(@"1 + 2", out int exceptionalResult);
        if (exceptionalResult != 0)
            throw new Exception("InvokeJS failed " + res);
        else if (res != "3")
            throw new Exception("InvokeJS returned invalid result " + res);

Expected behavior

Doesn't throw

Actual behavior

Throws, exception message indicates there was no return value

Regression?

Yes

Known Workarounds

If you put 'return' in the JS expression it may work

Configuration

No response

Other information

No response

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The transition to using a custom closure instead of eval broke the return value of InvokeJS.

Reproduction Steps

        var res = Interop.Runtime.InvokeJS(@"1 + 2", out int exceptionalResult);
        if (exceptionalResult != 0)
            throw new Exception("InvokeJS failed " + res);
        else if (res != "3")
            throw new Exception("InvokeJS returned invalid result " + res);

Expected behavior

Doesn't throw

Actual behavior

Throws, exception message indicates there was no return value

Regression?

Yes

Known Workarounds

If you put 'return' in the JS expression it may work

Configuration

No response

Other information

No response

Author:	kg
Assignees:	-
Labels:	arch-wasm
Milestone:	-

[kg]

cc @pavelsavara

The ideal would be for us to figure out that we need to insert a 'return' keyword somehow, but I'm not sure there's any obviously correct way to do that.

[kg]

The new version of InvokeJS appears to also be considerably slower than the old one (~2x slower for a simple test), according to my benchmarks. While I would hope no one is relying on it to have high performance, it's worth trying to figure out whether we can accept the hit (and whether it's potentially much worse in Firefox or Safari, since I've only run these benchmarks in v8.)

[pavelsavara]

Is slower speed of micro-benchmark caused by the fact that eval() is interpreted and function constructor is JIT ?
Anyway, I don't think people should use this in perf critical scenarios.
We have new Function("a", "b", "return a + b"); in C# which would perform much better with re-use of the function, than InvokeJS which needs to parse code on each call.

I don't have any good idea how to make 1+2 expression work either.
We could get back to eval, but I don't like it.
I will think about it some more, especially how to make that eval with MONO, BINDING, Module in the closure instead of global.

[maraf]

The previous version with eval supported a scenarios where state could be shared between InvokeJS calls.

Interop.Runtime.InvokeJS("var a = 5;");

...

var a = Interop.Runtime.InvokeJS("a");

Is this a scenario we want to support?

This is actually not true. It works this way only when the eval is executed from global scope (not within a function).

[pavelsavara]

The original implementation was essentially

EM_ASM_INT

var res = eval (js_str);
if (res === null || res === undefined)
  return 0;
else
   res = res.toString ();

[lewing]

The api is still private so we can definitely still discuss what the right course of action is.