diff --git a/ICSharpCode.ILSpyX/TreeView/FlatListTreeNode.cs b/ICSharpCode.ILSpyX/TreeView/FlatListTreeNode.cs index 1fa7473f2..0c0b1e0c7 100644 --- a/ICSharpCode.ILSpyX/TreeView/FlatListTreeNode.cs +++ b/ICSharpCode.ILSpyX/TreeView/FlatListTreeNode.cs @@ -39,7 +39,7 @@ namespace ICSharpCode.ILSpyX.TreeView byte height = 1; /// Length in the flat list, including children (children within the flat list). -1 = invalidated - int totalListLength = -1; + internal int totalListLength = -1; int Balance { get { return Height(right) - Height(left); } @@ -106,8 +106,16 @@ namespace ICSharpCode.ILSpyX.TreeView root.GetTotalListLength(); // ensure all list lengths are calculated Debug.Assert(index >= 0); Debug.Assert(index < root.totalListLength); + int originalIndex = index; SharpTreeNode node = root; - while (true) + // Falling out of the descent means the augmented lengths describe more visible nodes + // than the tree actually holds, which is what a structural mutation racing this walk + // leaves behind (see TreeThreadAffinity). This is a mitigation, not a fix: it turns a + // NullReferenceException with no context into a report of the state that produced it, + // and it cannot make the read correct. Note it does not detect a stale index either - + // collapsing a node restructures the flat list without changing totalListLength, so the + // assert above stays satisfied while the structure moves underneath. + while (node != null) { if (node.left != null && index < node.left.totalListLength) { @@ -128,6 +136,9 @@ namespace ICSharpCode.ILSpyX.TreeView node = node.right; } } + throw new InvalidOperationException( + $"The flat list tree ran out of nodes while looking up visible index {originalIndex}; " + + $"totalListLength is {root.totalListLength}. The tree was restructured while it was being read."); } internal static int GetVisibleIndexForNode(SharpTreeNode node) diff --git a/ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs b/ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs index 74b66cd6b..8f22c7801 100644 --- a/ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs +++ b/ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs @@ -23,6 +23,7 @@ using System.ComponentModel; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Linq; +using System.Threading; using ICSharpCode.ILSpyX.TreeView.PlatformAbstractions; @@ -161,6 +162,7 @@ namespace ICSharpCode.ILSpyX.TreeView set { if (isHidden != value) { + VerifyAccess(nameof(IsHidden)); isHidden = value; if (modelParent != null) UpdateIsVisible(modelParent.isVisible && modelParent.isExpanded, true); @@ -287,6 +289,7 @@ namespace ICSharpCode.ILSpyX.TreeView set { if (isExpanded != value) { + VerifyAccess(nameof(IsExpanded)); isExpanded = value; if (isExpanded) { @@ -363,10 +366,38 @@ namespace ICSharpCode.ILSpyX.TreeView } /// - /// Ensures the children were initialized (loads children if lazy loading is enabled) + /// Ensures the children were initialized (loads children if lazy loading is enabled). /// + /// + /// Realizing children mutates a tree that a UI may be indexing concurrently, so on a tree + /// that has an owner with a way onto it (see ) + /// this marshals itself there rather than relying on every caller to remember: the app + /// realizes children from background decompile tasks in several places, and a caller that + /// forgets corrupts the flat list. + /// + /// A call that is already on the owning thread runs inline, which is what keeps a blocking + /// invoke from deadlocking on itself and keeps nesting cheap: once the outermost call has + /// marshalled, everything triggers below it is already on the + /// owner and hops no further. + /// + /// A tree with no owner is not marshalled: building a subtree on a worker and publishing it + /// on the UI thread is a legitimate pattern, and nothing is displaying that subtree yet. + /// public void EnsureLazyChildren() { + if (!LazyLoading) + return; + SharpTreeNode? ownerNode = OwnerNode; + if (ownerNode?.ownerInvoke is { } invoke && ownerNode.owner != Thread.CurrentThread) + invoke(LoadLazyChildren); + else + LoadLazyChildren(); + } + + void LoadLazyChildren() + { + // Re-checked after marshalling: the owning thread may have realized the children while + // the calling thread was queued behind it. if (LazyLoading) { LazyLoading = false; diff --git a/ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs b/ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs index 63b03e778..a34e22915 100644 --- a/ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs +++ b/ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs @@ -45,6 +45,11 @@ namespace ICSharpCode.ILSpyX.TreeView void OnCollectionChanged(NotifyCollectionChangedEventArgs e) { + // Checked here rather than in each mutator: every one of them funnels through this method, + // and this is the last point before OnChildrenChanged rewrites the flat-list tree and + // notifies the flattener. A violation therefore leaves the flat list untouched instead of + // half-updated; only the backing List has already moved. + parent.VerifyChildrenChange(e); Debug.Assert(!isRaisingEvent); isRaisingEvent = true; try diff --git a/ICSharpCode.ILSpyX/TreeView/TreeThreadAffinity.cs b/ICSharpCode.ILSpyX/TreeView/TreeThreadAffinity.cs new file mode 100644 index 000000000..2682a0bcf --- /dev/null +++ b/ICSharpCode.ILSpyX/TreeView/TreeThreadAffinity.cs @@ -0,0 +1,298 @@ +// Copyright (c) 2026 Siegfried Pammer +// +// 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.Collections.Concurrent; +using System.Collections.Generic; +using System.Collections.Specialized; +using System.Diagnostics; +using System.IO; +using System.Linq; +using System.Text; +using System.Threading; + +namespace ICSharpCode.ILSpyX.TreeView +{ + // Thread affinity of the tree model. + // + // The model is not thread-safe. TreeFlattener.Count and SharpTreeNode.GetNodeByVisibleIndex + // both read the augmented 'totalListLength' fields, so a structural mutation racing a read can + // hand out an index for a node that no longer exists; issue #3290 is a NullReferenceException + // inside GetNodeByVisibleIndex that is unreachable single-threaded. The rule is that a tree + // displayed by a UI is mutated only from that UI's thread, but ICSharpCode.ILSpyX is + // host-agnostic and must not name a dispatcher, so ownership is stated explicitly instead: + // a host calls SetOwner() on the root of a tree it takes over, passing the thread and the way + // to get onto it. + // + // Ownership carries the rule two ways. EnsureLazyChildren uses the invoke delegate to move + // itself onto the owning thread, so a caller cannot get it wrong; and, in debug builds, every + // other structural mutation is verified against the owning thread so a call site that bypasses + // the rule is named instead of silently corrupting the flat list. + partial class SharpTreeNode + { + Thread? owner; + Action? ownerInvoke; + + /// + /// The nearest node on the model-parent chain that carries an explicit owner, or null when + /// nothing on the chain has been claimed. + /// + /// + /// Resolving the owner by walking up instead of stamping every node gives the propagation + /// rules for free: + /// + /// Ownership covers the whole subtree below the node it was set on, so a single call on + /// the root of a displayed tree protects everything in it. + /// Children added later inherit it, with no bookkeeping at insertion time. + /// A subtree built on a background thread carries no owner and is therefore unchecked + /// while it is being built. It inherits the owner the instant it is attached below an owned + /// parent - and that attachment is itself a mutation of the owned tree, so it is checked. + /// + /// + SharpTreeNode? OwnerNode { + get { + for (SharpTreeNode? node = this; node != null; node = node.modelParent) + { + if (node.owner != null) + return node; + } + return null; + } + } + + Thread? EffectiveOwner => OwnerNode?.owner; + + /// + /// Declares as the only thread allowed to structurally mutate this + /// node and its subtree. + /// + /// + /// Runs an action on and blocks until it has completed - the host's + /// dispatcher invoke. Supplying it lets marshal itself + /// instead of every caller having to know it must. Null leaves the tree unmarshalled, which + /// only makes sense for a tree no UI is displaying. + /// + /// + /// Re-owning is allowed - handing a tree over is exactly what this exists for - but the + /// handoff must be performed by the thread that currently owns the tree, because a + /// background thread taking ownership of a live tree away from the UI is the very race the + /// affinity check hunts for. + /// + public void SetOwner(Thread owner, Action? invoke = null) + { + VerifyAccess(nameof(SetOwner)); + this.owner = owner; + this.ownerInvoke = invoke; + } + + /// + /// Declares the calling thread as the only thread allowed to structurally mutate this node + /// and its subtree, without a way to marshal onto it. + /// + public void SetOwner() => SetOwner(Thread.CurrentThread); + + /// + /// Reports a violation if the calling thread is not the effective owner of this node. + /// + [Conditional("DEBUG")] + internal void VerifyAccess(string operation) + { +#if DEBUG + Thread? expected = EffectiveOwner; + if (expected == null || expected == Thread.CurrentThread) + return; + TreeThreadAffinity.Report(this, operation, expected); +#endif + } + + /// + /// Verifies a pending change to this node's collection. + /// + [Conditional("DEBUG")] + internal void VerifyChildrenChange(NotifyCollectionChangedEventArgs e) + { +#if DEBUG + VerifyAccess("Children." + e.Action); + if (e.NewItems == null) + return; + // An already-owned subtree being attached below a differently-owned parent would keep its + // own owner, so the two halves of one displayed tree would demand two different threads. + // That is a mistake worth naming, but only once: the incoming owner is dropped afterwards + // so the subtree inherits the parent's, instead of reporting again on every later mutation. + Thread? parentOwner = EffectiveOwner; + foreach (SharpTreeNode node in e.NewItems) + { + if (node.owner != null && node.owner != parentOwner) + { + TreeThreadAffinity.Report(node, "attach of a subtree owned by another thread", node.owner); + node.owner = null; + node.ownerInvoke = null; + } + } +#endif + } + } + + /// + /// Collects thread-affinity violations. Nothing here is reached in + /// release builds: every caller is [Conditional("DEBUG")]. + /// + public static class TreeThreadAffinity + { + /// + /// Environment variable selecting the log file. Defaults to ILSpy.TreeAffinity.log in the + /// temp directory; set it to an empty string to disable file logging. + /// + public const string LogFileVariable = "ILSPY_TREE_AFFINITY_LOG"; + + /// + /// Environment variable enabling at startup (set it to 1). + /// + public const string FailFastVariable = "ILSPY_TREE_AFFINITY_FAILFAST"; + + static readonly ConcurrentDictionary violations = new(); + static readonly object logLock = new(); + + /// + /// When true, a violation throws after being recorded, so a debugger stops at the offending + /// frame. It is not what makes a violation observable - is recorded + /// either way, because the throw may well be swallowed by a caller that catches Exception. + /// An exploratory session over a large corpus leaves this off so one bad call site does not + /// interfere with the rest of the run. + /// + public static bool FailFast { get; set; } = Environment.GetEnvironmentVariable(FailFastVariable) == "1"; + + /// + /// The file violations are appended to, or null when file logging is disabled. + /// + public static string? LogFilePath { get; set; } = ResolveLogFilePath(); + + static string? ResolveLogFilePath() + { + string? configured = Environment.GetEnvironmentVariable(LogFileVariable); + if (configured == null) + return Path.Combine(Path.GetTempPath(), "ILSpy.TreeAffinity.log"); + return configured.Length == 0 ? null : configured; + } + + /// + /// The distinct violating call sites recorded so far, most frequent first. + /// + public static IReadOnlyList Violations + => violations.Values.OrderByDescending(v => v.Count).ToList(); + + public static void Clear() => violations.Clear(); + + internal static void Report(SharpTreeNode node, string operation, Thread expected) + { + // Skip Report and the [Conditional] wrapper that called it, so frame 0 is the mutation. + string stackTrace = new StackTrace(2, fNeedFileInfo: true).ToString(); + var violation = new TreeThreadAffinityViolation(Describe(node), operation, expected, Thread.CurrentThread, stackTrace); + Debug.WriteLine(violation.ToString()); + // Deduplicate by call site: a mutation inside a loop must not produce one entry per + // iteration. The first occurrence is written through immediately so a long-running + // session can be inspected without stopping the process; repeats only bump the count. + var recorded = violations.GetOrAdd(stackTrace, violation); + if (recorded.Hit() == 1) + AppendToLog(recorded.ToString()); + // Recorded first, thrown second. The throw is only a convenience for stopping a + // debugger at the offending frame; it is not the record. Tree mutation happens inside + // callers that catch Exception (the background decompile writes the failure into the + // text view), so a throw alone would be swallowed and a fail-fast run would come back + // green while violating. Violations is what a test or a session asserts on. + if (FailFast) + throw new InvalidOperationException(violation.ToString()); + } + + static string Describe(SharpTreeNode node) + { + string text; + try + { + // ToString() is what the model itself already uses for diagnostics. It can still fail + // here: this runs on a thread the node did not expect, which is the whole problem. + text = node.ToString() ?? ""; + } + catch (Exception ex) + { + text = ""; + } + return node.GetType().FullName + " \"" + text + "\""; + } + + static void AppendToLog(string text) + { + string? path = LogFilePath; + if (path == null) + return; + try + { + lock (logLock) + { + File.AppendAllText(path, text + Environment.NewLine + Environment.NewLine); + } + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // A diagnostic that cannot write its log must not take the session down with it. + } + } + } + + /// + /// One distinct call site that mutated a tree from a thread other than its owner. + /// + public sealed class TreeThreadAffinityViolation + { + int count; + + internal TreeThreadAffinityViolation(string node, string operation, Thread expected, Thread actual, string stackTrace) + { + this.Node = node; + this.Operation = operation; + this.ExpectedThread = DescribeThread(expected); + this.ActualThread = DescribeThread(actual); + this.StackTrace = stackTrace; + } + + public string Node { get; } + public string Operation { get; } + public string ExpectedThread { get; } + public string ActualThread { get; } + public string StackTrace { get; } + + /// Number of times this call site has been hit. + public int Count => count; + + internal int Hit() => Interlocked.Increment(ref count); + + static string DescribeThread(Thread thread) + => thread.ManagedThreadId + " \"" + (thread.Name ?? "") + "\""; + + public override string ToString() + { + var b = new StringBuilder(); + b.Append("SharpTreeNode thread affinity violation: ").Append(Operation).AppendLine(); + b.Append(" node: ").AppendLine(Node); + b.Append(" expected: thread ").AppendLine(ExpectedThread); + b.Append(" actual: thread ").AppendLine(ActualThread); + b.Append(StackTrace); + return b.ToString(); + } + } +} diff --git a/ILSpy.Tests/Controls/FlatListTreeNodeTests.cs b/ILSpy.Tests/Controls/FlatListTreeNodeTests.cs new file mode 100644 index 000000000..d3f4492d0 --- /dev/null +++ b/ILSpy.Tests/Controls/FlatListTreeNodeTests.cs @@ -0,0 +1,72 @@ +// Copyright (c) 2026 Siegfried Pammer +// +// 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 AwesomeAssertions; + +using ICSharpCode.ILSpyX.TreeView; + +using NUnit.Framework; + +namespace ICSharpCode.ILSpy.Tests.Controls; + +[TestFixture] +public class FlatListTreeNodeTests +{ + sealed class TestNode : SharpTreeNode + { + readonly string text; + public TestNode(string text) => this.text = text; + public override object Text => text; + public override string ToString() => text; + } + + [Test] + public void GetNodeByVisibleIndex_WalkingPastTheEnd_ThrowsNamingIndexAndLength() + { + var root = new TestNode("root"); + root.Children.Add(new TestNode("child")); + root.IsExpanded = true; + var listRoot = root.GetListRoot(); + listRoot.GetTotalListLength().Should().Be(2); + + // A restructure that happened under a reader leaves the augmented length disagreeing with + // the structure it describes: the length says there is a node at this index, the descent + // runs out of nodes before reaching it. + listRoot.totalListLength = 5; + + var error = Assert.Throws( + () => SharpTreeNode.GetNodeByVisibleIndex(listRoot, 4)); + + error!.Message.Should().Contain("4").And.Contain("5"); + } + + [Test] + public void GetNodeByVisibleIndex_WithinTheList_ReturnsTheNodeAtThatIndex() + { + var root = new TestNode("root"); + var child = new TestNode("child"); + root.Children.Add(child); + root.IsExpanded = true; + var listRoot = root.GetListRoot(); + + Assert.That(SharpTreeNode.GetNodeByVisibleIndex(listRoot, 0), Is.SameAs(root)); + Assert.That(SharpTreeNode.GetNodeByVisibleIndex(listRoot, 1), Is.SameAs(child)); + } +} diff --git a/ILSpy.Tests/Controls/SharpTreeViewTests.cs b/ILSpy.Tests/Controls/SharpTreeViewTests.cs index 80c860eb7..ba87155e1 100644 --- a/ILSpy.Tests/Controls/SharpTreeViewTests.cs +++ b/ILSpy.Tests/Controls/SharpTreeViewTests.cs @@ -151,4 +151,125 @@ public class SharpTreeViewTests c.IsSelected.Should().BeTrue(); a.IsSelected.Should().BeFalse("moving the selection clears the old node's flag"); } + + /// The number of rows the flattener should expose for a tree rooted in + /// when the root itself is not shown. + static int VisibleRowCount(SharpTreeNode root) + { + int count = 0; + void Walk(SharpTreeNode node) + { + foreach (SharpTreeNode child in node.Children) + { + if (child.IsHidden) + continue; + count++; + if (child.IsExpanded) + Walk(child); + } + } + Walk(root); + return count; + } + + static void AssertEveryRowResolves(SharpTreeView tree, SharpTreeNode root, string because) + { + tree.UpdateLayout(); + Dispatcher.UIThread.RunJobs(); + int expected = VisibleRowCount(root); + tree.ItemCount.Should().Be(expected, because); + for (int i = 0; i < expected; i++) + tree.ItemsView[i].Should().NotBeNull($"row {i} must resolve after {because}"); + } + + /// + /// Collapsing and removing nodes above a scrolled viewport shrinks the flattened list under + /// the virtualizing panel's realized index range. The panel must not be left indexing rows + /// that no longer exist. + /// + [AvaloniaTest] + public void Shrinking_The_Tree_Above_A_Scrolled_Viewport_Keeps_Every_Row_Resolvable() + { + var groups = Enumerable.Range(0, 200) + .Select(i => new TestNode($"g{i}", Enumerable.Range(0, 5) + .Select(j => new TestNode($"g{i}_{j}")).ToArray())) + .ToArray(); + var root = new TestNode("root", groups); + var tree = new SharpTreeView { ShowRoot = false, Root = root }; + var window = new Window { Content = tree, Width = 300, Height = 400 }; + window.Show(); + Dispatcher.UIThread.RunJobs(); + + foreach (var group in groups) + group.IsExpanded = true; + AssertEveryRowResolves(tree, root, "expanding every group"); + tree.GetRealizedContainers().Count().Should() + .BeLessThan(tree.ItemCount, "the panel must be virtualizing, or this test proves nothing"); + + // Park the realized range far from index 0, with a live selection inside it. + tree.SelectedItem = groups[^1].Children[^1]; + tree.ScrollIntoView(tree.ItemCount - 1); + AssertEveryRowResolves(tree, root, "scrolling to the last row"); + + for (int i = 0; i < 190; i++) + groups[i].IsExpanded = false; + AssertEveryRowResolves(tree, root, "collapsing 190 groups above the viewport"); + + for (int i = 0; i < 150; i++) + root.Children.RemoveAt(0); + AssertEveryRowResolves(tree, root, "removing 150 groups above the viewport"); + + tree.ScrollIntoView(0); + AssertEveryRowResolves(tree, root, "scrolling back to the top"); + } + + /// + /// A lazily loaded node replaces its placeholder with real children while the tree is + /// scrolled: the row count grows and shrinks in the same gesture. + /// + [AvaloniaTest] + public void Lazy_Loading_Under_A_Scrolled_Viewport_Keeps_Every_Row_Resolvable() + { + var lazy = Enumerable.Range(0, 100).Select(i => new LazyNode($"l{i}", 7)).ToArray(); + var root = new TestNode("root"); + foreach (var node in lazy) + root.Children.Add(node); + var tree = new SharpTreeView { ShowRoot = false, Root = root }; + var window = new Window { Content = tree, Width = 300, Height = 400 }; + window.Show(); + Dispatcher.UIThread.RunJobs(); + + AssertEveryRowResolves(tree, root, "the initial collapsed list"); + + tree.ScrollIntoView(tree.ItemCount - 1); + AssertEveryRowResolves(tree, root, "scrolling to the last row"); + + foreach (var node in lazy) + { + node.IsExpanded = true; + AssertEveryRowResolves(tree, root, $"lazily expanding {node.Text}"); + } + + foreach (var node in lazy) + node.ReloadChildren(); + AssertEveryRowResolves(tree, root, "reloading every lazy subtree in place"); + } + + sealed class LazyNode : SharpTreeNode + { + readonly string text; + readonly int childCount; + public LazyNode(string text, int childCount) + { + this.text = text; + this.childCount = childCount; + LazyLoading = true; + } + public override object Text => text; + protected override void LoadChildren() + { + for (int i = 0; i < childCount; i++) + Children.Add(new TestNode($"{text}_{i}")); + } + } } diff --git a/ILSpy.Tests/Controls/TreeThreadAffinityTests.cs b/ILSpy.Tests/Controls/TreeThreadAffinityTests.cs new file mode 100644 index 000000000..9f519c248 --- /dev/null +++ b/ILSpy.Tests/Controls/TreeThreadAffinityTests.cs @@ -0,0 +1,444 @@ +// Copyright (c) 2026 Siegfried Pammer +// +// 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.Collections.Concurrent; +using System.Runtime.ExceptionServices; +using System.Threading; + +using AwesomeAssertions; + +using ICSharpCode.ILSpyX.TreeView; + +using NUnit.Framework; + +namespace ICSharpCode.ILSpy.Tests.Controls; + +[TestFixture] +public class TreeThreadAffinityTests +{ + sealed class TestNode : SharpTreeNode + { + readonly string text; + public TestNode(string text) => this.text = text; + public override object Text => text; + public override string ToString() => text; + } + + /// + /// A lazy node that records where and how often its children were built, plus an optional hook + /// so a test can drive whatever LoadChildren would do in the app. + /// + sealed class LazyTestNode : SharpTreeNode + { + readonly string text; + + public LazyTestNode(string text) + { + this.text = text; + LazyLoading = true; + } + + public Action? OnLoadChildren { get; set; } + public Thread? LoadChildrenThread { get; private set; } + public int LoadChildrenCount { get; private set; } + + protected override void LoadChildren() + { + LoadChildrenThread = Thread.CurrentThread; + LoadChildrenCount++; + OnLoadChildren?.Invoke(this); + } + + public override object Text => text; + public override string ToString() => text; + } + + /// + /// A stand-in for the UI thread: a thread with a work queue, whose has the + /// same contract as the host dispatcher's - run the action there, block until it is done, and + /// run it inline when the caller already is that thread. + /// + sealed class OwnerThread : IDisposable + { + readonly BlockingCollection queue = new(); + readonly Thread thread; + int invokeCount; + + public OwnerThread() + { + thread = new Thread(() => { + foreach (var work in queue.GetConsumingEnumerable()) + work(); + }) { Name = "affinity-test-owner", IsBackground = true }; + thread.Start(); + } + + public Thread Thread => thread; + + /// Number of calls that actually had to be marshalled. + public int InvokeCount => invokeCount; + + public void Invoke(Action action) + { + if (Thread.CurrentThread == thread) + { + action(); + return; + } + Interlocked.Increment(ref invokeCount); + ExceptionDispatchInfo? failure = null; + using var done = new ManualResetEventSlim(); + queue.Add(() => { + try + { + action(); + } + catch (Exception ex) + { + failure = ExceptionDispatchInfo.Capture(ex); + } + finally + { + done.Set(); + } + }); + done.Wait(); + failure?.Throw(); + } + + public void Dispose() + { + queue.CompleteAdding(); + thread.Join(); + queue.Dispose(); + } + } + + bool oldFailFast; + string? oldLogFilePath; + + [SetUp] + public void SetUp() + { + oldFailFast = TreeThreadAffinity.FailFast; + oldLogFilePath = TreeThreadAffinity.LogFilePath; + // Tests must not append to whatever log an exploratory session is collecting, and must not + // inherit the mode from the environment: the ones that need a throw opt in individually. + TreeThreadAffinity.LogFilePath = null; + TreeThreadAffinity.FailFast = false; + TreeThreadAffinity.Clear(); + } + + [TearDown] + public void TearDown() + { + TreeThreadAffinity.FailFast = oldFailFast; + TreeThreadAffinity.LogFilePath = oldLogFilePath; + TreeThreadAffinity.Clear(); + } + + /// + /// Runs to completion on a dedicated thread and returns whatever it + /// threw. Join() makes this deterministic: no sleeps, no polling. + /// + static Exception? RunOnOtherThread(Action action) + { + Exception? error = null; + var thread = new Thread(() => { + try + { + action(); + } + catch (Exception ex) + { + error = ex; + } + }) { Name = "affinity-test-worker", IsBackground = true }; + thread.Start(); + thread.Join(); + return error; + } + + [Test] + public void EnsureLazyChildrenFromNonOwningThread_LoadsOnTheOwningThread() + { + using var owner = new OwnerThread(); + var root = new LazyTestNode("root"); + root.SetOwner(owner.Thread, owner.Invoke); + + root.EnsureLazyChildren(); + + root.LoadChildrenThread.Should().BeSameAs(owner.Thread); + root.LoadChildrenCount.Should().Be(1); + owner.InvokeCount.Should().Be(1); + } + + [Test] + public void EnsureLazyChildrenOnUnownedTree_LoadsOnTheCallingThread() + { + var root = new LazyTestNode("root"); + + var error = RunOnOtherThread(root.EnsureLazyChildren); + + error.Should().BeNull(); + root.LoadChildrenThread.Should().NotBeNull(); + root.LoadChildrenThread!.Name.Should().Be("affinity-test-worker"); + root.LoadChildrenCount.Should().Be(1); + } + + [Test] + public void EnsureLazyChildrenOnTheOwningThread_DoesNotMarshalAndLoadsOnce() + { + using var owner = new OwnerThread(); + var root = new LazyTestNode("root"); + root.SetOwner(owner.Thread, owner.Invoke); + + owner.Invoke(() => { + root.EnsureLazyChildren(); + root.EnsureLazyChildren(); + }); + + // One marshalled call: the test thread getting onto the owner. Nothing inside it needed a + // second hop, which is what keeps a blocking invoke from deadlocking on itself. + owner.InvokeCount.Should().Be(1); + root.LoadChildrenCount.Should().Be(1); + root.LoadChildrenThread.Should().BeSameAs(owner.Thread); + } + + [Test] + public void NestedEnsureLazyChildrenFromNonOwningThread_MarshalsOnlyOnce() + { + // The shape AssemblyReferenceReferencedTypesTreeNode has: loading a node's children + // realises a child node's children from inside LoadChildren. + using var owner = new OwnerThread(); + var root = new LazyTestNode("root"); + var child = new LazyTestNode("child"); + root.OnLoadChildren = node => { + node.Children.Add(child); + child.EnsureLazyChildren(); + }; + root.SetOwner(owner.Thread, owner.Invoke); + + root.EnsureLazyChildren(); + + owner.InvokeCount.Should().Be(1); + root.LoadChildrenThread.Should().BeSameAs(owner.Thread); + child.LoadChildrenThread.Should().BeSameAs(owner.Thread); + child.LoadChildrenCount.Should().Be(1); + } + +#if DEBUG + + [Test] + public void EnsureLazyChildrenFromNonOwningThread_ProducesNoViolation() + { + TreeThreadAffinity.FailFast = true; + using var owner = new OwnerThread(); + var root = new LazyTestNode("root"); + root.OnLoadChildren = node => node.Children.Add(new TestNode("child")); + root.SetOwner(owner.Thread, owner.Invoke); + + root.EnsureLazyChildren(); + + TreeThreadAffinity.Violations.Should().BeEmpty(); + } + + [Test] + public void FailFastViolation_IsRecordedEvenWhenTheThrowIsSwallowed() + { + // The app's background decompile wraps every node in catch (Exception), so a fail-fast + // throw never reaches the test host. The collector has to stay authoritative no matter who + // catches, or a fail-fast suite run comes back green and means nothing. + TreeThreadAffinity.FailFast = true; + var root = new TestNode("root"); + root.SetOwner(); + + var error = RunOnOtherThread(() => { + try + { + root.Children.Add(new TestNode("child")); + } + catch (Exception) + { + } + }); + + error.Should().BeNull(); + TreeThreadAffinity.Violations.Should().ContainSingle() + .Which.Operation.Should().Contain("Children.Add"); + } + + [Test] + public void ChildrenAddFromNonOwningThread_IsReported() + { + TreeThreadAffinity.FailFast = true; + var root = new TestNode("root"); + root.SetOwner(); + + var error = RunOnOtherThread(() => root.Children.Add(new TestNode("child"))); + + error.Should().BeOfType(); + error!.Message.Should().Contain("Children.Add").And.Contain("\"root\""); + } + + [Test] + public void IsExpandedFromNonOwningThread_IsReported() + { + TreeThreadAffinity.FailFast = true; + var root = new TestNode("root"); + root.Children.Add(new TestNode("child")); + root.SetOwner(); + + var error = RunOnOtherThread(() => root.IsExpanded = true); + + error.Should().BeOfType(); + error!.Message.Should().Contain(nameof(SharpTreeNode.IsExpanded)); + } + + [Test] + public void IsHiddenFromNonOwningThread_IsReported() + { + TreeThreadAffinity.FailFast = true; + var root = new TestNode("root"); + var child = new TestNode("child"); + root.Children.Add(child); + root.SetOwner(); + + var error = RunOnOtherThread(() => child.IsHidden = true); + + error.Should().BeOfType(); + error!.Message.Should().Contain(nameof(SharpTreeNode.IsHidden)); + } + + [Test] + public void UnownedTree_IsNotChecked() + { + TreeThreadAffinity.FailFast = true; + var root = new TestNode("root"); + + var error = RunOnOtherThread(() => { + root.Children.Add(new TestNode("child")); + root.IsExpanded = true; + }); + + error.Should().BeNull(); + TreeThreadAffinity.Violations.Should().BeEmpty(); + } + + [Test] + public void SubtreeBuiltOffThreadThenPublishedByOwner_IsNotReported() + { + // The pattern the analyzers use: assemble a subtree on a worker, hand it to the owning + // thread, attach it there. + TreeThreadAffinity.FailFast = true; + var root = new TestNode("root"); + root.SetOwner(); + + TestNode? built = null; + var error = RunOnOtherThread(() => { + built = new TestNode("built"); + built.Children.Add(new TestNode("grandchild")); + built.IsExpanded = true; + }); + + error.Should().BeNull(); + root.Children.Add(built!); + TreeThreadAffinity.Violations.Should().BeEmpty(); + } + + [Test] + public void OwnershipIsInheritedByChildrenAttachedLater() + { + TreeThreadAffinity.FailFast = true; + var root = new TestNode("root"); + root.SetOwner(); + var child = new TestNode("child"); + root.Children.Add(child); + + var error = RunOnOtherThread(() => child.Children.Add(new TestNode("grandchild"))); + + error.Should().BeOfType(); + error!.Message.Should().Contain("\"child\""); + } + + [Test] + public void AttachingSubtreeOwnedByAnotherThread_IsReportedOnceThenInherits() + { + var root = new TestNode("root"); + root.SetOwner(); + var foreign = new TestNode("foreign"); + RunOnOtherThread(() => foreign.SetOwner()).Should().BeNull(); + + root.Children.Add(foreign); + + TreeThreadAffinity.Violations.Should().ContainSingle() + .Which.Operation.Should().Contain("owned by another thread"); + + // The conflicting owner is dropped, so the subtree now inherits the root's owner: further + // mutation from the owning thread is clean, and from any other thread is a violation. + TreeThreadAffinity.Clear(); + foreign.Children.Add(new TestNode("grandchild")); + TreeThreadAffinity.Violations.Should().BeEmpty(); + + TreeThreadAffinity.FailFast = true; + RunOnOtherThread(() => foreign.Children.Add(new TestNode("other"))) + .Should().BeOfType(); + } + + [Test] + public void RepeatedViolationsFromOneCallSite_AreDeduplicatedAndCounted() + { + var root = new TestNode("root"); + root.SetOwner(); + + var error = RunOnOtherThread(() => { + for (int i = 0; i < 5; i++) + root.Children.Add(new TestNode("child" + i)); + }); + + error.Should().BeNull(); + var violation = TreeThreadAffinity.Violations.Should().ContainSingle().Subject; + violation.Count.Should().Be(5); + violation.StackTrace.Should().Contain(nameof(RepeatedViolationsFromOneCallSite_AreDeduplicatedAndCounted)); + violation.ActualThread.Should().Contain("affinity-test-worker"); + } + + [Test] + public void OwnershipHandoffFromNonOwningThread_IsReported() + { + TreeThreadAffinity.FailFast = true; + var root = new TestNode("root"); + root.SetOwner(); + + var error = RunOnOtherThread(() => root.SetOwner()); + + error.Should().BeOfType(); + error!.Message.Should().Contain(nameof(SharpTreeNode.SetOwner)); + } + +#else + + [Test] + public void ThreadAffinityChecksAreDebugOnly() + { + Assert.Ignore("SharpTreeNode thread-affinity checking is compiled out in release builds."); + } + +#endif +} diff --git a/ILSpy.Tests/TreeThreadAffinityGuard.cs b/ILSpy.Tests/TreeThreadAffinityGuard.cs new file mode 100644 index 000000000..6e5f4084c --- /dev/null +++ b/ILSpy.Tests/TreeThreadAffinityGuard.cs @@ -0,0 +1,67 @@ +// Copyright (c) 2026 Siegfried Pammer +// +// 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 ICSharpCode.ILSpyX.TreeView; + +using NUnit.Framework; +using NUnit.Framework.Interfaces; + +[assembly: TreeThreadAffinityGuard] + +// Deliberately outside any namespace: this is applied to the assembly, and the attribute has to be +// nameable from the assembly-level attribute list. + +/// +/// Fails any test that mutated a displayed tree from a thread other than the tree's owner. +/// +/// +/// The affinity check records into instead of relying +/// on a throw, because tree mutation happens inside callers that catch Exception - the background +/// decompile turns any exception a node raises into text in the output pane. A throw there is +/// swallowed and the run still passes, so something has to read the collector, and it has to be +/// per test: an assembly-level teardown failure is reported but does not fail the run or change +/// the exit code. Debug builds only - the check compiles away in release, where the collector +/// stays empty and this is a no-op. +/// +/// A fixture that provokes violations on purpose clears the collector in its own +/// [TearDown], which runs before this. +/// +[AttributeUsage(AttributeTargets.Assembly)] +public sealed class TreeThreadAffinityGuardAttribute : Attribute, ITestAction +{ + public ActionTargets Targets => ActionTargets.Test; + + public void BeforeTest(ITest test) + { + TreeThreadAffinity.Clear(); + } + + public void AfterTest(ITest test) + { + var violations = TreeThreadAffinity.Violations; + if (violations.Count == 0) + return; + TreeThreadAffinity.Clear(); + Assert.Fail($"{violations.Count} tree thread-affinity violation(s) were recorded while this test ran:" + + Environment.NewLine + + string.Join(Environment.NewLine, violations.Select(v => v.ToString()))); + } +} diff --git a/ILSpy/Controls/TreeView/SharpTreeView.cs b/ILSpy/Controls/TreeView/SharpTreeView.cs index e126e9db1..2e2b0bcbe 100644 --- a/ILSpy/Controls/TreeView/SharpTreeView.cs +++ b/ILSpy/Controls/TreeView/SharpTreeView.cs @@ -20,6 +20,7 @@ using System; using System.Collections.Generic; using System.Collections.Specialized; using System.Linq; +using System.Threading; using Avalonia; using Avalonia.Controls; @@ -129,6 +130,15 @@ namespace ICSharpCode.ILSpy.Controls.TreeView } if (Root != null) { + // The tree becomes reachable from the UI here, so this is where the UI thread takes + // ownership of it. Avalonia raises property changes on the UI thread, so the calling + // thread is the right owner. Every SharpTreeView in the app routes through Reload, + // so this single call claims every displayed tree. + // + // The dispatcher invoke goes with it: EnsureLazyChildren uses it to get back onto + // this thread when a background decompile realizes a node's children, which the + // tree model cannot do for itself - it must not name a UI framework. + Root.SetOwner(Thread.CurrentThread, action => Dispatcher.UIThread.Invoke(action)); if (!(ShowRoot && ShowRootExpander)) Root.IsExpanded = true; flattener = new TreeFlattener(Root, ShowRoot); diff --git a/ILSpy/TreeNodes/ReferenceFolderTreeNode.cs b/ILSpy/TreeNodes/ReferenceFolderTreeNode.cs index a72d81bce..bad5ba817 100644 --- a/ILSpy/TreeNodes/ReferenceFolderTreeNode.cs +++ b/ILSpy/TreeNodes/ReferenceFolderTreeNode.cs @@ -19,8 +19,6 @@ using System.Linq; using System.Threading.Tasks; -using Avalonia.Threading; - using ICSharpCode.Decompiler; using ICSharpCode.Decompiler.Metadata; using ICSharpCode.ILSpy.Properties; @@ -69,8 +67,7 @@ namespace ICSharpCode.ILSpy.TreeNodes output.WriteLine($"Effective TargetFramework-Id: {effectiveTargetFramework}"); output.WriteLine($"Detected RuntimePack: {runtimePack}"); - // Children realise lazily on the UI thread; we may run from a background decompile. - Dispatcher.UIThread.Invoke(EnsureLazyChildren); + EnsureLazyChildren(); output.WriteLine(); output.WriteLine("Referenced assemblies (in metadata order):"); foreach (var node in Children.OfType()) diff --git a/ILSpy/TreeNodes/ResourceListTreeNode.cs b/ILSpy/TreeNodes/ResourceListTreeNode.cs index 518a6d2e6..dadc9b9fe 100644 --- a/ILSpy/TreeNodes/ResourceListTreeNode.cs +++ b/ILSpy/TreeNodes/ResourceListTreeNode.cs @@ -18,8 +18,6 @@ using System.Linq; -using Avalonia.Threading; - using ICSharpCode.Decompiler; using ICSharpCode.Decompiler.Metadata; using ICSharpCode.ILSpy.Properties; @@ -80,7 +78,7 @@ namespace ICSharpCode.ILSpy.TreeNodes public override void Decompile(Language language, ITextOutput output, DecompilationOptions options) { - Dispatcher.UIThread.Invoke(EnsureLazyChildren); + EnsureLazyChildren(); foreach (var child in Children.OfType()) { child.Decompile(language, output, options);