From 84a7839a298fe359c4cf37a6480e7886f9225f19 Mon Sep 17 00:00:00 2001 From: Christoph Wille Date: Sun, 16 Aug 2026 09:52:21 +0200 Subject: [PATCH] Withdraw a window's Help items from the macOS app menu, and add a retention canary The app-level NativeMenu declared in App.axaml lives as long as the process, while every MainWindow builds its own Help items over its own command instances (AboutCommand reaches the DockWorkspace and, through it, the whole app graph). PromoteHelpToMacAppMenu inserted each window's items without taking the previous window's out and nothing removed them on close, so on macOS the headless suite kept every test's app graph alive - the same 13 MB per test as the anchors fixed earlier on this branch, and the reason the memory win did not reproduce on macOS (retained gen2 still climbing to ~4 GB there while a Windows run peaks at 0.7 GB). Forcing the macOS path on Windows reproduces the growth (14.3 GB peak private bytes over the suite); withdrawn, it is 0.7 GB. Three smaller anchors of the same kind, found while making the canary below hold in the full suite: RichNodeText and AnalyzerTreeNode cached the first container's exports in statics, which subscribed later windows to a stale settings object, handed later analyzers the first test's assembly list, and kept the first app graph reachable for the run; and a search still in flight when its container went away kept its drain timer and IsSearching - hence the pane's indeterminate progress animation on the render clock - alive, retaining every window a search test closed mid-run (about 30 of them, ~400 MB). The canary test closes a MainWindow the way the per-test teardown does and waits for it to become collectable. It fails on any single anchor being restored (checked by leaving DetachFlyouts out), which is the regression guard the individual anchor fixes lacked; the teardown body is exposed as TearDownTestState so the test performs exactly what AfterTest does. Assisted-by: Claude:claude-fable-5:Claude Code --- ILSpy.Tests/MainWindow/MainMenuTests.cs | 32 +++++++++ ILSpy.Tests/ResetAppStateAttribute.cs | 7 ++ ILSpy.Tests/TeardownRetentionTests.cs | 91 +++++++++++++++++++++++++ ILSpy/Analyzers/AnalyzerTreeNode.cs | 13 ++-- ILSpy/Controls/TreeView/RichNodeText.cs | 6 +- ILSpy/Search/SearchPaneModel.cs | 12 +++- ILSpy/Views/MainMenu.axaml.cs | 25 ++++++- 7 files changed, 174 insertions(+), 12 deletions(-) create mode 100644 ILSpy.Tests/TeardownRetentionTests.cs diff --git a/ILSpy.Tests/MainWindow/MainMenuTests.cs b/ILSpy.Tests/MainWindow/MainMenuTests.cs index 009516885..50ce4f43c 100644 --- a/ILSpy.Tests/MainWindow/MainMenuTests.cs +++ b/ILSpy.Tests/MainWindow/MainMenuTests.cs @@ -95,6 +95,38 @@ public class MainMenuTests openItem.Gesture!.Should().Be(expected); } + // The app-level NativeMenu (App.axaml) is process-wide, while every MainWindow builds + // its own Help items over its own command instances. On macOS each new window promotes + // them into that app menu; the ones an earlier window promoted must be replaced, not + // kept - otherwise the app menu pins every earlier window's command graph (and, in the + // headless suite, every test's app graph) for the life of the process. + [AvaloniaTest] + public void Promoting_Help_Again_Replaces_The_Items_An_Earlier_Window_Promoted() + { + var appMenu = NativeMenu.GetMenu(Application.Current!); + appMenu.Should().NotBeNull("App.axaml declares the NativeMenu the Help items move into"); + + static (NativeMenu Root, System.Collections.Generic.Dictionary ByTag) WindowMenuWithHelp(string header) + { + var help = new NativeMenuItem { Header = "_Help", Menu = new NativeMenu() }; + help.Menu.Items.Add(new NativeMenuItem { Header = header }); + var root = new NativeMenu(); + root.Items.Add(help); + return (root, new System.Collections.Generic.Dictionary(StringComparer.Ordinal) { ["_Help"] = help }); + } + + var first = WindowMenuWithHelp("About (first window)"); + MainMenu.PromoteHelpToMacAppMenu(first.Root, first.ByTag); + var afterFirst = appMenu!.Items.Count; + var second = WindowMenuWithHelp("About (second window)"); + MainMenu.PromoteHelpToMacAppMenu(second.Root, second.ByTag); + + appMenu.Items.Count.Should().Be(afterFirst, "the second window's Help items replace the first window's"); + appMenu.Items.OfType().Select(i => i.Header) + .Should().Contain("About (second window)") + .And.NotContain("About (first window)"); + } + // NativeMenuItem.Gesture is display-only when NativeMenuBar renders the menu inline // (the managed fallback binds it to MenuItem.InputGesture, which never handles input), // so every menu gesture must also be registered as a window-level KeyBinding or the diff --git a/ILSpy.Tests/ResetAppStateAttribute.cs b/ILSpy.Tests/ResetAppStateAttribute.cs index c4d3ebc73..de957195a 100644 --- a/ILSpy.Tests/ResetAppStateAttribute.cs +++ b/ILSpy.Tests/ResetAppStateAttribute.cs @@ -94,6 +94,13 @@ public sealed class ResetAppStateAttribute : Attribute, ITestAction if (Application.Current == null || !Dispatcher.UIThread.CheckAccess()) return; + TearDownTestState(); + } + + // Everything the per-test teardown does on the dispatcher thread; exposed so a test can + // perform the teardown itself and check what it leaves behind (see TeardownRetentionTests). + internal static void TearDownTestState() + { // Drive background work to quiescence BEFORE the next test rebuilds the composition. A test // that triggers a decompile spawns a Task.Run plus dispatcher continuations and rarely awaits // them to completion; left running, that continuation lands during the next test and reads diff --git a/ILSpy.Tests/TeardownRetentionTests.cs b/ILSpy.Tests/TeardownRetentionTests.cs new file mode 100644 index 000000000..d273afb5c --- /dev/null +++ b/ILSpy.Tests/TeardownRetentionTests.cs @@ -0,0 +1,91 @@ +// Copyright (c) 2026 Christoph Wille +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Threading.Tasks; + +using Avalonia.Headless; +using Avalonia.Headless.NUnit; +using Avalonia.Threading; + +using ICSharpCode.ILSpy.AppEnv; +using ICSharpCode.ILSpy.AssemblyTree; +using ICSharpCode.ILSpy.Views; + +using NUnit.Framework; + +namespace ICSharpCode.ILSpy.Tests; + +// Most tests in this suite show a MainWindow, and the per-test teardown closes it and rebuilds +// the composition container. Anything that still reaches a closed window - a static event, a +// shared XAML resource with a subscriber, an animation on the render clock, the app-level menu - +// keeps that test's whole app graph (view-models, tree, loaded assemblies; about 13 MB) alive +// for the rest of the run, and over the suite that is enough to push a 16 GB CI runner into +// paging. Rather than asserting the absence of each known anchor, this test performs the +// teardown itself and checks that the window is actually collectable afterwards. +[TestFixture] +public class TeardownRetentionTests +{ + [AvaloniaTest] + public async Task A_Main_Window_Closed_By_The_Teardown_Is_Collectable() + { + var window = ShowMainWindow(); + // Let the assembly loads the window started run to completion first: each one posts its + // completion to the dispatcher, and one posted after the teardown would hold the tree (and + // with it the window) until the next test pumps it - a false positive, not retention. + await Waiters.WaitForAsync(static () => AllAssembliesLoaded()); + + ResetAppStateAttribute.TearDownTestState(); + // What the next test's BeforeTest does: the fresh container drops the [Shared] MainWindow. + AppComposition.CreateContainer(); + + // The closed window's final composition batch (its target's disposal) references it until + // the compositor has committed and rendered it, and commits are throttled behind the + // previous batch's completion, which comes back through the thread pool - so keep pumping + // the dispatcher (and the headless render loop, which only ticks on request) while polling. + await Waiters.WaitForAsync(() => IsCollected(window), TimeSpan.FromSeconds(10), + "the closed MainWindow to become unreachable once its container is gone"); + } + + static bool IsCollected(WeakReference window) + { + AvaloniaHeadlessPlatform.ForceRenderTimerTick(); + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + return !window.IsAlive; + } + + static bool AllAssembliesLoaded() + { + var assemblies = AppComposition.Current.GetExport().AssemblyList?.GetAssemblies(); + return assemblies is { Length: > 0 } && assemblies.All(a => a.IsLoaded); + } + + // The window must not be referenced from this test's own frame while the GC runs. + [MethodImpl(MethodImplOptions.NoInlining)] + static WeakReference ShowMainWindow() + { + var window = AppComposition.Current.GetExport(); + window.Show(); + Dispatcher.UIThread.RunJobs(); + return new WeakReference(window); + } +} diff --git a/ILSpy/Analyzers/AnalyzerTreeNode.cs b/ILSpy/Analyzers/AnalyzerTreeNode.cs index 7c30fb6a2..c6093196d 100644 --- a/ILSpy/Analyzers/AnalyzerTreeNode.cs +++ b/ILSpy/Analyzers/AnalyzerTreeNode.cs @@ -37,23 +37,24 @@ namespace ICSharpCode.ILSpy.Analyzers /// public abstract class AnalyzerTreeNode : SharpTreeNode { - static LanguageService? cachedLanguageService; - static AnalyzerRegistry? cachedRegistry; - static AssemblyTreeModel? cachedAssemblyTreeModel; + // The exports below are resolved on every access rather than cached in statics: the + // composition root is rebuilt per test in the headless suite, and a static cache would + // hand later tests the first test's language service and assembly list and keep that + // first app graph reachable for the run. A warm GetExport is a dictionary lookup. /// /// The active language used to format entity text. Resolved lazily through the /// composition host so design-time previews (no MEF) don't NRE during XAML reload. /// protected static Languages.Language Language - => (cachedLanguageService ??= AppComposition.Current.GetExport()).CurrentLanguage; + => AppComposition.Current.GetExport().CurrentLanguage; /// /// The active backing the assembly tree. Search nodes pass /// it into AnalyzerContext so each analyser can iterate the loaded modules. /// protected static AssemblyList? CurrentAssemblyList - => (cachedAssemblyTreeModel ??= AppComposition.Current.GetExport()).AssemblyList; + => AppComposition.Current.GetExport().AssemblyList; /// /// All MEF-registered exports, ordered by their declared @@ -63,7 +64,7 @@ namespace ICSharpCode.ILSpy.Analyzers /// returns true for the wrapped entity. /// public static IReadOnlyList> Analyzers - => (cachedRegistry ??= AppComposition.Current.GetExport()).Analyzers; + => AppComposition.Current.GetExport().Analyzers; public override bool CanDelete() => Parent is { IsRoot: true }; diff --git a/ILSpy/Controls/TreeView/RichNodeText.cs b/ILSpy/Controls/TreeView/RichNodeText.cs index 83bfd75d7..26cab85c9 100644 --- a/ILSpy/Controls/TreeView/RichNodeText.cs +++ b/ILSpy/Controls/TreeView/RichNodeText.cs @@ -58,9 +58,11 @@ namespace ICSharpCode.ILSpy.Controls.TreeView static readonly AttachedProperty CleanupHookedProperty = AvaloniaProperty.RegisterAttached("CleanupHooked", typeof(RichNodeText)); - static LanguageSettings? languageSettings; + // Resolved on every call rather than cached: the composition root is rebuilt per test in the + // headless suite, and a static cache would both subscribe later windows to a stale settings + // object and keep the first window's tree reachable through it. A warm export lookup is cheap. static LanguageSettings? GetLanguageSettings() - => languageSettings ??= AppComposition.TryGetExport()?.SessionSettings.LanguageSettings; + => AppComposition.TryGetExport()?.SessionSettings.LanguageSettings; static RichNodeText() { diff --git a/ILSpy/Search/SearchPaneModel.cs b/ILSpy/Search/SearchPaneModel.cs index 56aca2131..5e96f5d30 100644 --- a/ILSpy/Search/SearchPaneModel.cs +++ b/ILSpy/Search/SearchPaneModel.cs @@ -53,7 +53,7 @@ namespace ICSharpCode.ILSpy.Search [Export] [ExportToolPane(ContentId = PaneContentId, Alignment = ToolPaneAlignment.Top, Order = 0, IsVisibleByDefault = false)] [Shared] - public partial class SearchPaneModel : ToolPaneModel + public sealed partial class SearchPaneModel : ToolPaneModel, IDisposable { public const string PaneContentId = "Search"; @@ -265,6 +265,16 @@ namespace ICSharpCode.ILSpy.Search run.Start(); } + // The composition container is the only owner and disposes this model with itself. A search + // still in flight at that point owns a dispatcher timer and keeps IsSearching (and with it the + // pane's indeterminate progress animation) on; both would otherwise outlive the container. + public void Dispose() + { + currentSearch?.Cancel(); + currentSearch = null; + IsSearching = false; + } + void OnRunCompleted(RunningSearch sender) { // Ignore late completions from cancelled runs — those are noise. diff --git a/ILSpy/Views/MainMenu.axaml.cs b/ILSpy/Views/MainMenu.axaml.cs index 1c6786c0b..68aeca559 100644 --- a/ILSpy/Views/MainMenu.axaml.cs +++ b/ILSpy/Views/MainMenu.axaml.cs @@ -74,12 +74,16 @@ public static class MainMenu { TranslateGesturesForMacOS(menu); PromoteHelpToMacAppMenu(menu, topLevelByTag); + window.Closed += (_, _) => WithdrawPromotedHelpItems(); } RegisterGestureKeyBindings(window, menu); NativeMenu.SetMenu(window, menu); } + // The Help items currently promoted into the app-level NativeMenu (see PromoteHelpToMacAppMenu). + static readonly List promotedHelpItems = new(); + // NativeMenuItem.Gesture is display-only when NativeMenuBar renders the menu inline: // the managed fallback binds it to MenuItem.InputGesture, which never handles input. // So each gesture is registered as a window-level KeyBinding too - that is what makes @@ -121,25 +125,40 @@ public static class MainMenu // the exporter subscribes to that instance's Items, so inserting fires a re-export. // Items go at the top, above the Services / Hide / Quit block the exporter appended. // We then remove _Help from the window menu so the items don't appear in both places. - static void PromoteHelpToMacAppMenu(NativeMenu rootMenu, Dictionary topLevelByTag) + // The app menu outlives any one window, and each window's Help items are built over + // that window's own command instances, so the items an earlier window promoted are + // taken out first (and again when the window closes); leaving them in would keep + // that window's command graph, and everything those commands reach, alive for the + // life of the process. + internal static void PromoteHelpToMacAppMenu(NativeMenu rootMenu, Dictionary topLevelByTag) { - if (!topLevelByTag.TryGetValue("_Help", out var helpItem) || helpItem.Menu is null) - return; + WithdrawPromotedHelpItems(); if (Application.Current is null) return; var appMenu = NativeMenu.GetMenu(Application.Current); if (appMenu is null) return; + if (!topLevelByTag.TryGetValue("_Help", out var helpItem) || helpItem.Menu is null) + return; var index = 0; foreach (var item in helpItem.Menu.Items.ToArray()) { helpItem.Menu.Items.Remove(item); appMenu.Items.Insert(index++, item); + promotedHelpItems.Add(item); } rootMenu.Items.Remove(helpItem); topLevelByTag.Remove("_Help"); } + static void WithdrawPromotedHelpItems() + { + var appMenu = Application.Current is null ? null : NativeMenu.GetMenu(Application.Current); + foreach (var item in promotedHelpItems) + appMenu?.Items.Remove(item); + promotedHelpItems.Clear(); + } + static bool TryGetExports( out SettingsService settings, out MainMenuCommandRegistry registry,