-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[browser] host improvements #123726
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
base: main
Are you sure you want to change the base?
[browser] host improvements #123726
Conversation
|
Tagging subscribers to this area: @agocke, @jeffschwMSFT, @elinor-fung |
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
Updates the WASM browser host/loader to unify the virtual app base (/managed), improve runtime startup/exit behavior across browser/Node/V8 shells, and tighten fail-fast/debug behavior.
Changes:
- Standardize the virtual app base/working directory to
/managedacross CoreCLR/Mono tooling and runtime. - Refactor browserhost asset/wasm instantiation and loader startup flow (polyfills/config validation/modules hooks).
- Adjust fail-fast/exit/debug behavior (probe memory, debugbreak behavior, abort/trap paths, syscall trimming).
Reviewed changes
Copilot reviewed 51 out of 52 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/minipal/getexepath.h | Align WASM exe path convention to /managed. |
| src/native/libs/System.Runtime.InteropServices.JavaScript.Native/libSystem.Runtime.InteropServices.JavaScript.Native.footer.js | Formatting/dep list cleanup. |
| src/native/libs/System.Runtime.InteropServices.JavaScript.Native/interop/utils.ts | Add TextDecoder fallback for V8 via Module helper. |
| src/native/libs/System.Runtime.InteropServices.JavaScript.Native/interop/http.ts | Centralize debug-only asserts in commonAsserts. |
| src/native/libs/System.Native.Browser/utils/strings.ts | Use globalThis.TextDecoder/TextEncoder checks. |
| src/native/libs/System.Native.Browser/utils/runtime-list.ts | Use shared globalThisAny helper. |
| src/native/libs/System.Native.Browser/utils/host.ts | Wrap timers; revise abort/exit path for browser. |
| src/native/libs/System.Native.Browser/native/scheduling.ts | Fail-fast wrapper around timer/background callbacks. |
| src/native/libs/System.Native.Browser/native/crypto.ts | Adjust crypto warning-once behavior/logging. |
| src/native/libs/System.Native.Browser/libSystem.Native.Browser.footer.js | Formatting/dep list cleanup. |
| src/native/libs/System.Native.Browser/libSystem.Native.Browser.Utils.footer.js | Add deps for trap support; formatting cleanup. |
| src/native/libs/Common/JavaScript/types/internal.ts | Add Emscripten preInit/preRun/postRun typing. |
| src/native/libs/Common/JavaScript/types/exchange.ts | Move host asset functions into host/assets. |
| src/native/libs/Common/JavaScript/types/ems-ambient.ts | Extend ambient types (FS.chdir, ENV, abort, trap). |
| src/native/libs/Common/JavaScript/per-module/index.ts | Add globalThisAny and browserVirtualAppBase. |
| src/native/libs/Common/JavaScript/cross-module/index.ts | Plumb instantiateWasm through internals table. |
| src/native/libs/Common/JavaScript/CMakeLists.txt | Include new browserhost host module sources. |
| src/native/corehost/browserhost/loader/run.ts | Restructure startup flow; add config validation + polyfills init. |
| src/native/corehost/browserhost/loader/polyfills.ts | Make polyfills async; restructure Node/V8 fetch handling. |
| src/native/corehost/browserhost/loader/index.ts | Rename/route instantiate hook to instantiateMainWasm. |
| src/native/corehost/browserhost/loader/host-builder.ts | Validate config when reusing an existing HostBuilder. |
| src/native/corehost/browserhost/loader/exit.ts | Track runtime state; improve shell quit + abort handling. |
| src/native/corehost/browserhost/loader/dotnet.ts | Remove Node auto-self-hosting; init polyfills async. |
| src/native/corehost/browserhost/loader/config.ts | Default virtual working directory to /managed. |
| src/native/corehost/browserhost/loader/bootstrap.ts | Remove Node/Shell auto resource probing logic. |
| src/native/corehost/browserhost/loader/assets.ts | Refactor wasm instantiation + register bytes API. |
| src/native/corehost/browserhost/libBrowserHost.footer.js | Remove NodeFS mounting; trim unsupported socket syscalls. |
| src/native/corehost/browserhost/host/per-module.ts | Re-export shared per-module environment helpers. |
| src/native/corehost/browserhost/host/index.ts | Centralize Emscripten setup and export table wiring. |
| src/native/corehost/browserhost/host/host.ts | Use /managed base for TPA and app context paths. |
| src/native/corehost/browserhost/host/cross-module.ts | Re-export shared cross-module glue. |
| src/native/corehost/browserhost/host/assets.ts | New asset registry + instantiateWasm implementation. |
| src/native/corehost/browserhost/CMakeLists.txt | Enable table growth; drop nodefs link option. |
| src/mono/wasm/testassets/WasmBasicTestApp/App/FilesToIncludeInFileSystemTest.cs | Update VFS file checks to relative paths. |
| src/mono/wasm/Wasm.Build.Tests/FilesToIncludeInFileSystemTests.cs | Update expected output for relative VFS paths. |
| src/mono/wasm/Wasm.Build.Tests/Common/BuildEnvironment.cs | Clarify Webcil TODO with issue link. |
| src/mono/sample/wasm/console-v8/wwwroot/main.mjs | Add V8 console sample entrypoint module. |
| src/mono/sample/wasm/Directory.Build.targets | Run V8 using --module. |
| src/mono/sample/mbr/browser/WasmDelta.csproj | Fix TargetPath to be relative (no leading backslash). |
| src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.targets | Make default VFS TargetPath relative. |
| src/mono/browser/test-main.js | Default working directory to /managed. |
| src/mono/browser/runtime/startup.ts | Ensure /managed + working dir exists; chdir accordingly. |
| src/mono/browser/runtime/loader/globals.ts | Add browserVirtualAppBase constant. |
| src/mono/browser/runtime/loader/config.ts | Default virtual working directory to /managed. |
| src/mono/browser/runtime/globals.ts | Add browserVirtualAppBase constant. |
| src/mono/browser/runtime/assets.ts | Install VFS assets under /managed by default. |
| src/libraries/Common/tests/TestUtilities/System/AssemblyPathHelper.cs | Use /managed base for browser assembly paths. |
| src/coreclr/vm/precode_portable.cpp | Assert in all builds for NULL actual code. |
| src/coreclr/pal/src/debug/debug.cpp | Add browser PAL_ProbeMemory implementation. |
| src/coreclr/pal/src/arch/wasm/stubs.cpp | Fail-fast if debugger not attached (heuristic). |
| src/coreclr/hosts/corerun/wasm/libCorerun.extpost.js | Normalize Node cwd on Windows for Unix-like VFS. |
| src/coreclr/hosts/corerun/corerun.cpp | Keep browser runtime alive (no shutdown/exit). |
| } | ||
| #endif // TARGET_BROWSER | ||
| // In browser we don't shutdown the runtime here as we want to keep it alive | ||
| return 0; |
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.
I think this would break runtime tests on nodeJS as they depend on the exit code resolution.
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.
Ok, this needs bit more work
radekdoulik
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.
Besides the exit code change, it LGTM.
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
Copilot reviewed 53 out of 54 changed files in this pull request and generated 4 comments.
| function trim() { | ||
| return 138; // EOPNOTSUPP; | ||
| } | ||
|
|
||
| // TODO-WASM: fix PAL https://github.com/dotnet/runtime/issues/122506 | ||
| LibraryManager.library.__syscall_pipe = trim; | ||
| delete LibraryManager.library.__syscall_pipe__deps; | ||
|
|
||
| LibraryManager.library.__syscall_connect = trim; | ||
| delete LibraryManager.library.__syscall_connect__deps; | ||
|
|
||
| LibraryManager.library.__syscall_sendto = trim; | ||
| delete LibraryManager.library.__syscall_sendto__deps; | ||
|
|
||
| LibraryManager.library.__syscall_socket = trim; | ||
| delete LibraryManager.library.__syscall_socket__deps; |
Copilot
AI
Jan 29, 2026
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.
These syscall stubs return a positive errno value (138). Emscripten/musl syscall shims typically expect negative errno return values (or setting errno and returning -1). Returning a positive value risks being interpreted as a successful return (e.g., a valid fd) and can lead to incorrect behavior. Please adjust the stub to follow the expected error-return convention for _syscall* (e.g., return -EOPNOTSUPP / -138, or set errno appropriately and return -1).
| { | ||
| loc = loc.Substring(1); | ||
| const string browserVirtualAppBase = "/managed"; // keep in sync other places that define browserVirtualAppBase | ||
| loc = loc.Replace(browserVirtualAppBase, ""); |
Copilot
AI
Jan 29, 2026
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.
The comment says we need to strip the leading '/', but the new logic removes all occurrences of "/managed" via Replace and can leave a leading '/' (e.g., "/managed/Foo.dll" -> "/Foo.dll"). This is both inconsistent with the comment and potentially fragile (Replace can remove occurrences beyond the prefix). Consider normalizing by removing only the expected prefix ("/managed/") and/or trimming the leading '/' in a prefix-safe way so the comparison matches the intended Module.FullyQualifiedName format.
| loc = loc.Replace(browserVirtualAppBase, ""); | |
| if (loc.StartsWith(browserVirtualAppBase, StringComparison.Ordinal)) | |
| { | |
| int prefixLength = browserVirtualAppBase.Length; | |
| if (loc.Length > prefixLength && loc[prefixLength] == '/') | |
| { | |
| prefixLength++; | |
| } | |
| loc = loc[prefixLength..]; | |
| } | |
| else if (loc[0] == '/') | |
| { | |
| loc = loc[1..]; | |
| } |
| export const mainModulePromiseController = createPromiseCompletionSource<WebAssembly.Instance>(); | ||
| export const nativeModulePromiseController = createPromiseCompletionSource<EmscriptenModuleInternal>(() => { |
Copilot
AI
Jan 29, 2026
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.
mainModulePromiseController is created and resolved, but it isn’t referenced anywhere else in the repo (only defined/resolved in this file). If it’s not part of a planned public surface, consider removing it (and the corresponding resolve call) to avoid dead code; otherwise add the missing consumer so it’s clear why it exists.
| return res; | ||
| } | ||
| export async function instantiateMainWasm(imports: WebAssembly.Imports, successCallback: InstantiateWasmSuccessCallback): Promise<void> { | ||
| //asset |
Copilot
AI
Jan 29, 2026
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.
Leftover comment //asset doesn’t convey intent and looks like a temporary note. Please remove it or replace it with a descriptive comment about why instantiation is delegated via dotnetBrowserHostExports.
| //asset | |
| // Delegate instantiation to the browser host so it can configure and track the main WebAssembly module consistently across environments. |
Filesystem
coreRunto never exitcwd()incoreRunto pretend thatNODERAWFSon Windows is UnixbrowserVirtualAppBaseconstant(s) and unify the app directory to/managed.browserVirtualAppBase, unless explicitly absolute.WasmFilesToIncludeInFileSystembrowserVirtualAppBasefor TPAvirtualWorkingDirectoryand change current directory to itFail fast
PortableEntryPoint::GetActualCodeif the function is NULLint3does elsewherequiton V8abortPosiximplemented as emscripten___trap(), so that we don't run C++ finalizers onexit()Other
PAL_ProbeMemoryfor browser-sALLOW_TABLE_GROWTH=1for R2Rbrowserhost/host/assets.tsglobalThisAnybrowserHostto run on V8/D8 shellTextDecoder