Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Attempt to better handle hook disposal #1803

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions Dalamud/Hooking/AsmHook.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -44,7 +46,7 @@ public AsmHook(IntPtr address, byte[] assembly, string name, AsmHookBehaviour as
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>
Expand All @@ -70,7 +72,7 @@ public AsmHook(IntPtr address, string[] assembly, string name, AsmHookBehaviour
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>
Expand Down Expand Up @@ -116,6 +118,8 @@ public void Dispose()

this.IsDisposed = true;

HookManager.TrackedHooks.TryRemove(this.hookId, out _);

if (this.isEnabled)
{
this.isEnabled = false;
Expand Down
5 changes: 5 additions & 0 deletions Dalamud/Hooking/Hook.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ public T OriginalDisposeSafe
/// <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>
Expand Down
4 changes: 3 additions & 1 deletion Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ internal FunctionPointerVariableHook(IntPtr address, T detour, Assembly callingA

unhooker.TrimAfterHook();

HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, detour, callingAssembly));
HookManager.TrackedHooks.TryAdd(this.HookId, new HookInfo(this, detour, callingAssembly));
}
}

Expand Down Expand Up @@ -137,6 +137,8 @@ public override void Dispose()

this.Disable();

HookManager.TrackedHooks.TryRemove(this.HookId, out _);

var index = HookManager.MultiHookTracker[this.Address].IndexOf(this);
HookManager.MultiHookTracker[this.Address][index] = null;

Expand Down
6 changes: 3 additions & 3 deletions Dalamud/Hooking/Internal/GameInteropProviderPluginScoped.cs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
using System.Collections.Concurrent;
using System.Diagnostics;
using System.Diagnostics;
using System.Linq;

using Dalamud.Game;
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;

Expand All @@ -26,7 +26,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.
Expand Down
4 changes: 3 additions & 1 deletion Dalamud/Hooking/Internal/MinHookHook.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ internal MinHookHook(IntPtr address, T detour, Assembly callingAssembly)

unhooker.TrimAfterHook();

HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, detour, callingAssembly));
HookManager.TrackedHooks.TryAdd(this.HookId, new HookInfo(this, detour, callingAssembly));
}
}

Expand Down Expand Up @@ -76,6 +76,8 @@ public override void Dispose()
HookManager.MultiHookTracker[this.Address][index] = null;
}

HookManager.TrackedHooks.TryRemove(this.HookId, out _);

base.Dispose();
}

Expand Down
4 changes: 3 additions & 1 deletion Dalamud/Hooking/Internal/ReloadedHook.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ internal ReloadedHook(IntPtr address, T detour, Assembly callingAssembly)

unhooker.TrimAfterHook();

HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, detour, callingAssembly));
HookManager.TrackedHooks.TryAdd(this.HookId, new HookInfo(this, detour, callingAssembly));
}
}

Expand Down Expand Up @@ -63,6 +63,8 @@ public override void Dispose()
if (this.IsDisposed)
return;

HookManager.TrackedHooks.TryRemove(this.HookId, out _);

this.Disable();

base.Dispose();
Expand Down
13 changes: 0 additions & 13 deletions Dalamud/Interface/Internal/Windows/PluginStatWindow.cs
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,6 @@ public override void Draw()
ImGui.EndTabItem();
}

var toRemove = new List<Guid>();

if (ImGui.BeginTabItem("Hooks"))
{
ImGui.Checkbox("Show Dalamud Hooks", ref this.showDalamudHooks);
Expand Down Expand Up @@ -291,9 +289,6 @@ public override void Draw()
{
try
{
if (trackedHook.Hook.IsDisposed)
toRemove.Add(guid);

if (!this.showDalamudHooks && trackedHook.Assembly == Assembly.GetExecutingAssembly())
continue;

Expand Down Expand Up @@ -355,14 +350,6 @@ public override void Draw()
}
}

if (ImGui.IsWindowAppearing())
{
foreach (var guid in toRemove)
{
HookManager.TrackedHooks.TryRemove(guid, out _);
}
}

ImGui.EndTabBar();
}
}
44 changes: 44 additions & 0 deletions Dalamud/Utility/WeakConcurrentCollection.cs
Original file line number Diff line number Diff line change
@@ -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);
}
Loading