From df1cdff1a53919d879f9bc8dbcf8425219cb0a16 Mon Sep 17 00:00:00 2001 From: MidoriKami <9083275+MidoriKami@users.noreply.github.com> Date: Sat, 16 Dec 2023 12:01:40 -0800 Subject: [PATCH] AddonEventManager fix thread safety (#1576) Co-authored-by: goat <16760685+goaaats@users.noreply.github.com> --- .../Game/Addon/Events/AddonEventManager.cs | 33 +++++-- .../Game/Addon/Lifecycle/AddonLifecycle.cs | 89 +++++++------------ 2 files changed, 56 insertions(+), 66 deletions(-) diff --git a/Dalamud/Game/Addon/Events/AddonEventManager.cs b/Dalamud/Game/Addon/Events/AddonEventManager.cs index 23f3b1a6d..af713a771 100644 --- a/Dalamud/Game/Addon/Events/AddonEventManager.cs +++ b/Dalamud/Game/Addon/Events/AddonEventManager.cs @@ -9,6 +9,8 @@ using Dalamud.IoC.Internal; using Dalamud.Logging.Internal; using Dalamud.Plugin.Internal.Types; using Dalamud.Plugin.Services; +using Dalamud.Utility; + using FFXIVClientStructs.FFXIV.Client.UI; using FFXIVClientStructs.FFXIV.Component.GUI; @@ -31,6 +33,9 @@ internal unsafe class AddonEventManager : IDisposable, IServiceType [ServiceManager.ServiceDependency] private readonly AddonLifecycle addonLifecycle = Service.Get(); + [ServiceManager.ServiceDependency] + private readonly Framework framework = Service.Get(); + private readonly AddonLifecycleEventListener finalizeEventListener; private readonly AddonEventManagerAddressResolver address; @@ -87,6 +92,8 @@ internal unsafe class AddonEventManager : IDisposable, IServiceType /// IAddonEventHandle used to remove the event. internal IAddonEventHandle? AddEvent(string pluginId, IntPtr atkUnitBase, IntPtr atkResNode, AddonEventType eventType, IAddonEventManager.AddonEventHandler eventHandler) { + if (!ThreadSafety.IsMainThread) throw new InvalidOperationException("This should be done only from the main thread. Modifying active native code on non-main thread is not supported."); + if (this.pluginEventControllers.FirstOrDefault(entry => entry.PluginId == pluginId) is { } eventController) { return eventController.AddEvent(atkUnitBase, atkResNode, eventType, eventHandler); @@ -103,6 +110,8 @@ internal unsafe class AddonEventManager : IDisposable, IServiceType /// The Unique Id for this event. internal void RemoveEvent(string pluginId, IAddonEventHandle eventHandle) { + if (!ThreadSafety.IsMainThread) throw new InvalidOperationException("This should be done only from the main thread. Modifying active native code on non-main thread is not supported."); + if (this.pluginEventControllers.FirstOrDefault(entry => entry.PluginId == pluginId) is { } eventController) { eventController.RemoveEvent(eventHandle); @@ -130,11 +139,14 @@ internal unsafe class AddonEventManager : IDisposable, IServiceType /// Unique ID for this plugin. internal void AddPluginEventController(string pluginId) { - if (this.pluginEventControllers.All(entry => entry.PluginId != pluginId)) + this.framework.RunOnFrameworkThread(() => { - Log.Verbose($"Creating new PluginEventController for: {pluginId}"); - this.pluginEventControllers.Add(new PluginEventController(pluginId)); - } + if (this.pluginEventControllers.All(entry => entry.PluginId != pluginId)) + { + Log.Verbose($"Creating new PluginEventController for: {pluginId}"); + this.pluginEventControllers.Add(new PluginEventController(pluginId)); + } + }); } /// @@ -143,12 +155,15 @@ internal unsafe class AddonEventManager : IDisposable, IServiceType /// Unique ID for this plugin. internal void RemovePluginEventController(string pluginId) { - if (this.pluginEventControllers.FirstOrDefault(entry => entry.PluginId == pluginId) is { } controller) + this.framework.RunOnFrameworkThread(() => { - Log.Verbose($"Removing PluginEventController for: {pluginId}"); - this.pluginEventControllers.Remove(controller); - controller.Dispose(); - } + if (this.pluginEventControllers.FirstOrDefault(entry => entry.PluginId == pluginId) is { } controller) + { + Log.Verbose($"Removing PluginEventController for: {pluginId}"); + this.pluginEventControllers.Remove(controller); + controller.Dispose(); + } + }); } /// diff --git a/Dalamud/Game/Addon/Lifecycle/AddonLifecycle.cs b/Dalamud/Game/Addon/Lifecycle/AddonLifecycle.cs index 6288cd2cd..beaab7fcd 100644 --- a/Dalamud/Game/Addon/Lifecycle/AddonLifecycle.cs +++ b/Dalamud/Game/Addon/Lifecycle/AddonLifecycle.cs @@ -38,9 +38,6 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType private readonly Hook onAddonRefreshHook; private readonly CallHook onAddonRequestedUpdateHook; - private readonly ConcurrentBag newEventListeners = new(); - private readonly ConcurrentBag removeEventListeners = new(); - // Note: these can be sourced from ObjectPool of appropriate types instead, but since we don't import that NuGet // package, and these events are always called from the main thread, this is fine. #pragma warning disable CS0618 // Type or member is obsolete @@ -61,8 +58,6 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType // We want value of the function pointer at vFunc[2] this.disallowedReceiveEventAddress = ((nint*)this.address.AtkEventListener)![2]; - - this.framework.Update += this.OnFrameworkUpdate; this.onAddonSetupHook = new CallHook(this.address.AddonSetup, this.OnAddonSetup); this.onAddonSetup2Hook = new CallHook(this.address.AddonSetup2, this.OnAddonSetup); @@ -106,8 +101,6 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType /// public void Dispose() { - this.framework.Update -= this.OnFrameworkUpdate; - this.onAddonSetupHook.Dispose(); this.onAddonSetup2Hook.Dispose(); this.onAddonFinalizeHook.Dispose(); @@ -128,7 +121,20 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType /// The listener to register. internal void RegisterListener(AddonLifecycleEventListener listener) { - this.newEventListeners.Add(listener); + this.framework.RunOnTick(() => + { + this.EventListeners.Add(listener); + + // If we want receive event messages have an already active addon, enable the receive event hook. + // If the addon isn't active yet, we'll grab the hook when it sets up. + if (listener is { EventType: AddonEvent.PreReceiveEvent or AddonEvent.PostReceiveEvent }) + { + if (this.ReceiveEventListeners.FirstOrDefault(listeners => listeners.AddonNames.Contains(listener.AddonName)) is { } receiveEventListener) + { + receiveEventListener.Hook?.Enable(); + } + } + }); } /// @@ -137,7 +143,24 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType /// The listener to unregister. internal void UnregisterListener(AddonLifecycleEventListener listener) { - this.removeEventListeners.Add(listener); + this.framework.RunOnTick(() => + { + this.EventListeners.Remove(listener); + + // If we are disabling an ReceiveEvent listener, check if we should disable the hook. + if (listener is { EventType: AddonEvent.PreReceiveEvent or AddonEvent.PostReceiveEvent }) + { + // Get the ReceiveEvent Listener for this addon + if (this.ReceiveEventListeners.FirstOrDefault(listeners => listeners.AddonNames.Contains(listener.AddonName)) is { } receiveEventListener) + { + // If there are no other listeners listening for this event, disable the hook. + if (!this.EventListeners.Any(listeners => listeners.AddonName.Contains(listener.AddonName) && listener.EventType is AddonEvent.PreReceiveEvent or AddonEvent.PostReceiveEvent)) + { + receiveEventListener.Hook?.Disable(); + } + } + } + }); } /// @@ -169,54 +192,6 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType } } - // Used to prevent concurrency issues if plugins try to register during iteration of listeners. - private void OnFrameworkUpdate(IFramework unused) - { - if (this.newEventListeners.Any()) - { - foreach (var toAddListener in this.newEventListeners) - { - this.EventListeners.Add(toAddListener); - - // If we want receive event messages have an already active addon, enable the receive event hook. - // If the addon isn't active yet, we'll grab the hook when it sets up. - if (toAddListener is { EventType: AddonEvent.PreReceiveEvent or AddonEvent.PostReceiveEvent }) - { - if (this.ReceiveEventListeners.FirstOrDefault(listener => listener.AddonNames.Contains(toAddListener.AddonName)) is { } receiveEventListener) - { - receiveEventListener.Hook?.Enable(); - } - } - } - - this.newEventListeners.Clear(); - } - - if (this.removeEventListeners.Any()) - { - foreach (var toRemoveListener in this.removeEventListeners) - { - this.EventListeners.Remove(toRemoveListener); - - // If we are disabling an ReceiveEvent listener, check if we should disable the hook. - if (toRemoveListener is { EventType: AddonEvent.PreReceiveEvent or AddonEvent.PostReceiveEvent }) - { - // Get the ReceiveEvent Listener for this addon - if (this.ReceiveEventListeners.FirstOrDefault(listener => listener.AddonNames.Contains(toRemoveListener.AddonName)) is { } receiveEventListener) - { - // If there are no other listeners listening for this event, disable the hook. - if (!this.EventListeners.Any(listener => listener.AddonName.Contains(toRemoveListener.AddonName) && listener.EventType is AddonEvent.PreReceiveEvent or AddonEvent.PostReceiveEvent)) - { - receiveEventListener.Hook?.Disable(); - } - } - } - } - - this.removeEventListeners.Clear(); - } - } - private void RegisterReceiveEventHook(AtkUnitBase* addon) { // Hook the addon's ReceiveEvent function here, but only enable the hook if we have an active listener.