Repository navigation
Dispatch Mono [JSExport] by handle - #134507
Conversation
Two changes that do not need new public API. CoreCLR already binds exports by storing the generated wrapper in JSProxyContext.JSExportByHandle and letting JavaScript call CallJSExport with that handle. Mono instead looked the wrapper up natively by name and invoked the MonoMethod directly, so the two runtimes disagreed about how a bound export is reached, and a wrapper signature change would not be diagnosed: mono_class_get_method_from_name is passed -1 for the signature. Move BindManagedFunction into a file shared by both flavors, give Mono a CallJSExport and a handle-dispatching invoke in driver.c, and delete the SystemInteropJS_GetAssemblyExport internal call. BindAssemblyExports stays per-flavor: that is assembly bootstrap rather than dispatch, and the two implementations differ in whether an assembly without exports is an error. The generated JSImport stub no longer opens an unsafe context or calls Unsafe.SkipInit, so a project using only [JSImport] no longer needs AllowUnsafeBlocks and SYSLIB1074 is gone. [JSExport] still generates a pointer-based wrapper, so SYSLIB1075 remains until that changes. Also drop Interop.Runtime.BindAssemblyExports and GetAssemblyExport from the CoreCLR declarations. They were already unreachable: GetAssemblyExport has no implementation in the CoreCLR native library, and BindAssemblyExports there is the reverse thunk into managed code, so calling it as a P/Invoke would recurse.
pavelsavara
added
arch-wasm
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the thread-context fallback and Mono callback reflection overhead, and remove stale SYSLIB1074 resources.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
This PR removes unsafe code generation from [JSImport] stubs and unifies Mono [JSExport] dispatch through managed handles.
Changes:
- Replaces
Unsafe.SkipInitwith safe default initialization. - Adds handle-based Mono export dispatch and removes obsolete interop.
- Updates diagnostics, tests, and generated baselines.
Review findings:
- Moderate: Avoid requiring the interop thread during export binding; fall back to the main/UI context when needed.
- Moderate: Avoid reflection and boxing overhead for every Mono export callback.
- Nit: Remove obsolete
SYSLIB1074descriptors and resource strings.
| File | Description |
|---|---|
src/mono/browser/runtime/types/internal.ts |
Adds CSFnHandle. |
src/mono/browser/runtime/managed-exports.ts |
Adds handle-based dispatch. |
src/mono/browser/runtime/invoke-cs.ts |
Stores and invokes export handles. |
src/mono/browser/runtime/driver.c |
Adds native handle dispatch entry points. |
src/mono/browser/runtime/cwraps.ts |
Declares new native wrappers. |
src/mono/browser/runtime/corebindings.c |
Removes obsolete export lookup. |
src/libraries/System.Runtime.InteropServices.JavaScript/tests/JSImportGenerator.UnitTest/Fails.cs |
Updates unsafe diagnostic tests. |
src/libraries/System.Runtime.InteropServices.JavaScript/tests/JSImportGenerator.UnitTest/Compiles.cs |
Tests imports without unsafe blocks. |
src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.Mono.cs |
Removes Mono-specific binding logic. |
src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.Exports.cs |
Adds shared managed export binding. |
src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.CoreCLR.cs |
Removes duplicated binding logic. |
src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Interop/JavaScriptExports.Mono.cs |
Adds the Mono managed dispatcher. |
src/libraries/System.Runtime.InteropServices.JavaScript/src/System.Runtime.InteropServices.JavaScript.csproj |
Includes the shared exports implementation. |
src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/Marshaling/ImplicitArgumentGenerator.cs |
Uses default initialization. |
src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/Marshaling/BaseJSGenerator.cs |
Uses default initialization. |
src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSSignatureContext.cs |
Disables skip-init generation. |
src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs |
Removes the generated unsafe wrapper. |
src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/Analyzers/JSImportExportDiagnosticsAnalyzer.cs |
Supports optional unsafe diagnostics. |
src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/Analyzers/JSImportDiagnosticsAnalyzer.cs |
Stops reporting SYSLIB1074. |
src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/Analyzers/JSExportDiagnosticsAnalyzer.cs |
Retains SYSLIB1075. |
src/libraries/Common/src/Interop/Browser/Interop.Runtime.Mono.cs |
Removes obsolete interop declarations. |
src/libraries/Common/src/Interop/Browser/Interop.Runtime.CoreCLR.cs |
Removes obsolete interop declarations. |
Is the generated code actually safe according to the new unsafe definition, or are we just missing correct annotations somewhere? |
BindManagedFunction used AssertIsInteropThread(), which throws on a thread without JS interop in the multi-threaded build. JSFunctionBinding.BindManagedFunction documents the opposite: it can be reached from an assembly module initializer on such a thread, in which case the export binds to the UI thread. Add JSProxyContext.BindingContextOrMain() and use it, restoring that fallback. The JSImport analyzer no longer reports SYSLIB1074, so the descriptor and its three resource strings were unreferenced. Remove them and leave a marker on the id so it is not reused. The row in list-of-diagnostics.md stays: ids are retired, not recycled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jkotas My goal here was to emit code that doesn't require To make it safe by the new standard, we also need to
Both of those are invariants by construction, because the generator is self-consistent. The API would be ideally internal, but we can't do that with generated code. So I'm not 100% clear what are the rules and what are the options. This is interop, doing pointer arithmetics by nature. I guess we need to solve that before we approve the new APIs. Please advise. |
BindingContextOrMain lets BindManagedFunction register an export into a context owned by another thread, which is the documented module-initializer fallback. That made the handle allocation and the JSExportByHandle insert race with the owning thread's CallJSExport lookup: a non-atomic increment and an unsynchronized Dictionary write against a concurrent read. Make both fields private and reach them only through AllocJSExportHandle and TryGetJSExport, which take lock (this) under FEATURE_WASM_MANAGED_THREADS. This is the pattern the rest of JSProxyContext already uses for its shared tables, and it compiles away entirely in the single-threaded build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Another problem is lifetime. For example, some of the values that are wrapped by JSMarshalerArgument - like GCHandle - have manually managed lifetime. You need to make sure that 100% safe code cannot cause GCHandle double-free or use-after-free when working with JSMarshalerArgument (the usual problem is a copy of the struct created by accident). I can believe that you may be able to design JavaScript interop source generator that generates safe code only (in the new unsafe model). This hardening would come with performance overhead and it will require new public APIs. I am not sure whether it is worth it. It is common and acceptable for interop to use unsafe code.
The unsafe code rules apply equally to source code and code generated by the source generator. The AllowUnsafeBlocks requirement cannot be circumvented by claiming that the code was generated by a source generator. In the new unsafe model - if it is possible to hit a buffer overrun or other type of memory safety violation by calling safe public APIs, it means that annotations on the APIs are not right, and one of more APIs involved in the sequence should be annotated as unsafe.
Yes, I did not mean to block these cleanups. I commented on this because the PR description seemed to be declaring victory on the use of unsafe code prematurely. |
Does generated
Unless we accept the performance overhead and possibly complexity of the new API which would be safe by new rules, we would have to keep That makes "drop AllowUnsafeBlocks" part of my goal on this and on follow-up PRs moot. |
Yes. LibraryImport requires AllowUnsafeBlocks both with old and new rules. |
Dropping Unsafe.SkipInit and the generated unsafe block removed the pointer syntax, which is all the compiler's current rule keys on, so SYSLIB1074 stopped being reported and user projects could build without AllowUnsafeBlocks. That was masking the problem rather than fixing it. The generated stub still calls JSFunctionBinding.InvokeJS and the JSMarshalerArgument marshalers, 44 of whose 104 public ToManaged/ToJS members are declared unsafe. Under the caller-unsafe rules a member-level unsafe modifier requires callers to be in an unsafe context, so the requirement would come back and users who had removed the flag would have to add it again. Removing a requirement and later re-imposing it is worse than never removing it. LibraryImport requires AllowUnsafeBlocks under both the old and the new rules; JS interop is not different. Restore SYSLIB1074, its descriptor and resources, the SkipInit code emission and the generated unsafe block, leaving this branch as only the Mono JSExport dispatch unification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
pavelsavara
changed the title
|
Heads-up on scope: as of a47fdf4 this PR is only the Mono Reasoning: dropping the pointer syntax only satisfies the compiler's current rule. The stub still calls Two already-resolved review threads now describe code that is no longer here, so please disregard them rather than re-reviewing:
Still current and unchanged: the Verified after the revert: Note This comment was generated with GitHub Copilot. |
|
/ba-g unrelated CI failures |
Mono resolved each
[JSExport]to aMonoMethod*and kept that pointer in the JS-side binding closure, while CoreCLR already routed calls through a managed entry point. This unifies the two: both flavors now bind an export to an integer handle into a managed table, and the JS side stores aCSFnHandleinstead of aMonoMethod.Beyond the duplication, the Mono lookup could not diagnose a wrapper signature change:
mono_class_get_method_from_nameis passed-1, which means "any parameter count".JSHostImplementation.Exports.cs(new, shared) holds the reflection-basedBindManagedFunctionthat populates the handle table.JavaScriptExports.Mono.csgainsCallJSExport(int methodHandle, JSMarshalerArgument*), mirroring the CoreCLR entry point.driver.cfactors the existing invoke paths intoinvoke_jsexport_methodand addsmono_wasm_set_jsexport_dispatcherplus the*_by_handlesync/async/post entry points.BindAssemblyExportsdeliberately stays per-flavor: the CoreCLR version throws when an assembly contains no[JSExport], which the Mono version tolerates, and collapsing that difference would be a behavioral change unrelated to this cleanup.With dispatch going through managed code on both flavors, the
SystemInteropJS_GetAssemblyExporticall and the matchingGetAssemblyExport/BindAssemblyExportsLibraryImportdeclarations were dead and are removed.On
AllowUnsafeBlocksAn earlier revision of this PR also dropped
Unsafe.SkipInitand the generatedunsafeblock from the[JSImport]stub, which retiredSYSLIB1074and let user projects build without<AllowUnsafeBlocks>. That has been reverted — see the discussion below with @jkotas.Removing the pointer syntax only satisfies the compiler's current rule. The stub still calls
JSFunctionBinding.InvokeJSand theJSMarshalerArgumentmarshalers, 44 of whose 104 publicToManaged/ToJSmembers are declaredunsafe. Under the caller-unsafe rules a member-levelunsafemodifier requires its callers to be in an unsafe context, so the requirement would return and users who had removed the flag would have to add it back. Removing a requirement and later re-imposing it is worse than never removing it.LibraryImportrequiresAllowUnsafeBlocksunder both the old and the new rules. JS interop is not different, and making it genuinely caller-safe would need new public API and per-call validation whose cost is not obviously worth it. This PR is therefore only the dispatch unification.Testing
JSImportGenerator.Unit.Tests: 36 run, 0 failed.System.Runtime.InteropServices.JavaScripttests on browser-wasm CoreCLR (V8): 481 run, 472 passed, 0 failed, 9 skipped.RuntimeFlavor=CoreCLR,RuntimeFlavor=Mono, and Mono withWasmEnableThreads=true.The Mono runtime path is not exercised by the local runs above and relies on CI for coverage.
Context: #120215
Note
This pull request description was generated with GitHub Copilot.