fix(alc): sweep collectible-ALC static-cache pins (WASM batch 1) - #23707
fix(alc): sweep collectible-ALC static-cache pins (WASM batch 1)#23707nickrandolph wants to merge 15 commits into
Conversation
|
Nick Randolph seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Pull request overview
This PR addresses collectible AssemblyLoadContext (ALC) unload leaks on WebAssembly by sweeping process-lifetime static caches that retain app Assembly/Type/delegate references (since AssemblyLoadContext.Unloading is not raised on WASM). The sweeps are wired into the existing ALC cleanup hook (Application.CleanupNonDefaultAlcCaches via Window.CloseAlcWindow) or equivalent teardown paths, and are guarded by targeted unit/runtime tests.
Changes:
- Added ALC-scoped cleanup APIs for several process-wide registries (lookup assemblies, WindowId maps, WASM UIElement registrar, CompositionTarget handlers, HtmlElementHelper, PagePool, Hot Reload operation retention).
- Wired new sweeps into
Application.CleanupNonDefaultAlcCaches/Window.CloseAlcWindow. - Added new unit/runtime tests and a batch spec documenting the findings/fixes.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Uno.UWP/UI/ViewManagement/ApplicationView.cs | Add DestroyForWindowId to release per-window registry entries. |
| src/Uno.UWP/Microsoft/UI/Windowing/AppWindow.cs | Add DestroyForWindowId to release per-window registry entries. |
| src/Uno.UWP/ApplicationModel/Resources/ResourceLoader.static.cs | Add sweep to drop non-default-ALC lookup assemblies and rebuild caches. |
| src/Uno.UWP/ApplicationModel/DataTransfer/DragDrop/Core/CoreDragDropManager.cs | Add DestroyForWindowId to release per-window registry entries. |
| src/Uno.UI/UI/Xaml/Window/Window.alc.cs | Invoke new DestroyForWindowId cleanup calls on ALC window close. |
| src/Uno.UI/UI/Xaml/UIElement.wasm.cs | Add WASM registrar sweep for non-default-ALC Type keys. |
| src/Uno.UI/UI/Xaml/Style/Style.cs | Extend non-default-ALC key sweeping to UseUWPDefaultStylesOverride. |
| src/Uno.UI/UI/Xaml/PagePool.cs | Make pool process-wide; start scavenger only when enqueuing and pooling enabled; add ALC sweep. |
| src/Uno.UI/UI/Xaml/Media/CompositionTarget.wasm.cs | Add ALC handler sweep; reuse a dispatch snapshot buffer to reduce per-frame allocations. |
| src/Uno.UI/UI/Xaml/HtmlElementHelper.wasm.cs | Add sweep for non-default-ALC Type-keyed cache entries. |
| src/Uno.UI/UI/Xaml/Controls/Frame/Frame.legacy.cs | Stop per-Frame pool creation; use the process-wide PagePool.Instance. |
| src/Uno.UI/UI/Xaml/Application.Alc.cs | Wire new sweeps into CleanupNonDefaultAlcCaches. |
| src/Uno.UI.UnitTests/Windows_UI_Xaml/Given_WindowId_Maps_Alc.cs | New unit test for WindowId map removals. |
| src/Uno.UI.UnitTests/Windows_UI_Xaml/Given_ResidualTypeStatics_Alc.cs | New unit test for UseUWPDefaultStylesOverride ALC sweep. |
| src/Uno.UI.UnitTests/ResourceLoader/Given_ResourceLoader_Alc.cs | New unit test for ResourceLoader ALC sweep. |
| src/Uno.UI.RuntimeTests/Tests/HotReload/Given_HotReloadClientOperation_Alc.cs | New runtime test for hot-reload raw Type[] release on terminal state. |
| src/Uno.UI.RemoteControl/HotReload/ClientHotReloadProcessor.Common.Status.cs | Cap local operation history and drop raw Type[] at terminal state. |
| specs/048-wasm-alc-pin-sweeps/spec.md | New spec documenting Batch 1 findings, patterns, fixes, and verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The build 221560 found UI Test snapshots differences: Details
|
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23707/wasm-skia-net9/index.html |
|
The build 221668 found UI Test snapshots differences: Details
|
autocarl
left a comment
There was a problem hiding this comment.
Reviewed at exact head 28a8604d04553f5bd2fc6dacebc98e1fe188155e, including the synthetic merge against current master (ba8b3420c9f6f513411444bfeb3dadfbdced6b76). Hosted CI is green and the merge tree is conflict-free, but this is fix-first: terminal hot-reload cleanup breaks an existing paused-update consumer; per-window teardown performs destructive all-ALC sweeps; and failed operation history still retains collectible exception objects. I also found two narrower cleanup/lifecycle gaps inline. I am not approving this revision.
…graphs and pause-correlation types - CompositionTarget (WASM): check Target and Method.DeclaringType independently when pruning Rendering handlers (coalescing let a collectible-ALC method survive behind a default-ALC target); add ClearAlcHandlers(alc) scoped overload - ResourceLoader: add ClearAlcAssemblies(alc) scoped overload; parameterless all-non-default sweep delegates to a shared core and stays for global shutdown - Application/Window ALC teardown: thread the dying ALC into CleanupNonDefaultAlcCaches so the destructive sweeps (ResourceLoader lookup assemblies, CompositionTarget handlers) no longer break a live sibling secondary app; cache-only sweeps stay all-non-default (rebuild on demand) - HotReloadClientOperation: detach previewed-app exception graphs on terminal release (default-ALC summary keeps type/message/stack text); add type-correlation scope so TryUpdateFilesAsync reads completed ops Types for pauseHandle.Drop, with a finally sweep preserving the ALC release guarantee - PagePool: drop all pooled instances when pooling is disabled (stale pages could otherwise be served past TTL on re-enable) and never dequeue an entry older than TimeToLive - tests: exception-graph detach + correlation-scope retention (Given_HotReloadClientOperation_Alc), ALC-scoped ResourceLoader sweep keeps sibling ALC (Given_ResourceLoader_Alc) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23707/docs/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23707/wasm-skia-net9/index.html |
|
The build 225057 found UI Test snapshots differences: Details
|
ajpinedam
left a comment
There was a problem hiding this comment.
Review panel — high and medium findings
Output of an eight-lens reviewer panel (architect, contract, skeptic, quality, operability, performance, security, plus a Socratic questioner) run against master..HEAD at 6a9d489. Every anchor below was re-verified against the code at that commit.
20 inline comments follow: 9 high, 11 medium, plus two further high findings at the end that are not tied to a single changed line. Each notes which lenses raised it — items flagged independently by three or more are the ones I would fix first:
dyingAlcoptional → the unscoped destructive path is still reachable fromSetCurrentApplication(null)(architect, skeptic, operability)UseUWPDefaultStylesOverrideswept as a cache when it is user configuration that never rebuilds (architect, skeptic, contract)ClearAlcAssembliesCoredestroys before an unguarded rebuild, outsideRunCleanupStep(operability, performance, skeptic, security)- process-global
_activeTypeCorrelationScopesmakes concurrent callers interfere and rests the release guarantee on a convention (architect, skeptic, operability, quality, performance) - teardown sweeps mutate non-thread-safe collections that other threads iterate (skeptic, architect, jerome)
Two cross-cutting notes that are not tied to a single line:
Window.CloseAlcWindows()(Window.alc.cs:445-458) still closes every secondary-ALC window, so this commit's headline — scoping window teardown to the dying ALC — is not fulfilled on the path that matters. App A callingExit()still tears sibling B down:RaiseClosed, content cleared, content root removed,DestroyForWindowId, thenCleanupNonDefaultAlcCaches(B_alc). It is also unscoped within one ALC that owns two windows. (skeptic, jerome, architect)- The Skia twin of the swept WASM pin is untouched.
CompositionTarget.Rendering.skia.cs:82holds the same static-event pin (plus_latestFrames/_targets), while ALC hosting is#if __SKIA__ || __WASM__. Either sweep it or record in the spec why it cannot pin — as written, a reader cannot tell whether Skia is done or forgotten, and Skia is the primary target. (quality, architect, jerome)
Scoped to high and medium at the author's request — the panel also produced one blocker-severity finding, a set of low/info items (comment-history narration, scavenger idle tick, Array.Clear of full buffer capacity each frame, PagePool.IsPoolingEnabled public but unread, the Tests.AssemblyLoadContext namespace shadowing that two commits here work around), and several open design questions. Not included here.
- Application.Alc: make CleanupNonDefaultAlcCaches' dyingAlc REQUIRED; global shutdown opts into the wide sweep via explicit CleanupAllSecondaryAlcCaches; Warning logged whenever the unscoped destructive branch runs - Style: exclude UseUWPDefaultStylesOverride (user configuration) from the all-non-default cache-clear group; swept only for the dying ALC via new RemoveAlcScopedUserStyleOverrides, never wholesale - ResourceLoader: drop only the dying assemblies' registrations and parsed markers; no destroy-before-rebuild (no loader clear, no O(all-resources) re-parse, no throw-leaves-empty failure mode); call routed via RunCleanupStep - dedup the four "drop collectible Type keys" loops into shared Uno.UI.Helpers.AlcCacheSweep (also hosts IsFromUnloadInitiatedAlc) - PagePool: back to per-Frame pools (no cross-Frame/XamlRoot page reuse) via a process-wide WEAK registry swept by the ALC hook and one shared scavenger; per-pool lock (sweep vs scavenger vs navigation races); GC.Collect gated on Android/iOS only; dequeue age-checks entries individually instead of list.Clear(); Instance singleton removed (ctor was already internal on master) - CompositionTarget (WASM): per-handler try/catch in the frame dispatcher (a throwing Rendering handler no longer kills the frame loop); _requestedFrame restructured to "callback pending" invariant cleared on entry/re-armed in finally; reentrancy-safe dispatch (local snapshot on re-entry); handler list guarded by a lock; ownership predicate moved to platform-neutral CompositionTargetHandlerSweep - HtmlElementHelper/UIElementNativeRegistrar (WASM): locked, use shared sweep - hot reload Status: replace the process-global correlation counter with per-scope registration; ops completing while scopes are active are released by the LAST overlapping scope's Dispose (no cross-caller release, no "owner MUST call" contract); ReleaseRetainedTypesForTerminalOperations removed; Types XML-documented as self-emptying post-terminal (CuratedTypes is the post-terminal accessor); DetachExceptionGraphs skips reallocation when everything is already detached - observability: every sweep logs a Debug line with the removed count/ALC name - tests: Given_PagePool (TTL, drop-on-disable/re-enable, per-pool isolation, sweep keeps default keys), Given_CompositionTargetHandlerSweep (scoped/all paths, collectible-method-behind-default-target, static null target), dispatcher tests assert handler-not-retained via WeakReference + reentrancy + throw resilience, Given_WindowId_Maps_Alc try/finally + collectibility assertion, Given_ResidualTypeStatics_Alc covers dying-swept/sibling-kept, Given_HotReloadClientOperation_Alc fails loud on desktop collectible-ALC load regressions and covers overlapping scopes - spec 048: verification table corrected to match actual coverage; WASM CI green stated as a merge precondition Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (9)
src/Uno.UI/UI/Xaml/UIElement.wasm.cs:686
- After removing
using System.Runtime.Loader;, the XML doc cref should be fully qualified so it still resolves and doesn’t produce XML-doc warnings (which may be treated as errors).
/// <summary>
/// Removes <see cref="UIElementNativeRegistrar"/> entries whose key <see cref="Type"/> belongs
/// to a non-default (collectible) <see cref="AssemblyLoadContext"/>. A downstream host that
/// loads previewed apps into their own collectible AssemblyLoadContexts creates elements of the
/// app's control types; the process-lifetime registrar then keeps each such <see cref="Type"/>
src/Uno.UI/UI/Xaml/Window/Window.alc.cs:438
- There are two consecutive XML
<summary>blocks immediately beforeAlcWindowCleanupCallback, so the first (aboutCloseAlcWindows) is currently attached to the property, and the actualCloseAlcWindows()method ends up undocumented. This also produces duplicate<summary>elements for the property XML docs. Please move theCloseAlcWindowssummary to the method and keep only the callback summary on the property.
/// <summary>
/// Closes all <see cref="Window"/> instances that belong to a non-default (secondary) ALC.
/// This replaces reflection-based window cleanup with a proper internal API.
/// </summary>
/// <summary>
src/Uno.UI/UI/Xaml/PagePool.cs:300
- After removing
using System.Runtime.Loader;, fully-qualify the XML doc cref forAssemblyLoadContextto avoid unresolved-cref XML-doc warnings.
/// <summary>
/// Removes, from every live pool, the pooled pages whose page <see cref="Type"/> belongs to a
/// non-default (collectible) <see cref="AssemblyLoadContext"/>. A downstream host that loads
/// previewed apps into their own collectible AssemblyLoadContexts navigates the app's pages;
/// pooled instances (and the <see cref="Type"/> keys) then keep the app's context alive after
src/Uno.UI/UI/Xaml/Style/Style.cs:64
- Spelling: log message says "entrie(s)" (missing "es").
if (removed > 0 && _logger.IsEnabled(LogLevel.Debug))
{
_logger.Debug($"[ALC-CLEANUP] UseUWPDefaultStylesOverride: removed {removed} entrie(s) owned by dying ALC '{dyingAlc?.Name ?? "unload-initiated"}'.");
}
src/Uno.UI/UI/Xaml/UIElement.wasm.cs:8
using System.Runtime.Loader;is unused in this file (the only references toAssemblyLoadContextare in XML docs). In this repo, unnecessary usings can surface as CS8019 and fail the build under TreatWarningsAsErrors. Remove the using and fully-qualify the XML doc cref instead (see next comment).
This issue also appears on line 682 of the same file.
using System.Linq;
using System.Numerics;
using System.Runtime.InteropServices;
using System.Runtime.Loader;
using Windows.Foundation;
src/Uno.UI/UI/Xaml/PagePool.cs:6
using System.Runtime.Loader;is unused in this file (only referenced in XML docs). This can trigger CS8019 under TreatWarningsAsErrors. Remove it and fully-qualify the doc cref instead (see next comment).
This issue also appears on line 296 of the same file.
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.Loader;
using System.Threading.Tasks;
using Uno.Extensions;
src/Uno.UI/UI/Xaml/Style/Style.cs:44
- Spelling: log message says "entrie(s)" (missing "es"). This is user-visible in debug logs and makes grep/diagnostics harder.
This issue also appears on line 61 of the same file.
if (removed > 0 && _logger.IsEnabled(LogLevel.Debug))
{
_logger.Debug($"[ALC-CLEANUP] Style caches: removed {removed} non-default-ALC entrie(s).");
}
src/Uno.UI/UI/Xaml/HtmlElementHelper.wasm.cs:59
- Spelling: log message says "entrie(s)" (missing "es").
if (removed > 0 && typeof(HtmlElementHelper).Log().IsEnabled(Uno.Foundation.Logging.LogLevel.Debug))
{
typeof(HtmlElementHelper).Log().Debug($"[ALC-CLEANUP] HtmlElementHelper: removed {removed} non-default-ALC tag cache entrie(s).");
}
src/Uno.UWP/ApplicationModel/Resources/ResourceLoader.static.cs:181
- Spelling: log message says "assemblie(s)" (missing "es").
if (_log.IsEnabled(LogLevel.Debug))
{
_log.LogDebug($"[ALC-CLEANUP] ResourceLoader: removed {removed.Count} lookup assemblie(s) and {removedMarkers} parsed-resource marker(s).");
}
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23707/wasm-skia-net9/index.html |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23707/docs/index.html |
|
The build 225162 found UI Test snapshots differences: Details
|
|
|
… rooted path - on Android (Mono) Assembly.Location for bundled assemblies is a bare, non-rooted file name; LoadFromAssemblyPath rejects it with ArgumentException, which escaped the load-failure filter and failed the Android Skia shards - gate the collectible-ALC load on Path.IsPathFullyQualified so non-rooted locations take the documented ordinary-type fallback, same as an empty one - desktop keeps the fail-loud behaviour for rooted paths that fail to load Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- hot reload Status: TypeCorrelationScope.Dispose decrements all retained ops then selects the released ones via an explicit Where; DetachExceptionGraphs needs-detach probe rewritten as a single Any predicate - ResourceLoader: ClearAlcAssembliesCore filters _lookupAssemblies with an explicit Where while keeping the lazy HashSet allocation - CompositionTargetFrameDispatcher: per-handler catch now uses an exception filter so runtime-fatal exceptions (OOM/SO/AV) propagate instead of being swallowed with the frame loop kept alive in a corrupted process Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/Uno.UI/UI/Xaml/Style/Style.cs:63
- Typo in log message: "entrie(s)" should be "entries".
_logger.Debug($"[ALC-CLEANUP] UseUWPDefaultStylesOverride: removed {removed} entrie(s) owned by dying ALC '{dyingAlc?.Name ?? "unload-initiated"}'.");
src/Uno.UI/UI/Xaml/Media/CompositionTarget.wasm.cs:72
- ClearAlcHandlersCore is called with scope: null (global sweep semantics) and uses scope?.Name, but the parameter type is non-nullable AssemblyLoadContext. Make the parameter nullable to match the intended contract and avoid nullability warnings/errors when building with nullable enabled.
private static void ClearAlcHandlersCore(AssemblyLoadContext scope)
src/Uno.UI/UI/Xaml/Style/Style.cs:44
- Typo in log message: "entrie(s)" should be "entries".
This issue also appears on line 63 of the same file.
_logger.Debug($"[ALC-CLEANUP] Style caches: removed {removed} non-default-ALC entrie(s).");
src/Uno.UI/UI/Xaml/Application.Alc.cs:386
- CleanupNonDefaultAlcCachesCore is invoked with dyingAlc: null from CleanupAllSecondaryAlcCaches, but its parameter type is non-nullable AssemblyLoadContext. Make the parameter nullable to reflect the sentinel-null semantics used throughout the method (e.g. dyingAlc is null checks, dyingAlc?.Name).
private static void CleanupNonDefaultAlcCachesCore(AssemblyLoadContext dyingAlc)
src/Uno.UI/UI/Xaml/HtmlElementHelper.wasm.cs:59
- Typo in log message: "entrie(s)" should be "entries".
typeof(HtmlElementHelper).Log().Debug($"[ALC-CLEANUP] HtmlElementHelper: removed {removed} non-default-ALC tag cache entrie(s).");
src/Uno.UWP/ApplicationModel/Resources/ResourceLoader.static.cs:178
- Typo in log message: "assemblie(s)" should be "assemblies".
_log.LogDebug($"[ALC-CLEANUP] ResourceLoader: removed {removed.Count} lookup assemblie(s) and {removedMarkers} parsed-resource marker(s).");
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23707/docs/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23707/wasm-skia-net9/index.html |
…esults XML produced)
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23707/wasm-skia-net9/index.html |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23707/docs/index.html |
|
The build 225193 found UI Test snapshots differences: Details
|
Summary
Batch 1 of managed sweeps that release collectible-
AssemblyLoadContextpins for a downstream host that loads previewed apps into their own collectible AssemblyLoadContexts and unloads them on reload. On WebAssemblyAssemblyLoadContext.Unloadingis never raised, so process-lifetime static caches that key on (or reference) an app'sAssembly/Type/delegate keep the context alive afterUnload(). Each cache is swept from the existing ALC cleanup hook (Application.CleanupNonDefaultAlcCaches, invoked fromWindow.CloseAlcWindow) or exposes an ALC-scoped removal called from a reachable teardown path — matching the establishedStyle.ClearCachesForNonDefaultAlc/RemoveNonDefaultAlcEntriespattern.Closes #23706
Sweeps landed
ResourceLoader—ClearNonDefaultAlcAssemblies()drops previewed-app lookup assemblies (and their parsed-resource markers) from the process-lifetime_lookupAssemblieslist; wired into the cleanup hook.UIElementNativeRegistrar._classNames(WASM) —ClearNonDefaultAlcEntries()drops the collectible-ALCTypekeys of the element-type→registration-id map (invisible to a desktop-only guard). The JS-side registration map holds only strings (no ALC pin) and is intentionally left intact — its ids come from a shared counter, so deleting entries there could collide with live registrations.AppWindow/ApplicationView/CoreDragDropManagereach gainDestroyForWindowId, mirroringDisplayInformation.DestroyForWindowId, called fromWindow.CloseAlcWindow. These retained the closed window's instance and its event subscribers.CompositionTarget.Rendering(WASM) —ClearNonDefaultAlcHandlers()drops non-default-ALC subscribers from the static handler list; the per-frame dispatch now reuses a snapshot buffer instead of allocating a freshListevery frame.HotReloadClientOperationreleases its rawType[](keeping the curated string list), and the local-operation history is capped to a ~100-entry ring buffer, so retained operations no longer pin every hot-reloaded collectible type.PagePool(WASM/managed Frame) — collapsed to a single lazily-created process-wide instance (eachFramepreviously overwrote a shared static with a new pool, orphaning the old one while its eternal 30s scavenger kept it alive); the scavenger is scheduled at most once and only when pooling is enabled;ClearNonDefaultAlcEntries()drops collectible page-type keys.Type-keyed statics —HtmlElementHelper._cache(WASM) andFeatureConfiguration.Style.UseUWPDefaultStylesOverridegain a collectible-ALC key sweep.Skipped (verified not an ALC pin)
ImageBrush._naturalSizeCache: keyed by data-URI/URL strings, which carry no ALC identity — a memory-bloat concern, not a pin. Out of scope here; noted as a follow-up (hash/LRU the data-URI keys).UnicodeText.ICU._lookupCache: keyed by native-delegate types declared inside the framework (default ALC); app types are never keys. Left unchanged.Out of scope (follow-ups)
Hot-reload delta pipeline retention; image Blob URL lifetime;
FontFamilyLoadercaches.Tests (red/green)
Each sweep is guarded by a test asserting a collectible-ALC-keyed entry is dropped while default-ALC / framework entries are kept:
Given_ResourceLoader_Alc(unit) — proven red by neutering the sweep (fails at the removal assertion), green with it.Given_WindowId_Maps_Alc(unit)Given_ResidualTypeStatics_Alc(unit)Given_HotReloadClientOperation_Alc(runtime, HAS_UNO_WINUI)Notes
Local environment is flaky (package churn / Cecil locks); build+test were run where possible and CI validates the rest. See
specs/048-wasm-alc-pin-sweeps/spec.md.🤖 Generated with Claude Code