FGESEventListener.ReceiverWCO is not thread-safe

[PhDittmann]

'Source/GlobalEventSystem/Public/GESHandlerDataTypes.h' defines UObject* FGESMinimalEventListener.ReceiverWCO, which points to UObjects in the scene, but Listener.ReceiverWCO->IsValidLowLevelFast() seems to fail regarding deleted objects.

Spawning and deleting objects rapidly while triggering events leads to crashes:

Unhandled Exception: EXCEPTION_ACCESS_VIOLATION 0x0000008000000000
UnrealEditor_GlobalEventSystem!<lambda_62c3eddd4239e5a48e9e4bb3fd487051>::operator()() [...\Plugins\GlobalEventSystem-Unreal-0.12.0\Source\GlobalEventSystem\Private\GESHandler.cpp:907]
UnrealEditor_GlobalEventSystem!FGESHandler::EmitToListenerWithData() [...\Plugins\GlobalEventSystem-Unreal-0.12.0\Source\GlobalEventSystem\Private\GESHandler.cpp:552]
UnrealEditor_GlobalEventSystem!FGESHandler::EmitToListenersWithData() [...\Plugins\GlobalEventSystem-Unreal-0.12.0\Source\GlobalEventSystem\Private\GESHandler.cpp:492]
UnrealEditor_GlobalEventSystem!FGESHandler::EmitEvent() [...\Plugins\GlobalEventSystem-Unreal-0.12.0\Source\GlobalEventSystem\Private\GESHandler.cpp:842]
UnrealEditor_GlobalEventSystem!UGlobalEventSystemBPLibrary::GESEmitEvent() [...\Plugins\GlobalEventSystem-Unreal-0.12.0\Source\GlobalEventSystem\Private\GlobalEventSystemBPLibrary.cpp:131]
UnrealEditor_GlobalEventSystem!UGlobalEventSystemBPLibrary::execGESEmitEvent() [...\Plugins\GlobalEventSystem-Unreal-0.12.0\Intermediate\Build\Win64\UnrealEditor\Inc\GlobalEventSystem\UHT\GlobalEventSystemBPLibrary.gen.cpp:132]
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_CoreUObject
UnrealEditor_Engine
UnrealEditor_Engine
UnrealEditor_Engine
UnrealEditor_Engine
UnrealEditor_Engine
UnrealEditor_Engine
UnrealEditor_Engine
UnrealEditor_Core
UnrealEditor_Core
UnrealEditor_Core
UnrealEditor_Engine
UnrealEditor_Engine
UnrealEditor_Engine
UnrealEditor_Engine
UnrealEditor_UnrealEd
UnrealEditor_UnrealEd
UnrealEditor
UnrealEditor
UnrealEditor
UnrealEditor
UnrealEditor
UnrealEditor
kernel32
ntdll

In a first try I changed ReceiverWCO to a TWeakObjectPtr, which does not extend the lifetime of an object, but gets properly notified, when it gets deleted:

diff --git a/GlobalEventSystem-Unreal-0.12.0/Source/GlobalEventSystem/Private/GESHandler.cpp b/GlobalEventSystem-Unreal-0.12.0/Source/GlobalEventSystem/Private/GESHandler.cpp
index 1d82c3a..f0c8920 100644
--- a/GlobalEventSystem-Unreal-0.12.0/Source/GlobalEventSystem/Private/GESHandler.cpp
+++ b/GlobalEventSystem-Unreal-0.12.0/Source/GlobalEventSystem/Private/GESHandler.cpp
@@ -35,7 +35,7 @@ bool FGESHandler::FirstParamIsSubclassOf(UFunction* Function, FFieldClass* Class
 
 FString FGESHandler::ListenerLogString(const FGESEventListener& Listener)
 {
-       return Listener.ReceiverWCO->GetName() + TEXT(":") + Listener.FunctionName;
+       return Listener.ReceiverWCO.Get()->GetName() + TEXT(":") + Listener.FunctionName;
 }
 
 FString FGESHandler::EventLogString(const FGESEvent& Event)
@@ -154,12 +154,12 @@ void FGESHandler::AddListener(const FString& Domain, const FString& EventName, c
                Minimal.ReceiverWCO = Listener.ReceiverWCO;
                ListenContext.Listener = Minimal;
 
-               if (!ReceiverMap.Contains(Listener.ReceiverWCO))
+               if (!ReceiverMap.Contains(Listener.ReceiverWCO.Get()))
                {
                        TArray<FGESEventListenerWithContext> Array;
-                       ReceiverMap.Add(Listener.ReceiverWCO, Array);
+                       ReceiverMap.Add(Listener.ReceiverWCO.Get(), Array);
                }
-               ReceiverMap[Listener.ReceiverWCO].Add(ListenContext);
+               ReceiverMap[Listener.ReceiverWCO.Get()].Add(ListenContext);
 
                //if it's pinned re-emit it immediately to this listener
                if (Event.bPinned) 
@@ -333,14 +333,14 @@ void FGESHandler::RemoveListener(const FString& Domain, const FString& Event, co
        EventMap[KeyString].Listeners.Remove(Listener);
 
        //Remove matched entry in receiver map
-       if (ReceiverMap.Contains(Listener.ReceiverWCO))
+       if (ReceiverMap.Contains(Listener.ReceiverWCO.Get()))
        {
                FGESEventListenerWithContext ContextListener;
                ContextListener.Domain = Domain;
                ContextListener.Event = Event;
                ContextListener.Listener.FunctionName = Listener.FunctionName;
                ContextListener.Listener.ReceiverWCO = Listener.ReceiverWCO;
-               ReceiverMap[Listener.ReceiverWCO].Remove(ContextListener);
+               ReceiverMap[Listener.ReceiverWCO.Get()].Remove(ContextListener);
        }
 }
 
diff --git a/GlobalEventSystem-Unreal-0.12.0/Source/GlobalEventSystem/Public/GESHandlerDataTypes.h b/GlobalEventSystem-Unreal-0.12.0/Source/GlobalEventSystem/Public/GESHandlerDataTypes.h
index d7abac6..285baf1 100644
--- a/GlobalEventSystem-Unreal-0.12.0/Source/GlobalEventSystem/Public/GESHandlerDataTypes.h
+++ b/GlobalEventSystem-Unreal-0.12.0/Source/GlobalEventSystem/Public/GESHandlerDataTypes.h
@@ -32,7 +32,7 @@ struct FGESDynamicArg
 //Minimal definition to define a listener (for removal)
 struct FGESMinimalEventListener
 {
-       UObject* ReceiverWCO;   //WorldContextObject
+       TWeakObjectPtr<UObject> ReceiverWCO;    //WorldContextObject
        FString FunctionName;
 
        bool operator ==(FGESMinimalEventListener const& Other)

With these changes I'm not able to reproduce the crashes, but I'm sure if it's really thread-safe or the way Epic intended. Here are some resources I found about this topic:

• https://unrealcommunity.wiki/memory-management-6rlf3v4i
• https://forums.unrealengine.com/t/discussion-the-ab-use-of-isvalidlowlevel/21795
• https://docs.unrealengine.com/5.3/en-US/weak-pointers-in-unreal-engine/

[getnamo]

GES should only be called on game thread based on current design api. You're right however that the current implementation uses raw pointers a bit too liberally and thus can't properly detect when they are invalidated due to a garbage collection cycle. A refactor to smart pointers would likely alleviate this.

Atm you can typically guarantee pointer sanity by balancing calls with Unbind All Events for Context, but this is less than ideal API imo. We should be able to auto-detect invalid receivers on emit and cleanup (which is in theory how it currently works, but raw pointers...).

[getnamo]

If you believe your changes above are all that are needed for the refactor, feel free to make a pull request, I'll run some tests and merge.