Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions ILSpy.Tests/MainWindow/MainMenuTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// DEALINGS IN THE SOFTWARE.

using System;
using System.Collections.Generic;
using System.Linq;

using Avalonia;
Expand Down Expand Up @@ -95,6 +96,83 @@ public void File_Open_Carries_The_Ctrl_O_Gesture()
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");

MainMenu.PromoteHelpToMacAppMenu(
WindowMenuWithHelpItems("About (first window)", out var firstByTag), firstByTag);
var afterFirst = appMenu!.Items.Count;
var promoted = MainMenu.PromoteHelpToMacAppMenu(
WindowMenuWithHelpItems("About (second window)", out var secondByTag), secondByTag);
try
{
appMenu.Items.Count.Should().Be(afterFirst, "the second window's Help items replace the first window's");
appMenu.Items.OfType<NativeMenuItem>().Select(i => i.Header)
.Should().Contain("About (second window)")
.And.NotContain("About (first window)");
}
finally
{
RestoreAppMenu(appMenu, promoted);
}
}

// The Help items a window promotes are withdrawn when it closes, but only that window's own:
// a window closing after a second one has promoted its items must leave those in the app menu,
// or macOS shows an app menu with no About / Check for Updates while the second window is still
// on screen and nothing ever puts them back.
[AvaloniaTest]
public void Closing_An_Earlier_Window_Leaves_A_Later_Window_Help_Items_In_Place()
{
var appMenu = NativeMenu.GetMenu(Application.Current!);
appMenu.Should().NotBeNull("App.axaml declares the NativeMenu the Help items move into");

var first = MainMenu.PromoteHelpToMacAppMenu(
WindowMenuWithHelpItems("About (first window)", out var firstByTag), firstByTag);
var second = MainMenu.PromoteHelpToMacAppMenu(
WindowMenuWithHelpItems("About (second window)", out var secondByTag), secondByTag);
try
{
// What the first window's Closed handler does, now that the second window has promoted.
MainMenu.WithdrawHelpItems(first);

appMenu!.Items.OfType<NativeMenuItem>().Select(i => i.Header)
.Should().Contain("About (second window)",
"the still-open window's Help items must survive an earlier window closing");
}
finally
{
RestoreAppMenu(appMenu!, second);
}
}

// The app menu is declared on Application and outlives every test, so a test that promotes
// placeholder items into it has to take them back out; otherwise a later test reading it
// (see MainMenu_top_level_items_are_File_View_Window_in_order) sees this test's leftovers.
static void RestoreAppMenu(NativeMenu appMenu, List<NativeMenuItemBase> promoted)
{
foreach (var item in promoted)
appMenu.Items.Remove(item);
}

static NativeMenu WindowMenuWithHelpItems(string header, out Dictionary<string, NativeMenuItem> byTag)
{
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);
byTag = new Dictionary<string, NativeMenuItem>(StringComparer.Ordinal) { ["_Help"] = help };
return root;
}

// Avalonia's macOS NativeMenu bridge maps NativeMenuItem to NSMenuItem and sets
// NSMenuItem.action ONLY when Command != null. Without it, NSMenuValidation marks
// the item disabled (greyed out) and no click ever reaches managed code - which
Expand Down
54 changes: 49 additions & 5 deletions ILSpy.Tests/ResetAppStateAttribute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,16 @@
// DEALINGS IN THE SOFTWARE.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

using Avalonia;
using Avalonia.Controls.ApplicationLifetimes;
using Avalonia.Controls;
using Avalonia.Threading;
using Avalonia.VisualTree;

using ICSharpCode.ILSpyX.Settings;

Expand Down Expand Up @@ -92,6 +94,13 @@ public void AfterTest(ITest test)
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
Expand All @@ -100,15 +109,50 @@ public void AfterTest(ITest test)
DrainPendingWork();

// Close any windows the test showed so their view-models (alive and weakly subscribed to
// MessageBus) can't react to events raised by later tests, then drain once more.
if (Application.Current.ApplicationLifetime is IClassicDesktopStyleApplicationLifetime desktop)
// MessageBus) can't react to events raised by later tests, then drain once more. A window
// left open also outlives its container: the compositor keeps every open top level
// reachable, and with it the view-models, the assembly tree and the loaded assemblies -
// about 13 MB per test, which over the suite is what pushed the CI runner into paging.
foreach (var window in openWindows.ToArray())
{
foreach (var window in desktop.Windows.ToArray())
window.Close();
DetachFlyouts(window);
window.Close();
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two robustness gaps in this loop.

Nothing closes an open menu before the window dies. DetachFlyouts covers Button.Flyout, but a ContextMenu left open by a test keeps DefaultMenuInteractionHandler subscribed to the process-global InputManager.Instance.Process, and that handler holds Menu.TopLevel - i.e. this window and everything under it - for the rest of the run. Several tests here open a tree context menu and never dismiss it. Closing menus in the same pass is a one-line extension and removes a retention root of the same size as the flyout one:

foreach (var control in window.GetVisualDescendants().OfType<Control>())
{
    control.ContextMenu?.Close();
    if (control is Button { Flyout: not null } button)
        button.Flyout = null;
}

No error containment. If any Closing/Closed handler throws for the first window (MainWindow.OnClosing guards its own SaveLayout, but view OnDetachedFromVisualTree handlers are not guarded), the foreach aborts: every remaining window stays open and stays in the static openWindows list for the whole run - the exact retention this PR removes - and the exception surfaces as a teardown error on a test that otherwise passed. Wrap the body in try/catch and openWindows.Remove(window) unconditionally so the tracking list can never become the thing that retains a window.

Dispatcher.UIThread.RunJobs();
}

// Avalonia's Button subscribes to its flyout's Opened/Closed events when its template is
// applied and unsubscribes only when the Flyout property changes, not when the button leaves
// the tree. Dock's ToolChromeControl theme gives every tool pane's chrome button the same
// MenuFlyout resource, so that one shared flyout would keep the visual tree of every window
// this suite ever showed alive. Clearing the property before the window closes is what
// makes the button let go.
static void DetachFlyouts(Window window)
{
foreach (var button in window.GetVisualDescendants().OfType<Button>())
{
if (button.Flyout != null)
button.Flyout = null;
}
}

// The headless host runs the app without an application lifetime, so nothing tracks the
// windows the tests show. These are the same class handlers ClassicDesktopStyleApplicationLifetime
// installs to maintain its Windows list.
static readonly List<Window> openWindows = new();

static ResetAppStateAttribute()
{
Window.WindowOpenedEvent.AddClassHandler(typeof(Window), (sender, _) => {
if (sender is Window window && !openWindows.Contains(window))
openWindows.Add(window);
});
Window.WindowClosedEvent.AddClassHandler(typeof(Window), (sender, _) => {
if (sender is Window window)
openWindows.Remove(window);
});
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the four leak fixes has a regression test, which is at odds with the repo's TDD rule for new behaviour. Concretely: if someone later drops IDisposable from DebugStepsPaneModel, reverts this tracking, or restores IsIndeterminate="True" in the XAML, the suite stays green and the runner quietly goes back to paging - the failure mode is an intermittent 60s timeout in a different project, which is the hardest possible signal to trace back here.

A single cheap guard covers most of it: hold a WeakReference to the container's MainWindow (and to the DebugStepsPaneModel) at the end of a test, let the next BeforeTest rebuild the container, force a full GC, and assert the reference is dead. DecompilerViewTests already uses the GC.Collect()/WaitForPendingFinalizers() pattern, so there is precedent.


static void DrainPendingWork()
{
Task quiesce;
Expand Down
17 changes: 15 additions & 2 deletions ILSpy.Tests/Search/SearchProgressTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
using Avalonia.Controls;
using Avalonia.Headless.NUnit;
using Avalonia.Media;
using Avalonia.Threading;

using AwesomeAssertions;

Expand Down Expand Up @@ -115,7 +116,19 @@ public async Task SearchPane_Hosts_A_Progress_Indicator_Bound_To_IsSearching()
var progress = pane.FindControl<ProgressBar>("SearchProgress");
((object?)progress).Should().NotBeNull(
"the pane must host a progress indicator the user can see while a search runs");
progress!.IsIndeterminate.Should().BeTrue(
"the indicator runs in indeterminate mode — we don't know the total work up front");

// Indeterminate mode is tied to the search, not switched on permanently: the indicator
// is an infinite animation, and one that ran while idle would keep the render clock
// busy for as long as the pane exists.
var search = AppComposition.Current.GetExport<SearchPaneModel>();
pane.DataContext.Should().BeSameAs(search, "the indicator binds to the pane's own model; anything else makes the assertions below meaningless");
progress!.IsIndeterminate.Should().BeFalse("nothing is running yet");

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is now vacuous in the failure case it is supposed to catch. ProgressBar.IsIndeterminate defaults to false, so if the pane's DataContext has not been set to this SearchPaneModel yet (or is a different instance than the one resolved from composition), line 124 passes for the wrong reason and line 127 fails with "the indicator runs in indeterminate mode..." - which points at the binding direction rather than at the missing DataContext.

Add pane.DataContext.Should().BeSameAs(search) before line 124, so the test fails where the real problem is. The single RunJobs() on line 126 is also load-bearing now where the old assertion read a static XAML value; a short Waiters.WaitForAsync(() => progress.IsIndeterminate) would take the timing out of it entirely.

search.IsSearching = true;
Dispatcher.UIThread.RunJobs();
progress.IsIndeterminate.Should().BeTrue(
"the indicator runs in indeterminate mode while a search is in flight - we don't know the total work up front");
search.IsSearching = false;
Dispatcher.UIThread.RunJobs();
progress.IsIndeterminate.Should().BeFalse("the animation stops with the search");
}
}
91 changes: 91 additions & 0 deletions ILSpy.Tests/TeardownRetentionTests.cs
Original file line number Diff line number Diff line change
@@ -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<AssemblyTreeModel>().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<MainWindow>();
window.Show();
Dispatcher.UIThread.RunJobs();
return new WeakReference(window);
}
}
13 changes: 7 additions & 6 deletions ILSpy/Analyzers/AnalyzerTreeNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,24 @@ namespace ICSharpCode.ILSpy.Analyzers
/// </summary>
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.

/// <summary>
/// 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.
/// </summary>
protected static Languages.Language Language
=> (cachedLanguageService ??= AppComposition.Current.GetExport<LanguageService>()).CurrentLanguage;
=> AppComposition.Current.GetExport<LanguageService>().CurrentLanguage;

/// <summary>
/// The active <see cref="AssemblyList"/> backing the assembly tree. Search nodes pass
/// it into <c>AnalyzerContext</c> so each analyser can iterate the loaded modules.
/// </summary>
protected static AssemblyList? CurrentAssemblyList
=> (cachedAssemblyTreeModel ??= AppComposition.Current.GetExport<AssemblyTreeModel>()).AssemblyList;
=> AppComposition.Current.GetExport<AssemblyTreeModel>().AssemblyList;

/// <summary>
/// All MEF-registered <see cref="IAnalyzer"/> exports, ordered by their declared
Expand All @@ -63,7 +64,7 @@ protected static AssemblyList? CurrentAssemblyList
/// <see cref="IAnalyzer.Show"/> returns true for the wrapped entity.
/// </summary>
public static IReadOnlyList<ExportFactory<IAnalyzer, AnalyzerMetadata>> Analyzers
=> (cachedRegistry ??= AppComposition.Current.GetExport<AnalyzerRegistry>()).Analyzers;
=> AppComposition.Current.GetExport<AnalyzerRegistry>().Analyzers;

public override bool CanDelete() => Parent is { IsRoot: true };

Expand Down
6 changes: 4 additions & 2 deletions ILSpy/Controls/TreeView/RichNodeText.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,11 @@ public static class RichNodeText
static readonly AttachedProperty<bool> CleanupHookedProperty =
AvaloniaProperty.RegisterAttached<TextBlock, bool>("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<SettingsService>()?.SessionSettings.LanguageSettings;
=> AppComposition.TryGetExport<SettingsService>()?.SessionSettings.LanguageSettings;

static RichNodeText()
{
Expand Down
7 changes: 5 additions & 2 deletions ILSpy/Search/SearchPane.axaml
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,12 @@
</Grid>
<!-- Indeterminate progress strip: lights up while RunningSearch is in flight. Mirrors
WPF's searchProgressBar; the height is just enough to be noticed without stealing
space from the results list. -->
space from the results list. IsIndeterminate follows the search too, not just
IsVisible: the indeterminate indicator is an infinite animation that keeps running
(and keeps the pane's visual tree alive through the render clock) for as long as
the pseudo-class is set, hidden or not. -->
<ProgressBar Grid.Row="1" Name="SearchProgress"
IsIndeterminate="True"
IsIndeterminate="{Binding IsSearching}"
IsVisible="{Binding IsSearching}"
Height="2" Margin="0,0,0,1"
BorderThickness="0"
Expand Down
12 changes: 11 additions & 1 deletion ILSpy/Search/SearchPaneModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public sealed class SearchModeEntry
[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";

Expand Down Expand Up @@ -265,6 +265,16 @@ void RestartSearch()
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.
Expand Down
Loading