diff --git a/ICSharpCode.ILSpyX/AssemblyList.cs b/ICSharpCode.ILSpyX/AssemblyList.cs index cb4c425b9..a5a9bda07 100644 --- a/ICSharpCode.ILSpyX/AssemblyList.cs +++ b/ICSharpCode.ILSpyX/AssemblyList.cs @@ -340,7 +340,6 @@ namespace ICSharpCode.ILSpyX { VerifyAccess(); file = Path.GetFullPath(file); - LoadedAssembly evicted; LoadedAssembly newAsm; lock (lockObj) { @@ -358,9 +357,11 @@ namespace ICSharpCode.ILSpyX Debug.Assert(newAsm.FileName == file); byFilename[file] = newAsm; this.assemblies[index] = newAsm; - evicted = target; } - evicted.Dispose(); + // The replaced assembly is intentionally NOT disposed: its MetadataFile may still be + // referenced by open document tabs / tree nodes, and there is no safe point at which + // the list can know those references are gone. Dropping it lets the GC reclaim it + // once nothing holds it, rather than risk a use-after-dispose. return newAsm; } @@ -391,7 +392,8 @@ namespace ICSharpCode.ILSpyX this.assemblies.Remove(target); this.assemblies.Insert(index, newAsm); } - target.Dispose(); + // Not disposed on purpose -- see HotReplaceAssembly. The old MetadataFile may still be + // live in the UI; let the GC reclaim it instead of risking a use-after-dispose. return newAsm; } @@ -403,21 +405,21 @@ namespace ICSharpCode.ILSpyX assemblies.Remove(assembly); byFilename.Remove(assembly.FileName); } - assembly.Dispose(); + // Removed from the list but NOT disposed: open tabs / tree nodes may still hold its + // MetadataFile and there's no safe point to know they don't. The GC reclaims it once + // the last reference is gone. } public void Clear() { VerifyAccess(); - LoadedAssembly[] removed; lock (lockObj) { - removed = assemblies.ToArray(); assemblies.Clear(); byFilename.Clear(); } - foreach (var asm in removed) - asm.Dispose(); + // Cleared but not disposed -- see Unload. Lingering references in the UI must not see + // a disposed MetadataFile. } public void Sort(IComparer comparer) { diff --git a/ILSpy.Tests/AssemblyList/AssemblyListDisposalTests.cs b/ILSpy.Tests/AssemblyList/AssemblyListDisposalTests.cs new file mode 100644 index 000000000..54a60b15c --- /dev/null +++ b/ILSpy.Tests/AssemblyList/AssemblyListDisposalTests.cs @@ -0,0 +1,73 @@ +// Copyright (c) 2026 AlphaSierraPapa for the SharpDevelop Team +// +// 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.Reflection; + +using AwesomeAssertions; + +using ICSharpCode.ILSpyX; + +using NUnit.Framework; + +namespace ICSharpCode.ILSpy.Tests; + +/// +/// The assembly list must NOT dispose a (and so its +/// MetadataFile) when it removes one: open document tabs and tree nodes can still hold the +/// MetadataFile, and the list has no safe point to know those references are gone. Disposing +/// unmaps the file out from under a live reader -> use-after-dispose. Removed assemblies are +/// dropped and left to the GC instead. +/// +[TestFixture] +public class AssemblyListDisposalTests +{ + static bool IsDisposed(LoadedAssembly assembly) + => (bool)typeof(LoadedAssembly) + .GetField("isDisposed", BindingFlags.NonPublic | BindingFlags.Instance)! + .GetValue(assembly)!; + + [Test] + public void Unload_Does_Not_Dispose_The_Removed_Assembly() + { + var list = new AssemblyList(); + var assembly = list.OpenAssembly(typeof(AssemblyListDisposalTests).Assembly.Location); + assembly.Should().NotBeNull("precondition: the test assembly opens"); + // Force the load to complete so there's a real MetadataFile that disposal would unmap. + assembly!.GetMetadataFileAsync().GetAwaiter().GetResult(); + + list.Unload(assembly); + + IsDisposed(assembly).Should().BeFalse( + "Unload must not dispose the LoadedAssembly / MetadataFile -- open tabs or tree nodes " + + "may still reference it, so the list leaves it to the GC instead of risking a " + + "use-after-dispose"); + } + + [Test] + public void Clear_Does_Not_Dispose_The_Removed_Assemblies() + { + var list = new AssemblyList(); + var assembly = list.OpenAssembly(typeof(AssemblyListDisposalTests).Assembly.Location); + assembly!.GetMetadataFileAsync().GetAwaiter().GetResult(); + + list.Clear(); + + IsDisposed(assembly).Should().BeFalse( + "Clear must not dispose the assemblies it removes -- same use-after-dispose hazard as Unload"); + } +} diff --git a/ILSpy/Docking/DockWorkspace.cs b/ILSpy/Docking/DockWorkspace.cs index f5797949a..9f4f86c79 100644 --- a/ILSpy/Docking/DockWorkspace.cs +++ b/ILSpy/Docking/DockWorkspace.cs @@ -282,12 +282,12 @@ namespace ILSpy.Docking } if (!anyTouchesRemoved) continue; - // Cancel synchronously, BEFORE AssemblyList.Unload reaches assembly.Dispose(). - // The decompile worker checks its CancellationToken between transforms; the - // spinner exits when its Task.Delay sees the cancellation. After this returns - // no managed reader should hold a live handle into the MetadataFile that's - // about to be unmapped. (LoadedAssembly.Text additionally returns its cached - // value once isDisposed flips, covering any residual cooperative-cancel race.) + // Cancel the in-flight decompile of an assembly the user just removed -- no point + // finishing work on something no longer in the list. The decompile worker checks + // its CancellationToken between transforms; the spinner exits when its Task.Delay + // sees the cancellation. Note AssemblyList.Unload no longer disposes the + // LoadedAssembly / MetadataFile (its lifetime can't be known safely), so this is a + // courtesy cancel, not a guard against unmapping a file out from under a reader. tab.CancelDecompilationCommand.Execute(null); if (anyAlive) continue;