-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[browser] throw and marshal exception caused by invalid arguments in Tasks #123523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[browser] throw and marshal exception caused by invalid arguments in Tasks #123523
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical issue where the WASM runtime would crash when marshalling a Task<T> from JavaScript to C# if the task's result value failed a type assertion. The root cause was that type assertion failures in the res_converter function would call mono_assert, which aborts the entire runtime via nativeAbort.
Changes:
- Wrapped the
res_convertercall in a try-catch block to capture exceptions and propagate them as Task exceptions instead of crashing the runtime - Added comprehensive test coverage for out-of-range value scenarios (short, byte) and type assertion failures (string vs long)
- Added helper functions in both JavaScript and C# test infrastructure to support the new test cases
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mono/browser/runtime/managed-exports.ts | Added try-catch around res_converter call to catch type assertion errors and propagate them as Task exceptions instead of aborting the runtime |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/JavaScriptTestHelper.mjs | Added returnResolvedPromiseWithIntMaxValue helper function to test overflow scenarios |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/JavaScriptTestHelper.cs | Added JSImport declarations for overflow tests and JSExport methods for awaiting Task and Task |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/JSImportTest.cs | Added test cases for short and byte overflow scenarios when marshalling from JS to C# |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/JSExportTest.cs | Added test cases for type assertion failures (short overflow and string type mismatch) |
...teropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/JSImportTest.cs
Outdated
Show resolved
Hide resolved
...teropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/JSImportTest.cs
Outdated
Show resolved
Hide resolved
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
#Hi @ArcadeMode, thanks for your effort! 💙 For this one, I guess we need to discuss the overall intent. I would summarize it as: "don't abort whole runtime, when marshaling assert fails, instead just throw (and marshal) exception". I think improving it makes sense. The scenarios when the JS value can't be marshaled are
Those conversions happen when
The scenarios when the C# value can't be marshaled are
Those conversions happen when
It would be great to cover all of those use-cases with a unit test and fix all paths. |
|
+1 on "don't abort whole runtime, when marshaling assert fails, instead just throw (and marshal) exception" |
|
Thanks for the pointers and agreement on direction @pavelsavara, @maraf. I'll go ahead and look at the other cases you have mentioned and see what might have to be done. I have yet to look at I'll drop a message here when i'm done with the feedback. |
|
For completeness sake, I want to mention that 'regular' assertion failures during marshalling do not cause the runtime to exit, instead, an error is thrown back to the caller of the function. There are also unit tests already confirming this behavior such as This PR is revolving around method invocations that include Task/Promise specifically as either return type or argument type. These values may resolve later and thus cannot throw the assertion failure back to the caller (the result isnt known yet). Currently the promise/task resolving with a value that wont pass type assertion, will throw an unobservable error and cause the behavior described above. The proposed (and thus far agreed upon) solution is to reject the promise / set exception on the task so that the receiver of the Task/Promise can observe the error. (receiver I see as either the return value receiver or method argument receiver). So, with respect to the mentioned cases I want to scope the relevance to Promise/Task related conversions.
I will start with the ➡ cases. If this tailoring of scope is undesirable please let me know. |
Covering more data types and also JSExport direction would be appreciated. |
This is running on CoreCLR instead of Mono and has the separate JS interop. It manifests as spinning forever, probably in the "already exited". Here we want to make the same fix to marshaling for CoreCLR. |
src/native/libs/System.Runtime.InteropServices.JavaScript.Native/interop/marshal.ts
Outdated
Show resolved
Hide resolved
|
@pavelsavara this should now cover the discussed cases, tested a few numerical types, dates and a numerical > string case, with and without Tasks and with delegates. Brought the fixes to the CLR implementation too. I tried adding tests for delegates+Tasks but While testing I found that there were no range checks for Date implemented, this turned out to be a new path that caused the runtime to exit. So that was nice to fix while I was at it. |
pavelsavara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, many thanks!
There are infra problems with CI last 2 days, I want to see it pass before we merge. I will trigger it again when I have some update
|
When looking at your other PR I realized that there are other out-of-range scenarios which would be nice to test here. JS Array could contain
|
|
Also CI fails with That killed the runtime instead of marshaling the exception |
|
@pavelsavara I have resolved the failing test, used the wrong assertion method. w.r.t the int32 overflow tests. I have made a test that demonstrates the current behavior (overflowing ints). While undesirable it it no regression caused by the fixes presented in this PR, so I'd like to move forward with this PR before having a look at the array value assertions. I hope thats okay with you. I've created #123731 for the array value overflow. I'd like take a look at that after completing the span<float> implementation. then I can take a look at all array types in one go, I expect none of the types to get value assertions so there might be issues for other types as well. |
|
/ba-g CI timeouts |
|
Great work @ArcadeMode, many thanks! If you want to chat sometimes, some of us are in https://aka.ms/dotnet-discord |
When marshalling a

Task<T>from JS to CS and the value fails a type assertion, this wouldcrashkill the wasm runtime. It seems to cause some kind of loop that starts printing the same message nested into itself untill the page gives out, I've also seen the browser tab hit out of memory cases. The console gets filled with this: (look at that scrollbar)The message gives the impression that the main method is not running, however the error hits a case that kills the mono runtime regardless of whether main is running or not.
I have added some tests that demonstrated the issue in a few cases and applied a fix that instead sets the assertion error as the Exception result of the Task being marshalled. This way the user will be informed on the C# side that the assertion failed. This seems to me the most reasonable way of propagating the exception.