[browser] release eagerly created Task/Promise proxies on non-normal async interop paths - #134124
Merged
Merged
Conversation
Marshalling a Task to JS eagerly creates a TaskHolder and hands managed code its JSHandle. Two paths never released it: a JSExport whose Task was already completed, and getAssemblyExports failing before the promise was handed over. The promise now carries the handle number rather than the holder itself, so the holder is not retained once the handle is released. Also adds INTERNAL.getProxyCensus, which the tests use to count live proxies. Contributes to dotnet#132966
…ask completes synchronously Same fix as the CoreCLR wrapper: the promise carries the TaskHolder's handle number so the holder can be released without being retained by the promise. The bindAssemblyExports leak does not exist here, as the Mono wrapper has no equivalent error branch. Contributes to dotnet#132966
…re JS adopts it An async JSImport pre-creates a PromiseHolder and its GCHandle before calling into JS. If JS threw, or returned without producing a promise, nothing freed the holder. The holder is now registered on creation, released when the call fails or leaves the slot empty, and unregistered on both release paths. Dispose no longer assumes a callback, since a pre-created holder has none until JS adopts it.
ProxyLeakTest pins the JSHandle tables to their baseline across all four Task/Promise crossings, for completed and pending results and for both observed and abandoned ones, so a missed release on any non-normal path shows up as a growing table. Chromium only: draining a proxy needs a forced JS collection, and globalThis.gc is exposed by the --expose-gc argument this project passes for Chrome. Only the JSHandle tables are asserted on; the GCHandle table behind them is drained by the FinalizationRegistry a few entries per turn, so it lags by an unbounded amount and would make the assertions fragile rather than stricter.
ProxyLeakTest is built for both runtime flavors, but INTERNAL.getProxyCensus existed only in the CoreCLR interop tree, so every case in the class failed on Mono with 'getProxyCensus must be a Function but was undefined'. Mirror it over the Mono proxy tables.
With managed threads the census also counts proxies owned by other threads, which drain independently of the test and made a run fail on a count that had gone down rather than up, and getAssemblyExports never settles. Gate the class on IsNotMultithreadingSupported, and assert on growth rather than equality so that an unrelated drain cannot fail a test whose contract is only that a round trip must not add a proxy.
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. |
Contributor
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
…xport throws call_entry_point and bind_assembly_exports pre-allocate a Task proxy via begin_marshal_task_to_js(TaskPreCreated) and rely on end_marshal_task_to_js to adopt or release it. When the managed call fails, invoke_async_jsexport throws on is_args_exception before either wrapper reaches end_marshal_task_to_js, so the holder stays registered under its JSHandle and the proxy leaks. This corrects the claim in the earlier Mono commit that the bindAssemblyExports leak did not exist on Mono. The wrapper has no is_args_exception branch of its own, but invoke_async_jsexport has one, which is where the throw originates. CoreCLR is unaffected because it inlines the check and releases the holder before throwing. call_entry_point has the same shape and is fixed alongside it, though no test covers a synchronously throwing entrypoint.
3 tasks
This was referenced Sep 18, 2026
Member
Author
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Threaded asyncJSImport paths can still leak or double-release holders, and the tests do not cover those ownership cases.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Fixes eager Task/Promise proxy leaks across browser JS interop paths.
Changes:
- Releases eager handles on synchronous and error paths.
- Tracks pre-created promise holders and exposes proxy census diagnostics.
- Adds CoreCLR/Mono regression tests and helpers.
| File | Description |
|---|---|
marshal-to-js.ts (CoreCLR) |
Tags and releases eager Task handles. |
managed-exports.ts (CoreCLR) |
Releases holders on export errors. |
index.ts (CoreCLR) |
Exposes proxy census. |
gc-handles.ts (CoreCLR) |
Implements proxy census. |
marshal-to-js.ts (Mono) |
Mirrors eager handle cleanup. |
managed-exports.ts (Mono) |
Cleans up export failures. |
gc-handles.ts (Mono) |
Implements proxy census. |
exports-internal.ts |
Exposes Mono census diagnostics. |
ProxyLeakTest.cs |
Adds leak regression coverage. |
JavaScriptTestHelper.mjs |
Adds JavaScript test helpers. |
JavaScriptTestHelper.cs |
Adds exports/imports and test fixtures. |
*.csproj |
Includes the new test file. |
JSProxyContext.cs |
Tracks pre-created promise holders. |
JSFunctionBinding.cs |
Cleans up async import holders. |
…ugh the Task An async [JSImport] dispatched with DispatchJSImportAsyncPost leaves the caller awaiting the pre-created Task and returns, so nothing ever reads the exception slot that the bound wrapper writes when the JS function throws. The Task stayed pending forever: the await hung and the PromiseHolder leaked. bind_fn now delivers the failure through the Task when the caller is gone, which is what SystemInteropJS_InvokeJSImportSync already does for a bind failure. The condition is receiver_should_free plus a TaskPreCreated result slot, so the current-thread path and DiscardNoWait keep the synchronous convention. The CoreCLR tree has the same shape but no thread support, so nothing sets ReceiverShouldFree there yet. Marked with TODO-MT rather than adding a branch that cannot run.
InvokeJSImportImpl creates the holder before calling JS but only released it again on the single-threaded path. Under FEATURE_WASM_MANAGED_THREADS a throw, or a JS function returning null instead of a Promise, left the holder registered in ThreadJsOwnedHolders together with its GCHandle. JSProxyContext.Dispose frees holder.State as the other release paths already do, and walks a snapshot of the dictionary because the callback it invokes can re-enter and release a holder. Holders already taken that way are skipped, so the snapshot cannot turn a mutation into a double free. PromiseHolderCount exposes the managed half of the proxy census. A pre-created holder has no JS-side entry until JS adopts it, so a missed release is invisible to the JS-side census the other leak tests assert on. It is private and read through UnsafeAccessor, rooted from BindJSFunction because the runtime pack is trimmed when it is built.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The review found invalid GCV-handle disposal, an unreleased synchronous JSExport exception path, and incomplete proxy-census assertions.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
Member
Author
|
/azp run runtime-wasm |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…rt throws The async JSExport wrappers create the JS promise before invoking managed code. A synchronous managed throw is marshaled into the exception slot and rethrown out of invoke_async_jsexport, which unwinds past end_marshal_task_to_js, so the eagerly created proxy was never adopted nor released and stayed rooted in the JSHandle table. Release it on the catch path in all three wrappers, in both the Mono and CoreCLR implementations. Also guard Dispose against freeing a GCVHandle, which is a synthetic index rather than a real GCHandle, matching ReleasePromiseHolder.
…ports rethrow path abortPosix swallows ExitStatus and RuntimeError and returns, so control reaches the rethrow and the eagerly created proxy stayed rooted. The Mono implementation already released it in the equivalent position.
Member
Author
|
/azp run runtime-wasm |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Specialized async wrappers and worker teardown paths lack regression coverage, leaving important leak fixes insufficiently verified.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
Resolved since last review (6)
This was referenced Sep 21, 2026
Open
maraf
approved these changes
Sep 22, 2026
Member
Author
|
/ba-g known infra issue |
pavelsavara
enabled auto-merge (squash)
September 22, 2026 14:50
maraf
approved these changes
Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Async JS interop eagerly creates a
Task/Promiseproxy pair before it knows whether the other half will ever arrive. Several non-normal paths never released the eagerly created half, leaking one proxy per call for the lifetime of the page. One of them also hung.Released the eagerly created proxy when:
[JSExport]returningTaskcompletes synchronously.[JSExport]returningTaskthrows during the invocation itself, so thePromiseJavaScript created for it is never adopted.getAssemblyExportsfails before its promise is handed to JavaScript.[JSImport]throws, or returns without producing a promise, on both the single-threaded and the threaded paths.Two further fixes came out of reviewing the threaded paths:
JSProxyContext.Disposenow freesholder.Stateas the other release paths already do, and walks a snapshot ofThreadJsOwnedHoldersbecause the callback it invokes can re-enter and release a holder. Holders already taken that way are skipped, so the snapshot cannot turn a mutation into a double free. It also no longer passes a GCVHandle toGCHandle.Free, matchingReleasePromiseHolder.[JSImport]dispatched withDispatchJSImportAsyncPostleaves the caller awaiting the pre-createdTaskand returns, so nothing ever reads the exception slot the bound wrapper writes when the JS function throws. TheTaskstayed pending forever: theawaithung and the holder leaked.bind_fnnow delivers the failure through theTask, which is whatSystemInteropJS_InvokeJSImportSyncalready did for a bind failure.The throwing-
[JSExport]fix applies to all three async wrappers in both the Mono and the CoreCLR implementation. A synchronous managed throw is marshaled into the exception slot and rethrown out ofinvoke_async_jsexport, which unwinds pastend_marshal_task_to_js— the point that would otherwise adopt or release the eager proxy.The CoreCLR tree has the same wrapper shape but no thread support, so nothing sets
ReceiverShouldFreethere yet. The async-[JSImport]delivery fix is marked withTODO-MTrather than adding a branch that cannot run.Tests
ProxyLeakTestexercises completed, pending, faulted and throwingTask/Promisevalues passed in both directions, including observed and abandoned results. It warms the bindings, repeats each case 100 times, and asserts that the JSHandle tables do not grow.Those assertions deliberately cover only the two JSHandle tables. The GCHandle table behind them is drained by the JavaScript
FinalizationRegistry, which lags by an unbounded amount, so asserting on it would make the test flaky rather than stricter. For the JSPromiseto managedTaskdirection this means the path is exercised but only the JSHandle side is verified.PromiseHolderLeakTestcovers what that census structurally cannot see: a holder has no JS-side entry until JavaScript adopts it, so a missed release on a throwing async[JSImport]is invisible to the JS-side tables. It reads the managed holder count throughUnsafeAccessoron a privateJSFunctionBinding.PromiseHolderCount, rooted fromBindJSFunctionbecause the runtime pack is trimmed when it is built.That test is deliberately not gated: it guards a different defect on each flavour. Single-threaded it covers the release on the throwing path; multi-threaded it covers the async-post translation, which without the fix hangs instead of leaking.
Validation
[JSExport]: removing the catch-path release makes only the new case fail, withcsOwnedByJsHandlegoing from 3 to 103 — exactly one leaked proxy per iteration.[JSImport]: disabling the single-threaded release makesPromiseHolderLeakTestreportpromise holders before: 1, after: 101.Not covered, and stated plainly:
catchrelease is only reachable from aJSWebWorker-affine import, and theDisposechanges need a worker teardown with live holders — neither has a test.The unrelated cancelled HTTP response-read leak is tracked separately in #134069.
Resolves #132966
Note
This pull request description was generated with the assistance of GitHub Copilot.