From a2124bb73dde7f8eeb86f50c5c72d7413fc9e893 Mon Sep 17 00:00:00 2001 From: KazWolfe Date: Thu, 13 Mar 2025 14:16:28 -0700 Subject: [PATCH] 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 | 8 +++- Dalamud/Hooking/Hook.cs | 5 +++ .../Internal/FunctionPointerVariableHook.cs | 4 +- .../GameInteropProviderPluginScoped.cs | 6 +-- Dalamud/Hooking/Internal/MinHookHook.cs | 4 +- Dalamud/Hooking/Internal/ReloadedHook.cs | 4 +- .../Internal/Windows/PluginStatWindow.cs | 13 ------ Dalamud/Utility/WeakConcurrentCollection.cs | 44 +++++++++++++++++++ 8 files changed, 67 insertions(+), 21 deletions(-) create mode 100644 Dalamud/Utility/WeakConcurrentCollection.cs diff --git a/Dalamud/Hooking/AsmHook.cs b/Dalamud/Hooking/AsmHook.cs index 4ee96bb15..f1ed7fd11 100644 --- a/Dalamud/Hooking/AsmHook.cs +++ b/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(); /// /// Initializes a new instance of the 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())); } /// @@ -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())); } /// @@ -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; diff --git a/Dalamud/Hooking/Hook.cs b/Dalamud/Hooking/Hook.cs index da65fedc7..20796bbf0 100644 --- a/Dalamud/Hooking/Hook.cs +++ b/Dalamud/Hooking/Hook.cs @@ -71,6 +71,11 @@ public abstract class Hook : IDalamudHook where T : Delegate /// public virtual string BackendName => throw new NotImplementedException(); + /// + /// Gets the unique GUID for this hook. + /// + protected Guid HookId { get; } = Guid.NewGuid(); + /// /// Remove a hook from the current process. /// diff --git a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs index 7a177206f..40a33fc1b 100644 --- a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs +++ b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs @@ -100,7 +100,7 @@ internal class FunctionPointerVariableHook : Hook 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 : Hook this.Disable(); + HookManager.TrackedHooks.TryRemove(this.HookId, out _); + var index = HookManager.MultiHookTracker[this.Address].IndexOf(this); HookManager.MultiHookTracker[this.Address][index] = null; diff --git a/Dalamud/Hooking/Internal/GameInteropProviderPluginScoped.cs b/Dalamud/Hooking/Internal/GameInteropProviderPluginScoped.cs index f7c4db00a..52d266335 100644 --- a/Dalamud/Hooking/Internal/GameInteropProviderPluginScoped.cs +++ b/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 trackedHooks = new(); + private readonly WeakConcurrentCollection trackedHooks = new(); /// /// Initializes a new instance of the class. diff --git a/Dalamud/Hooking/Internal/MinHookHook.cs b/Dalamud/Hooking/Internal/MinHookHook.cs index 4cebca0d0..0305f3c84 100644 --- a/Dalamud/Hooking/Internal/MinHookHook.cs +++ b/Dalamud/Hooking/Internal/MinHookHook.cs @@ -35,7 +35,7 @@ internal class MinHookHook : Hook 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 : Hook where T : Delegate HookManager.MultiHookTracker[this.Address][index] = null; } + HookManager.TrackedHooks.TryRemove(this.HookId, out _); + base.Dispose(); } diff --git a/Dalamud/Hooking/Internal/ReloadedHook.cs b/Dalamud/Hooking/Internal/ReloadedHook.cs index bf441a86d..2b0a4e9ce 100644 --- a/Dalamud/Hooking/Internal/ReloadedHook.cs +++ b/Dalamud/Hooking/Internal/ReloadedHook.cs @@ -30,7 +30,7 @@ internal class ReloadedHook : Hook 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 : Hook where T : Delegate if (this.IsDisposed) return; + HookManager.TrackedHooks.TryRemove(this.HookId, out _); + this.Disable(); base.Dispose(); diff --git a/Dalamud/Interface/Internal/Windows/PluginStatWindow.cs b/Dalamud/Interface/Internal/Windows/PluginStatWindow.cs index 314f023da..eeafa98e7 100644 --- a/Dalamud/Interface/Internal/Windows/PluginStatWindow.cs +++ b/Dalamud/Interface/Internal/Windows/PluginStatWindow.cs @@ -258,8 +258,6 @@ internal class PluginStatWindow : Window ImGui.EndTabItem(); } - var toRemove = new List(); - 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(); } } diff --git a/Dalamud/Utility/WeakConcurrentCollection.cs b/Dalamud/Utility/WeakConcurrentCollection.cs new file mode 100644 index 000000000..a3bc04651 --- /dev/null +++ b/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; + +/// +/// An implementation of a weak concurrent set based on a . +/// +/// The type of object that we're tracking. +public class WeakConcurrentCollection : ICollection where T : class +{ + private readonly ConditionalWeakTable cwt = new(); + + /// + public int Count => this.cwt.Count(); + + /// + public bool IsReadOnly => false; + + private IEnumerable Keys => this.cwt.Select(pair => pair.Key); + + /// + public IEnumerator GetEnumerator() => this.cwt.Select(pair => pair.Key).GetEnumerator(); + + /// + IEnumerator IEnumerable.GetEnumerator() => this.cwt.Select(pair => pair.Key).GetEnumerator(); + + /// + public void Add(T item) => this.cwt.AddOrUpdate(item, null); + + /// + public void Clear() => this.cwt.Clear(); + + /// + public bool Contains(T item) => this.Keys.Contains(item); + + /// + public void CopyTo(T[] array, int arrayIndex) => this.Keys.ToArray().CopyTo(array, arrayIndex); + + /// + public bool Remove(T item) => this.cwt.Remove(item); +}