ILSpy

9708 Commits

54 Branches

94 Tags

91 MiB

Tree: ad08d21884

Commit Graph

Author	SHA1	Message	Date
Siegfried Pammer	228041085b	Fix #3290 : marshal EnsureLazyChildren onto the tree's owning thread The crash is a NullReferenceException in GetNodeByVisibleIndex, reached when a background decompile realizes a node's children while the UI thread is indexing the flattener. Eight ILSpyTreeNode.Decompile overrides call EnsureLazyChildren from that task; two wrapped it in Dispatcher.UIThread.Invoke, six did not, and one of the two lost its wrapper in the Avalonia port with no test noticing for a release cycle. A rule every call site has to remember is a rule that gets broken again, so EnsureLazyChildren marshals itself instead: SetOwner already named the owning thread, and now also carries the host's way onto it. A call already on the owner runs inline, so a blocking invoke cannot deadlock on itself and a nested load costs no further hop; an unowned tree is left unmarshalled, which keeps building a subtree on a worker and publishing it on the UI thread legal. The affinity check stays as the regression detector, but its fail-fast throw was worthless on its own: tree mutation happens inside callers that catch Exception, so the throw ended up rendered into the decompiled output and the run passed. The violation is now recorded before the throw, and an assembly-level NUnit test action fails the test that produced one - an assembly-level teardown failure is reported but leaves the exit code at zero. Assisted-by: Claude:claude-opus-5:Claude Code	2 weeks ago
Siegfried Pammer	ca3e528ce2	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	2 weeks ago

Author

SHA1

Message

Date

Siegfried Pammer

228041085b

Fix #3290 : marshal EnsureLazyChildren onto the tree's owning thread

The crash is a NullReferenceException in GetNodeByVisibleIndex, reached when a
background decompile realizes a node's children while the UI thread is indexing
the flattener. Eight ILSpyTreeNode.Decompile overrides call EnsureLazyChildren
from that task; two wrapped it in Dispatcher.UIThread.Invoke, six did not, and
one of the two lost its wrapper in the Avalonia port with no test noticing for a
release cycle. A rule every call site has to remember is a rule that gets broken
again, so EnsureLazyChildren marshals itself instead: SetOwner already named the
owning thread, and now also carries the host's way onto it. A call already on the
owner runs inline, so a blocking invoke cannot deadlock on itself and a nested
load costs no further hop; an unowned tree is left unmarshalled, which keeps
building a subtree on a worker and publishing it on the UI thread legal.

The affinity check stays as the regression detector, but its fail-fast throw was
worthless on its own: tree mutation happens inside callers that catch Exception,
so the throw ended up rendered into the decompiled output and the run passed. The
violation is now recorded before the throw, and an assembly-level NUnit test
action fails the test that produced one - an assembly-level teardown failure is
reported but leaves the exit code at zero.

Assisted-by: Claude:claude-opus-5:Claude Code

2 weeks ago

Siegfried Pammer

ca3e528ce2

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

2 weeks ago

2 Commits (ad08d21884fc080b6580b2acbe088ee688d21cbd)