[Core] Improved stability of UnsubscribeAction() by avoiding object p…

…ointer references
parasol-framework · Jul 3, 2023 · 22a120b · 22a120b
1 parent d557087
commit 22a120b
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 12 deletions.
diff --git a/src/core/defs.h b/src/core/defs.h
@@ -234,12 +234,13 @@ class PrivateAddress {
 //********************************************************************************************************************
 
 struct ActionSubscription {
-   OBJECTPTR Context;
+   OBJECTPTR Subscriber; // The object that initiated the subscription
+   OBJECTID SubscriberID; // Required for sanity checks on subscriber still existing.
    void (*Callback)(OBJECTPTR, ACTIONID, ERROR, APTR);
 
-   ActionSubscription() : Context(NULL), Callback(NULL) { }
-   ActionSubscription(OBJECTPTR pContext, void (*pCallback)(OBJECTPTR, ACTIONID, ERROR, APTR)) : Context(pContext), Callback(pCallback) { }
-   ActionSubscription(OBJECTPTR pContext, APTR pCallback) : Context(pContext), Callback((void (*)(OBJECTPTR, ACTIONID, ERROR, APTR))pCallback) { }
+   ActionSubscription() : Subscriber(NULL), SubscriberID(0), Callback(NULL) { }
+   ActionSubscription(OBJECTPTR pContext, void (*pCallback)(OBJECTPTR, ACTIONID, ERROR, APTR)) : Subscriber(pContext), SubscriberID(pContext->UID), Callback(pCallback) { }
+   ActionSubscription(OBJECTPTR pContext, APTR pCallback) : Subscriber(pContext), SubscriberID(pContext->UID), Callback((void (*)(OBJECTPTR, ACTIONID, ERROR, APTR))pCallback) { }
 };
 
 struct virtual_drive {

diff --git a/src/core/lib_objects.cpp b/src/core/lib_objects.cpp
@@ -436,10 +436,12 @@ ERROR Action(LONG ActionID, OBJECTPTR Object, APTR Parameters)
 
       glSubReadOnly++;
 
-      if ((glSubscriptions.contains(Object->UID)) and (glSubscriptions[Object->UID].contains(ActionID))) {
-         for (auto &list : glSubscriptions[Object->UID][ActionID]) {
-            pf::SwitchContext ctx(list.Context);
-            list.Callback(Object, ActionID, (error IS ERR_NoAction) ? ERR_Okay : error, Parameters);
+      if (auto it = glSubscriptions.find(Object->UID); it != glSubscriptions.end()) {
+         if (it->second.contains(ActionID)) {
+            for (auto &list : it->second[ActionID]) {
+               pf::SwitchContext ctx(list.Subscriber);
+               list.Callback(Object, ActionID, (error IS ERR_NoAction) ? ERR_Okay : error, Parameters);
+            }
          }
       }
 
@@ -1413,8 +1415,8 @@ void NotifySubscribers(OBJECTPTR Object, LONG ActionID, APTR Parameters, ERROR E
    if ((!glSubscriptions[Object->UID].empty()) and (!glSubscriptions[Object->UID][ActionID].empty())) {
       glSubReadOnly++; // Prevents changes to glSubscriptions while we're processing it.
       for (auto &sub : glSubscriptions[Object->UID][ActionID]) {
-         if (sub.Context) {
-            pf::SwitchContext ctx(sub.Context);
+         if (sub.Subscriber) {
+            pf::SwitchContext ctx(sub.Subscriber);
             sub.Callback(Object, ActionID, ErrorCode, Parameters);
          }
       }
@@ -1918,7 +1920,7 @@ ERROR UnsubscribeAction(OBJECTPTR Object, ACTIONID ActionID)
 restart:
          for (auto & [action, list] : glSubscriptions[Object->UID]) {
             for (auto it = list.begin(); it != list.end(); ) {
-               if (it->Context->UID IS subscriber) it = list.erase(it);
+               if (it->SubscriberID IS subscriber) it = list.erase(it);
                else it++;
             }
 
@@ -1942,7 +1944,7 @@ ERROR UnsubscribeAction(OBJECTPTR Object, ACTIONID ActionID)
 
       auto &list = glSubscriptions[Object->UID][ActionID];
       for (auto it = list.begin(); it != list.end(); ) {
-         if (it->Context->UID IS subscriber) it = list.erase(it);
+         if (it->SubscriberID IS subscriber) it = list.erase(it);
          else it++;
       }