AddonEventManager fix thread safety (#1576)

Co-authored-by: goat <16760685+goaaats@users.noreply.github.com>

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<AddonLifecycle>.Get();
 
+    [ServiceManager.ServiceDependency]
+    private readonly Framework framework = Service<Framework>.Get();
+    
     private readonly AddonLifecycleEventListener finalizeEventListener;
     
     private readonly AddonEventManagerAddressResolver address;
@@ -87,6 +92,8 @@ internal unsafe class AddonEventManager : IDisposable, IServiceType
     /// <returns>IAddonEventHandle used to remove the event.</returns>
     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
     /// <param name="eventHandle">The Unique Id for this event.</param>
     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
     /// <param name="pluginId">Unique ID for this plugin.</param>
     internal void AddPluginEventController(string pluginId)
     {
-        if (this.pluginEventControllers.All(entry => entry.PluginId != pluginId))
+        this.framework.RunOnFrameworkThread(() =>
         {
-            Log.Verbose($"Creating new PluginEventController for: {pluginId}");
+            if (this.pluginEventControllers.All(entry => entry.PluginId != pluginId))
-            this.pluginEventControllers.Add(new PluginEventController(pluginId));
+            {
-        }
+                Log.Verbose($"Creating new PluginEventController for: {pluginId}");
+                this.pluginEventControllers.Add(new PluginEventController(pluginId));
+            }
+        });
     }
 
     /// <summary>
@@ -143,12 +155,15 @@ internal unsafe class AddonEventManager : IDisposable, IServiceType
     /// <param name="pluginId">Unique ID for this plugin.</param>
     internal void RemovePluginEventController(string pluginId)
     {
-        if (this.pluginEventControllers.FirstOrDefault(entry => entry.PluginId == pluginId) is { } controller)
+        this.framework.RunOnFrameworkThread(() =>
         {
-            Log.Verbose($"Removing PluginEventController for: {pluginId}");
+            if (this.pluginEventControllers.FirstOrDefault(entry => entry.PluginId == pluginId) is { } controller)
-            this.pluginEventControllers.Remove(controller);
+            {
-            controller.Dispose();
+                Log.Verbose($"Removing PluginEventController for: {pluginId}");
-        }
+                this.pluginEventControllers.Remove(controller);
+                controller.Dispose();
+            }
+        });
     }
 
     /// <summary>

Dalamud/Game/Addon/Lifecycle/AddonLifecycle.cs

@@ -38,9 +38,6 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType
     private readonly Hook<AddonOnRefreshDelegate> onAddonRefreshHook;
     private readonly CallHook<AddonOnRequestedUpdateDelegate> onAddonRequestedUpdateHook;
 
-    private readonly ConcurrentBag<AddonLifecycleEventListener> newEventListeners = new();
-    private readonly ConcurrentBag<AddonLifecycleEventListener> 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<AddonSetupDelegate>(this.address.AddonSetup, this.OnAddonSetup);
         this.onAddonSetup2Hook = new CallHook<AddonSetupDelegate>(this.address.AddonSetup2, this.OnAddonSetup);
@@ -106,8 +101,6 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType
     /// <inheritdoc/>
     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
     /// <param name="listener">The listener to register.</param>
     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();
+                }
+            }
+        });
     }
 
     /// <summary>
@@ -137,7 +143,24 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType
     /// <param name="listener">The listener to unregister.</param>
     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();
+                    }
+                }
+            }
+        });
     }
 
     /// <summary>
@@ -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.