diff --git a/ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs b/ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs index 74b66cd6b..34c5931c9 100644 --- a/ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs +++ b/ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs @@ -161,6 +161,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 +288,7 @@ namespace ICSharpCode.ILSpyX.TreeView set { if (isExpanded != value) { + VerifyAccess(nameof(IsExpanded)); isExpanded = value; if (isExpanded) { 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..4792fe0cd --- /dev/null +++ b/ICSharpCode.ILSpyX/TreeView/TreeThreadAffinity.cs @@ -0,0 +1,286 @@ +// 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 checking for 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 convention 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, and every later structural + // mutation of that tree is verified against the owning thread. + partial class SharpTreeNode + { +#if DEBUG + Thread? owner; + + /// + /// The nearest explicit owner on the model-parent chain, 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. + /// + /// + Thread? EffectiveOwner { + get { + for (SharpTreeNode? node = this; node != null; node = node.modelParent) + { + if (node.owner != null) + return node.owner; + } + return null; + } + } +#endif + + /// + /// Declares as the only thread allowed to structurally mutate this + /// node and its subtree. Debug-only: the call is compiled away entirely in release builds. + /// + /// + /// 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 this + /// check hunts for. + /// + [Conditional("DEBUG")] + public void SetOwner(Thread owner) + { +#if DEBUG + VerifyAccess(nameof(SetOwner)); + this.owner = owner; +#endif + } + + /// + /// Declares the calling thread as the only thread allowed to structurally mutate this node + /// and its subtree. Debug-only. + /// + [Conditional("DEBUG")] + public void SetOwner() + { +#if DEBUG + SetOwner(Thread.CurrentThread); +#endif + } + + /// + /// 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; + } + } +#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 instead of being recorded. Tests use this to observe a + /// violation deterministically; an exploratory session over a large corpus leaves it off so + /// that one bad call site does not end the run and hide all the others. + /// + 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); + if (FailFast) + throw new InvalidOperationException(violation.ToString()); + 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()); + } + + 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/TreeThreadAffinityTests.cs b/ILSpy.Tests/Controls/TreeThreadAffinityTests.cs new file mode 100644 index 000000000..832dc3e2c --- /dev/null +++ b/ILSpy.Tests/Controls/TreeThreadAffinityTests.cs @@ -0,0 +1,245 @@ +// 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.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; + } + + 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; + } + +#if DEBUG + + [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/Controls/TreeView/SharpTreeView.cs b/ILSpy/Controls/TreeView/SharpTreeView.cs index e126e9db1..a8e71cb4a 100644 --- a/ILSpy/Controls/TreeView/SharpTreeView.cs +++ b/ILSpy/Controls/TreeView/SharpTreeView.cs @@ -129,6 +129,11 @@ 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. + Root.SetOwner(); if (!(ShowRoot && ShowRootExpander)) Root.IsExpanded = true; flattener = new TreeFlattener(Root, ShowRoot);