From ca3e528ce24bf62df6d6bbbb3b34846633ead6a4 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Tue, 1 Sep 2026 20:30:46 +0200 Subject: [PATCH] Add a Debug-only thread-affinity check to the shared tree model Issue #3290 is a NullReferenceException in GetNodeByVisibleIndex that is provably unreachable single-threaded: TreeFlattener.Count and GetNodeByVisibleIndex read the same totalListLength fields back to back, so a stale index yields ArgumentOutOfRangeException, never an NRE. A stress harness with reader threads racing an IsExpanded/Children mutator reproduces exactly that NRE, so the crash requires a mutation from a foreign thread. The rule that a displayed tree is only mutated from the UI thread was pure convention: ICSharpCode.ILSpyX/TreeView contained no VerifyAccess, lock or dispatcher of any kind, and two tree nodes already carry a Dispatcher.UIThread.Invoke workaround for the same hazard, which means it has been hit before and fixed one site at a time. ICSharpCode.ILSpyX is host-agnostic and must not name a dispatcher, so ownership is stated by the host instead of inferred: SetOwner(Thread) marks the thread allowed to mutate a node and its subtree. Unowned means unchecked, which is what makes the analyzer pattern legal - build a subtree on a worker, publish it on the UI thread - without an exception carved into the rule. The owner is resolved by walking up the model-parent chain to the nearest explicit owner rather than stamped onto every node. That buys the propagation rules for free: one call on the root covers the whole displayed tree, children attached later inherit it with no bookkeeping, and a subtree built off-thread is unchecked while it is being built yet inherits the owner the moment it is attached - an attachment which is itself a checked mutation of the owned tree. A subtree that already carries a different owner would otherwise leave one displayed tree demanding two threads, so that case is reported once and the incoming owner dropped, rather than reported on every later mutation. Re-owning is allowed because handing a tree over is the point, but the handoff must come from the current owner: a background thread taking a live tree away from the UI is the race being hunted. The check sits in SharpTreeNodeCollection.OnCollectionChanged, which every Children mutator funnels through, and in the IsExpanded and IsHidden setters - the three entry points that invalidate totalListLength. Checking in OnCollectionChanged also means a violation is reported before OnChildrenChanged rewrites the flat-list tree, so the AVL structure is left intact. Violations are collected rather than fatal by default, with per-call-site deduplication and a count, and the first hit of each site written straight through to a log file so a long exploratory session can be read while it runs. FailFast makes them throw so tests can observe one deterministically. Everything is behind #if DEBUG plus [Conditional("DEBUG")], so the release build has no field on SharpTreeNode and no call at any site; verified by decompiling the release assembly. Assisted-by: Claude:claude-opus-5:Claude Code --- ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs | 2 + .../TreeView/SharpTreeNodeCollection.cs | 5 + .../TreeView/TreeThreadAffinity.cs | 286 ++++++++++++++++++ .../Controls/TreeThreadAffinityTests.cs | 245 +++++++++++++++ ILSpy/Controls/TreeView/SharpTreeView.cs | 5 + 5 files changed, 543 insertions(+) create mode 100644 ICSharpCode.ILSpyX/TreeView/TreeThreadAffinity.cs create mode 100644 ILSpy.Tests/Controls/TreeThreadAffinityTests.cs 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);