[browser] release eagerly created Task/Promise proxies on non-normal async interop paths - #134124
Draft
pavelsavara wants to merge 10 commits into
Draft
pavelsavara wants to merge 10 commits into
pavelsavara wants to merge 10 commits into
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
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
1 participant
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
Every async marshaling path eagerly allocates one half of a
Task/Promisepair before it knows whether the other half will ever arrive. The normal paths release it; three non-normal ones did not, leaking a proxy per call for the life of the page.1. A
[JSExport]returning aTaskthat completes synchronouslybegin_marshal_task_to_jsregisters theTaskHolderunder a JSHandle but hands back the bareholder.promise. The synchronous-completion branch ofend_marshal_task_to_jsthen looked the handle up from the promise, which was never tagged, so it minted a fresh unrelated handle, released that, and left the holder's real handle live forever.The promise now carries the holder's handle number, so the holder can be released without being retained by the promise. Fixed in both the CoreCLR and Mono interop trees.
2.
getAssemblyExportsfailing before the promise is handed overSame eager holder, released on the success path only; an early error branch dropped it. (CoreCLR tree only — the Mono wrapper has no equivalent branch.)
3. An async
[JSImport]that fails before JS adopts the holderInvokeJSImportImplpre-creates aPromiseHolderand itsGCHandlebefore calling into JS. If JS threw, or returned without producing a promise, nothing freed the holder. It is now registered on creation, released when the call fails or leaves the slot empty, and unregistered on both release paths.Disposeno longer assumes a callback exists, since a pre-created holder has none until JS adopts it.Tests
ProxyLeakTestcovers all fourTask/Promisecrossings — managedTaskin and out as both return value and argument — for completed and pending results, and for results that JS/managed either observes or abandons. It runs each case once to warm the bindings, then 100 times, and asserts the JSHandle tables did not grow.Sensitivity was checked by reverting each fix: the affected counts move from 1 to 101, so the tests fail without the product changes.
Only the JSHandle tables are asserted on. They are maintained by explicit release calls, which is exactly where a missed release shows up. The GCHandle table behind them is drained by the JS
FinalizationRegistrya few entries per turn, so it lags by an unbounded amount — raising the quiesce loop from 3 rounds to 20 moved a count from 101 down to 29 without ever reaching a steady state, which is why asserting on it would be fragile rather than stricter.The class is restricted to Chromium (draining a proxy needs
globalThis.gc, exposed by the--expose-gcargument this project passes for Chrome) and to a single-threaded runtime (with managed threads the census also counts proxies owned by other threads, which drain independently of the test).Validation
Notes
A fourth leak found in the same sweep — a cancelled HTTP response read orphaning its fetch promise — is split out into #134069, since it is a
System.Net.Httpconcern rather than a marshaling one.Resolves #132966
Note
This pull request description was generated with the assistance of GitHub Copilot.