Conversation
There was a problem hiding this comment.
Pull request overview
Cleans up the Browser/CoreCLR JS host surface by consolidating string/UTF-8 helpers into BrowserUtilsExports, updating interop call sites accordingly, and improving failure handling during WASM instantiation.
Changes:
- Move
utf8ToStringRelaxedusage from JS interop local utilities todotnetBrowserUtilsExportsand addviewOrCopy/utf8ToStringRelaxedto the BrowserUtils exports table. - Add
ReceiverShouldFreehandling inresolveOrRejectPromiseand update websocket send buffering to avoid SharedArrayBuffer usage. - Improve host/loader error handling by calling
exit(1, err)when WASM instantiation fails; remove/refresh a few stale WASM-TODO comments.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Runtime.InteropServices.JavaScript.Native/interop/web-socket.ts | Switch to BrowserUtilsExports for UTF-8 decoding and SAB-safe copying when sending. |
| src/native/libs/System.Runtime.InteropServices.JavaScript.Native/interop/utils.ts | Remove local utf8ToStringRelaxed helper and unused Module dependency. |
| src/native/libs/System.Runtime.InteropServices.JavaScript.Native/interop/types.ts | Trim perf-measure enum and rename prefixes from mono.* to clr.*. |
| src/native/libs/System.Runtime.InteropServices.JavaScript.Native/interop/marshal-to-js.ts | Add ReceiverShouldFree handling and free posted argument buffers. |
| src/native/libs/System.Native.Browser/utils/strings.ts | Centralize relaxed UTF-8 decoding and patch Emscripten UTF8Decoder for SAB. |
| src/native/libs/System.Native.Browser/utils/index.ts | Initialize strings + export utf8ToStringRelaxed and viewOrCopy via BrowserUtilsExportsTable. |
| src/native/libs/System.Native.Browser/diagnostics/types.ts | Remove duplicate type definitions; re-export shared types. |
| src/native/libs/Common/JavaScript/types/exchange.ts | Extend BrowserUtilsExports types to include new exports. |
| src/native/libs/Common/JavaScript/types/ems-ambient.ts | Add typings for UTF8ArrayToString and UTF8Decoder. |
| src/native/libs/Common/JavaScript/loader/run.ts | Update a WASM-TODO comment with an issue link. |
| src/native/libs/Common/JavaScript/loader/polyfills.ts | Remove a stale WASM-TODO comment. |
| src/native/libs/Common/JavaScript/loader/assets.ts | Call dotnetApi.exit(1, err) on instantiate failures. |
| src/native/libs/Common/JavaScript/host/assets.ts | Use UTF8ArrayToString for probing; call exit(1, err) on instantiate failures. |
| src/native/libs/Common/JavaScript/cross-module/index.ts | Update BrowserUtilsExports table mapping indices for new exports. |
| src/native/corehost/browserhost/CMakeLists.txt | Update comment to reference tracking issue. |
src/native/libs/System.Runtime.InteropServices.JavaScript.Native/interop/marshal-to-js.ts
Show resolved
Hide resolved
| module = result.module; | ||
| } catch (err) { | ||
| dotnetApi.exit(1, err); | ||
| throw err; |
There was a problem hiding this comment.
dotnetApi.exit(...) always throws (see Common/JavaScript/loader/exit.ts), so the subsequent throw err; is dead code and can be removed to avoid confusion about which error is propagated.
| throw err; |
| return { instance, module }; | ||
| } catch (err) { | ||
| _ems_.dotnetApi.exit(1, err); | ||
| throw err; |
There was a problem hiding this comment.
_ems_.dotnetApi.exit(...) throws, so the throw err; immediately after is unreachable and can be removed to make the control flow clearer.
| throw err; |
| public void Initialize() | ||
| { | ||
| slot.Type = MarshalerType.None; | ||
| #if FEATURE_WASM_MANAGED_THREADS | ||
| // we know that this is at the start of some JSImport call, but we don't know yet what would be the target thread | ||
| // also this is called multiple times | ||
| JSProxyContext.JSImportWithUnknownContext(); | ||
| slot.ContextHandle = IntPtr.Zero; | ||
| slot.ReceiverShouldFree = false; | ||
| #endif | ||
| slot.ReceiverShouldFree = false; | ||
| } |
There was a problem hiding this comment.
slot.ReceiverShouldFree is only defined under #if FEATURE_WASM_MANAGED_THREADS, but Initialize() now assigns it unconditionally. This will fail to compile when FEATURE_WASM_MANAGED_THREADS is not defined. Move the assignment back under the same #if (or add a non-threading field/alternative).
| const originalUTF8DecoderDecode = _ems_.UTF8Decoder.decode; | ||
| _ems_.UTF8Decoder.decode = function (input: Uint8Array): string { | ||
| const view = isSharedArrayBuffer(input.buffer) ? input.slice() : input; | ||
| return originalUTF8DecoderDecode.call(this, view); |
There was a problem hiding this comment.
With tsconfig.json using "strict": true, the _ems_.UTF8Decoder nullability and method override here won’t type-check: (1) _ems_.UTF8Decoder is typed as TextDecoder | null, so accessing .decode is "possibly null"; and (2) assigning a wrapper with signature (input: Uint8Array) => string is not assignable to TextDecoder.decode which accepts input?: BufferSource plus optional options. Capture a non-null local decoder variable (or use a non-null assertion) and make the wrapper accept the full decode signature (including undefined/options) and forward them after applying the SAB copy when input is a Uint8Array.
| const originalUTF8DecoderDecode = _ems_.UTF8Decoder.decode; | |
| _ems_.UTF8Decoder.decode = function (input: Uint8Array): string { | |
| const view = isSharedArrayBuffer(input.buffer) ? input.slice() : input; | |
| return originalUTF8DecoderDecode.call(this, view); | |
| const utf8Decoder = _ems_.UTF8Decoder!; | |
| const originalUTF8DecoderDecode = utf8Decoder.decode; | |
| utf8Decoder.decode = function (input?: BufferSource, options?: TextDecodeOptions): string { | |
| let actualInput = input; | |
| if (input instanceof Uint8Array && isSharedArrayBuffer(input.buffer)) { | |
| actualInput = input.slice(); | |
| } | |
| return originalUTF8DecoderDecode.call(this, actualInput, options); |
| if (length !== 0) { | ||
| // TODO-WASM: copy, because the provided ArrayBufferView value must not be shared in MT. | ||
| buffer = bufferView; | ||
| // the provided ArrayBufferView value must not be SAB. |
There was a problem hiding this comment.
The comment says the provided view “must not be SAB”, but the code is explicitly handling SharedArrayBuffer-backed views by copying via viewOrCopy. Consider rewording the comment to reflect the behavior (copying only when needed) to avoid confusion for future maintainers.
| // the provided ArrayBufferView value must not be SAB. | |
| // the provided ArrayBufferView may be backed by SharedArrayBuffer; viewOrCopy will copy when needed. |
| return { instance, module }; | ||
| } catch (err) { | ||
| _ems_.dotnetApi.exit(1, err); | ||
| throw err; |
There was a problem hiding this comment.
_ems_.dotnetApi.exit() throws (and marks the runtime as exited), so throw err; is unreachable here and will never run. Remove the redundant rethrow to avoid dead code and make it clearer that exit() is the terminal path.
| throw err; |
Fixes #120226