[wasm] Block calls into managed code after a runtime assert #87043

kg · 2023-06-02T02:49:35Z

Right now if an assertion is hit in native code at any point, while we call exit and flush buffers and whatnot, it's still possible to call back into managed code.

This PR sets a flag when assertions are hit, and if the flag is set we reject attempts to call into managed code.

The included sample's buttons let you verify that managed exceptions being thrown doesn't set the flag (since the caller can catch and handle them) but a native assert in the runtime will set it.

I initially was setting the flag in response to calls to exit() as well, but then you can't call into managed code after Main returns, which isn't desirable with our current model.

cc @vargaz

ghost · 2023-06-02T02:49:45Z

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

Issue Details

Right now if an assertion is hit in native code at any point, while we call exit and flush buffers and whatnot, it's still possible to call back into managed code.

This PR sets a flag when assertions are hit, and if the flag is set we reject attempts to call into managed code.

The included sample's buttons let you verify that managed exceptions being thrown doesn't set the flag (since the caller can catch and handle them) but a native assert in the runtime will set it.

I initially was setting the flag in response to calls to exit() as well, but then you can't call into managed code after Main returns, which isn't desirable with our current model.

cc @vargaz

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

src/mono/wasm/runtime/globals.ts

pavelsavara · 2023-06-05T10:03:25Z

src/mono/wasm/runtime/loader/exit.ts

@@ -6,6 +6,12 @@ import { mono_log_debug, consoleWebSocket, mono_log_error, mono_log_info_no_pref

 export function abort_startup(reason: any, should_exit: boolean): void {
    mono_log_debug("abort_startup");
+    // FIXME: Should we set this after rejecting the promises?


I think this place is OK. You can remove the comment, I don't expect that somebody would try to recover from the rejections trowed here.

We also need to set runtimeExited in mono_exit method.

Originally I was setting it in mono_exit, but it caused calls in to stop working after main(), which breaks a bunch of the samples

Hmm.
We have dotnet.run() API implemented as runMainAndExit and I guess that's why.
It makes sense in console apps, but not so much in browser apps.

So perhaps web apps should not call runMainAndExit but just runMain. But that would break our (unit test) expectations about exit code of web apps.

@maraf what do you think ?

I think the exit code is also important since a startup failure usually produces a nonzero exit code

I think even web apps should call runMainAndExit and after that the runtime should be in the "don't touch me state". Web apps and probably any "interactive apps" should have async main that exits when the app done.

Would this be compatible with Blazor and Uno?
@jeromelaban

I think Blazor has async main which never resolves the Task, right @maraf ?

Exactly, this is typical blazor main https://github.com/dotnet/aspnetcore/blob/main/src/Components/WebAssembly/Samples/HostedBlazorWebassemblyApp/Client/Program.cs

https://github.com/dotnet/aspnetcore/blob/main/src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHost.cs#L109

@kg for Uno, we are using the runMain API to start the apps to call in a synchronous entry point that exits right away. If runMainAndExit implies that apps need to have an async main, this will be a breaking change.

I tend to think that the idea of the Main method handling exit in the browser does not make sense synchronously nor asynchronously, as terminating the app is not really possible (aside from closing the page/tab).

There is Module.noExitRuntime in the emscripten which is true in the browser. Perhaps we should set this new set_runtime_aborted only in shell ? Or when Module.noExitRuntime == false.

I think think that we also want to set emscripten's ABORT flag by calling _proc_exit() or abort() js functions. At least on current thread.

Also we should register set_runtime_aborted into Module.onAbort callback.

src/mono/sample/wasm/browser-shutdown/Program.cs

If runtimeHelpers.runtimeExited is set, reject attempts to call managed code from JS Checkpoint: Flow message through from Environment.FailFast Ensure we stop pumping timers and background exec upon assert Store asserted state in driver.c If we set the asserted state multiple times don't trample the first message that was set Revert damaged files

src/mono/wasm/runtime/exports.ts

pavelsavara · 2023-06-06T09:08:44Z

src/mono/wasm/runtime/exports.ts

@@ -125,6 +127,27 @@ function initializeExports(globalObjects: GlobalObjects): RuntimeAPI {
    return exportedRuntimeAPI;
 }

+function set_runtime_aborted (module: any, reason: any) {


please move both methods to src\mono\wasm\runtime\run.ts

pavelsavara · 2023-06-06T09:13:49Z

src/mono/wasm/runtime/scheduling.ts

@@ -63,5 +69,8 @@ export function mono_wasm_schedule_timer(shortestDueTimeMs: number): void {
 }

 function mono_wasm_schedule_timer_tick() {
+    if (cwraps.mono_wasm_get_runtime_aborted())


please add similar test into fatal_handler in src\mono\wasm\runtime\loader\run.ts

Loader cannot access cwraps

Via runtimeHelpers same way as you did set_runtime_aborted also with checking that it was already set

I'm just saying that fatal_handler doesn't have to try to exit if we are already aborted.

Ah I see what you mean. Will do

and maybe it should set abort if we are not aborted yet

pavelsavara · 2023-06-06T09:15:35Z

src/mono/wasm/runtime/loader/exit.ts

+    // Someone may have already set an abort message or aborted the runtime.
+    // For example, this will happen in the case of Environment.FailFast.
+    // If we don't have a reason message of our own, see if we can get an existing one.
+    if (!reason)


&& runtimeHelpers.get_runtime_abort_message it may not exist yet when abort_startup happens. Also conditional call to set_runtime_aborted

ghost · 2023-07-09T17:01:47Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

kg added the arch-wasm WebAssembly architecture label Jun 2, 2023

dotnet-issue-labeler bot added the area-System.Runtime.InteropServices.JavaScript label Jun 2, 2023

ghost assigned kg Jun 2, 2023

build-analysis bot mentioned this pull request Jun 2, 2023

Tracking issue for CI build timeouts #76454

Closed

pavelsavara reviewed Jun 5, 2023

View reviewed changes

src/mono/sample/wasm/browser-shutdown/Program.cs Show resolved Hide resolved

kg added 2 commits June 5, 2023 15:39

Repair merge damage

92a9e4d

kg force-pushed the wasm-runtime-abort-shutdown branch from 4e6da6b to 92a9e4d Compare June 5, 2023 22:45

kg requested a review from pavelsavara June 5, 2023 22:59

pavelsavara reviewed Jun 6, 2023

View reviewed changes

pavelsavara mentioned this pull request Jul 4, 2023

[browser] mono_exit improvements #88387

Merged

ghost closed this Jul 9, 2023

ghost locked as resolved and limited conversation to collaborators Aug 8, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm] Block calls into managed code after a runtime assert #87043

[wasm] Block calls into managed code after a runtime assert #87043

kg commented Jun 2, 2023

ghost commented Jun 2, 2023

pavelsavara Jun 5, 2023

kg Jun 5, 2023

pavelsavara Jun 5, 2023

kg Jun 5, 2023

maraf Jun 5, 2023 •

edited

Loading

kg Jun 5, 2023

pavelsavara Jun 5, 2023

maraf Jun 5, 2023 •

edited

Loading

jeromelaban Jun 5, 2023 •

edited

Loading

pavelsavara Jun 9, 2023 •

edited

Loading

pavelsavara Jun 6, 2023

pavelsavara Jun 6, 2023

kg Jun 6, 2023

pavelsavara Jun 6, 2023

pavelsavara Jun 6, 2023

kg Jun 6, 2023

pavelsavara Jun 6, 2023

pavelsavara Jun 6, 2023

ghost commented Jul 9, 2023

[wasm] Block calls into managed code after a runtime assert #87043

[wasm] Block calls into managed code after a runtime assert #87043

Conversation

kg commented Jun 2, 2023

ghost commented Jun 2, 2023

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

maraf Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maraf Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

jeromelaban Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

pavelsavara Jun 9, 2023 • 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

Choose a reason for hiding this comment

ghost commented Jul 9, 2023

maraf Jun 5, 2023 •

edited

Loading

maraf Jun 5, 2023 •

edited

Loading

jeromelaban Jun 5, 2023 •

edited

Loading

pavelsavara Jun 9, 2023 •

edited

Loading