fix: Attempt to better handle hook disposal (#1803)

- Use a Weak Concurrent Collection to track scoped hooks
- Make `Hook`s remove themselves from the Tracked Hook list.

Dalamud/Hooking/AsmHook.cs

@@ -20,6 +20,8 @@ public sealed class AsmHook : IDisposable, IDalamudHook
     private bool isEnabled = false;
 
     private DynamicMethod statsMethod;
+    
+    private Guid hookId = Guid.NewGuid();
 
     /// <summary>
     /// Initializes a new instance of the <see cref="AsmHook"/> class.
@@ -44,7 +46,7 @@ public sealed class AsmHook : IDisposable, IDalamudHook
         this.statsMethod.GetILGenerator().Emit(OpCodes.Ret);
         var dele = this.statsMethod.CreateDelegate(typeof(Action));
 
-        HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, dele, Assembly.GetCallingAssembly()));
+        HookManager.TrackedHooks.TryAdd(this.hookId, new HookInfo(this, dele, Assembly.GetCallingAssembly()));
     }
 
     /// <summary>
@@ -70,7 +72,7 @@ public sealed class AsmHook : IDisposable, IDalamudHook
         this.statsMethod.GetILGenerator().Emit(OpCodes.Ret);
         var dele = this.statsMethod.CreateDelegate(typeof(Action));
 
-        HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, dele, Assembly.GetCallingAssembly()));
+        HookManager.TrackedHooks.TryAdd(this.hookId, new HookInfo(this, dele, Assembly.GetCallingAssembly()));
     }
 
     /// <summary>
@@ -116,6 +118,8 @@ public sealed class AsmHook : IDisposable, IDalamudHook
 
         this.IsDisposed = true;
 
+        HookManager.TrackedHooks.TryRemove(this.hookId, out _);
+
         if (this.isEnabled)
         {
             this.isEnabled = false;

Dalamud/Hooking/Hook.cs

@@ -71,6 +71,11 @@ public abstract class Hook<T> : IDalamudHook where T : Delegate
     /// <inheritdoc/>
     public virtual string BackendName => throw new NotImplementedException();
     
+    /// <summary>
+    /// Gets the unique GUID for this hook.
+    /// </summary>
+    protected Guid HookId { get; } = Guid.NewGuid();
+    
     /// <summary>
     /// Remove a hook from the current process.
     /// </summary>

Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs

@@ -100,7 +100,7 @@ internal class FunctionPointerVariableHook<T> : Hook<T>
 
             unhooker.TrimAfterHook();
 
-            HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, detour, callingAssembly));
+            HookManager.TrackedHooks.TryAdd(this.HookId, new HookInfo(this, detour, callingAssembly));
         }
     }
 
@@ -137,6 +137,8 @@ internal class FunctionPointerVariableHook<T> : Hook<T>
 
         this.Disable();
 
+        HookManager.TrackedHooks.TryRemove(this.HookId, out _);
+
         var index = HookManager.MultiHookTracker[this.Address].IndexOf(this);
         HookManager.MultiHookTracker[this.Address][index] = null;
 

Dalamud/Hooking/Internal/GameInteropProviderPluginScoped.cs

@@ -1,5 +1,4 @@
-using System.Collections.Concurrent;
+using System.Diagnostics;
-using System.Diagnostics;
 using System.Linq;
 
 using Dalamud.Game;
@@ -7,6 +6,7 @@ using Dalamud.IoC;
 using Dalamud.IoC.Internal;
 using Dalamud.Plugin.Internal.Types;
 using Dalamud.Plugin.Services;
+using Dalamud.Utility;
 using Dalamud.Utility.Signatures;
 using Serilog;
 
@@ -25,7 +25,7 @@ internal class GameInteropProviderPluginScoped : IGameInteropProvider, IInternal
     private readonly LocalPlugin plugin;
     private readonly SigScanner scanner;
 
-    private readonly ConcurrentBag<IDalamudHook> trackedHooks = new();
+    private readonly WeakConcurrentCollection<IDalamudHook> trackedHooks = new();
 
     /// <summary>
     /// Initializes a new instance of the <see cref="GameInteropProviderPluginScoped"/> class.

Dalamud/Hooking/Internal/MinHookHook.cs

@@ -35,7 +35,7 @@ internal class MinHookHook<T> : Hook<T> where T : Delegate
 
             unhooker.TrimAfterHook();
 
-            HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, detour, callingAssembly));
+            HookManager.TrackedHooks.TryAdd(this.HookId, new HookInfo(this, detour, callingAssembly));
         }
     }
 
@@ -76,6 +76,8 @@ internal class MinHookHook<T> : Hook<T> where T : Delegate
             HookManager.MultiHookTracker[this.Address][index] = null;
         }
 
+        HookManager.TrackedHooks.TryRemove(this.HookId, out _);
+
         base.Dispose();
     }
 

Dalamud/Hooking/Internal/ReloadedHook.cs

@@ -30,7 +30,7 @@ internal class ReloadedHook<T> : Hook<T> where T : Delegate
 
             unhooker.TrimAfterHook();
 
-            HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, detour, callingAssembly));
+            HookManager.TrackedHooks.TryAdd(this.HookId, new HookInfo(this, detour, callingAssembly));
         }
     }
 
@@ -63,6 +63,8 @@ internal class ReloadedHook<T> : Hook<T> where T : Delegate
         if (this.IsDisposed)
             return;
 
+        HookManager.TrackedHooks.TryRemove(this.HookId, out _);
+
         this.Disable();
 
         base.Dispose();

Dalamud/Interface/Internal/Windows/PluginStatWindow.cs

@@ -258,8 +258,6 @@ internal class PluginStatWindow : Window
             ImGui.EndTabItem();
         }
 
-        var toRemove = new List<Guid>();
-
         if (ImGui.BeginTabItem("Hooks"))
         {
             ImGui.Checkbox("Show Dalamud Hooks", ref this.showDalamudHooks);
@@ -291,9 +289,6 @@ internal class PluginStatWindow : Window
                 {
                     try
                     {
-                        if (trackedHook.Hook.IsDisposed)
-                            toRemove.Add(guid);
-
                         if (!this.showDalamudHooks && trackedHook.Assembly == Assembly.GetExecutingAssembly())
                             continue;
 
@@ -355,14 +350,6 @@ internal class PluginStatWindow : Window
             }
         }
 
-        if (ImGui.IsWindowAppearing())
-        {
-            foreach (var guid in toRemove)
-            {
-                HookManager.TrackedHooks.TryRemove(guid, out _);
-            }
-        }
-
         ImGui.EndTabBar();
     }
 }

Dalamud/Utility/WeakConcurrentCollection.cs

@@ -0,0 +1,44 @@
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
+using System.Runtime.CompilerServices;
+
+namespace Dalamud.Utility;
+
+/// <summary>
+/// An implementation of a weak concurrent set based on a <see cref="ConditionalWeakTable{TKey,TValue}"/>.
+/// </summary>
+/// <typeparam name="T">The type of object that we're tracking.</typeparam>
+public class WeakConcurrentCollection<T> : ICollection<T> where T : class
+{
+    private readonly ConditionalWeakTable<T, object> cwt = new();
+    
+    /// <inheritdoc/>
+    public int Count => this.cwt.Count();
+
+    /// <inheritdoc/>
+    public bool IsReadOnly => false;
+    
+    private IEnumerable<T> Keys => this.cwt.Select(pair => pair.Key);
+
+    /// <inheritdoc/>
+    public IEnumerator<T> GetEnumerator() => this.cwt.Select(pair => pair.Key).GetEnumerator();
+
+    /// <inheritdoc/>
+    IEnumerator IEnumerable.GetEnumerator() => this.cwt.Select(pair => pair.Key).GetEnumerator();
+
+    /// <inheritdoc/>
+    public void Add(T item) => this.cwt.AddOrUpdate(item, null);
+    
+    /// <inheritdoc/>
+    public void Clear() => this.cwt.Clear();
+
+    /// <inheritdoc/>
+    public bool Contains(T item) => this.Keys.Contains(item);
+
+    /// <inheritdoc/>
+    public void CopyTo(T[] array, int arrayIndex) => this.Keys.ToArray().CopyTo(array, arrayIndex);
+
+    /// <inheritdoc/>
+    public bool Remove(T item) => this.cwt.Remove(item);
+}