Fix dupe callbacks in VaultAddChildNode

This code called back into the python vault operation thingy, which
deletes itself. So, anytime we got dupe CBs, there was a use-after-free
error. Nasty crashes. I tightened up the CB management, threw away some
weird faux-management mess, and squashed some potential silent bugs.

Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp

@@ -246,18 +246,27 @@ struct VaultAgeInitTrans {
     );
 };
 
-struct AddChildNodeFetchTrans {
+class AddChildNodeTrans {
     FVaultAddChildNodeCallback  callback;
     void *                      cbParam;
     ENetError                   result;
     std::atomic<long>           opCount;
 
-    AddChildNodeFetchTrans()
+    void CompleteOp();
-        : callback(nil), cbParam(nil), result(kNetSuccess), opCount(0) { }
 
-    AddChildNodeFetchTrans(FVaultAddChildNodeCallback _callback, void * _param)
+public:
+    AddChildNodeTrans()
+        : callback(nullptr), cbParam(nullptr), result(kNetSuccess), opCount(0) { }
+
+    AddChildNodeTrans(FVaultAddChildNodeCallback _callback, void * _param)
         : callback(_callback), cbParam(_param), result(kNetSuccess), opCount(0) { }
 
+    void AddOp() { ++opCount; }
+
+    static void VaultNodeAdded (
+        ENetError           result,
+        void *              param
+    );
     static void VaultNodeFetched (
         ENetError           result,
         void *              param,
@@ -994,18 +1003,38 @@ void VaultAgeInitTrans::AgeInitCallback (
 
 /*****************************************************************************
 *
-*   AddChildNodeFetchTrans
+*   AddChildNodeTrans
 *
 ***/
 
 //============================================================================
-void AddChildNodeFetchTrans::VaultNodeRefsFetched (
+void AddChildNodeTrans::CompleteOp() {
+    if ((--opCount) == 0) {
+        if (callback)
+            callback(result, cbParam);
+        delete this; // commit hara-kiri
+    }
+}
+
+//============================================================================
+void AddChildNodeTrans::VaultNodeAdded(
+    ENetError result,
+    void* param
+) {
+    AddChildNodeTrans* trans = static_cast<AddChildNodeTrans*>(param);
+    if (IS_NET_ERROR(result))
+        trans->result = result;
+    trans->CompleteOp();
+}
+
+//============================================================================
+void AddChildNodeTrans::VaultNodeRefsFetched (
     ENetError           result,
     void *              param,
     NetVaultNodeRef *   refs,
     unsigned            refCount
 ) {
-    AddChildNodeFetchTrans * trans = (AddChildNodeFetchTrans *)param;
+    AddChildNodeTrans * trans = (AddChildNodeTrans *)param;
 
     if (IS_NET_ERROR(result)) {
         trans->result       = result;
@@ -1015,45 +1044,27 @@ void AddChildNodeFetchTrans::VaultNodeRefsFetched (
         FetchNodesFromRefs(
             refs,
             refCount,
-            AddChildNodeFetchTrans::VaultNodeFetched,
+            AddChildNodeTrans::VaultNodeFetched,
             param,
             &incFetchCount
         );
         trans->opCount += incFetchCount;
     }
-
+    trans->CompleteOp();
-    // Make the callback now if there are no nodes to fetch, or if error
-    if (!(--trans->opCount)) {
-        if (trans->callback)
-            trans->callback(
-                trans->result,
-                trans->cbParam
-            );
-        delete trans;
-    }
 }
 
 //============================================================================
-void AddChildNodeFetchTrans::VaultNodeFetched (
+void AddChildNodeTrans::VaultNodeFetched (
     ENetError           result,
     void *              param,
     NetVaultNode *      node
 ) {
     ::VaultNodeFetched(result, param, node);
-    
+
-    AddChildNodeFetchTrans * trans = (AddChildNodeFetchTrans *)param;
+    AddChildNodeTrans * trans = (AddChildNodeTrans *)param;
-    
     if (IS_NET_ERROR(result))
         trans->result = result;
-
+    trans->CompleteOp();
-    if (!(--trans->opCount)) {
-        if (trans->callback)
-            trans->callback(
-                trans->result,
-                trans->cbParam
-            );
-        delete trans;
-    }
 }
 
 
@@ -1737,9 +1748,6 @@ void VaultAddChildNode (
     FVaultAddChildNodeCallback  callback,
     void *                      param
 ) {
-    // Make sure we only do the callback once
-    bool madeCallback = false;
-
     // Too much of the client relies on the assumption that the node will be immediately
     // associated with its parent.  THIS SUCKS, because there's no way to guarantee the
     // association won't be circular (the db checks this in a comprehensive way).
@@ -1748,14 +1756,14 @@ void VaultAddChildNode (
     // This directly affects: New clothing items added to the avatar outfit folder,
     // new chronicle entries in some ages, and I'm sure several other situations.
 
+    AddChildNodeTrans * trans = nullptr;
     if (RelVaultNodeLink * parentLink = s_nodes.Find(parentId)) {
         RelVaultNodeLink * childLink = s_nodes.Find(childId);
         if (!childLink) {
             childLink = new RelVaultNodeLink(false, ownerId, childId, new RelVaultNode);
             childLink->node->SetNodeId_NoDirty(childId);
             s_nodes.Add(childLink);
-        }
+        } else if (ownerId) {
-        else if (ownerId) {
             childLink->ownerId = ownerId;
         }
 
@@ -1770,14 +1778,17 @@ void VaultAddChildNode (
             // callback now with error code
             if (callback)
                 callback(kNetErrCircularReference, param);
-        }
+            // I don't really care what Eric thinks, if we believe it's a circular reference, it's evil
-        else if (childLink->node->IsParentOf(parentId, 255)) {
+            // to report a failure, then send it to the server anyway! That's just a bug WAITING
+            // to be uncovered, and I don't want to  deal with it!
+            return;
+        } else if (childLink->node->IsParentOf(parentId, 255)) {
             LogMsg(kLogDebug, L"Node relationship would be circular: p:%u, c:%u", parentId, childId);
             // callback now with error code
             if (callback)
                 callback(kNetErrCircularReference, param);
-        }
+            return;
-        else {
+        } else {
             NetVaultNodeRef refs[] = {
                 { parentId, childId, ownerId }
             };
@@ -1786,66 +1797,52 @@ void VaultAddChildNode (
             ARRAY(unsigned) existingNodeIds;
 
             BuildNodeTree(refs, arrsize(refs), &newNodeIds, &existingNodeIds);
-        
+
+            trans = new AddChildNodeTrans(callback, param);
             if (!childLink->node->GetNodeType() || !parentLink->node->GetNodeType()) {
                 // One or more nodes need to be fetched before the callback is made
-                AddChildNodeFetchTrans * trans = new AddChildNodeFetchTrans(callback, param);
                 if (!childLink->node->GetNodeType()) {
-                    ++trans->opCount;
+                    trans->AddOp();
                     NetCliAuthVaultNodeFetch(
                         childId,
-                        AddChildNodeFetchTrans::VaultNodeFetched,
+                        AddChildNodeTrans::VaultNodeFetched,
                         trans
                     );
-                    ++trans->opCount;
+                    trans->AddOp();
                     NetCliAuthVaultFetchNodeRefs(
                         childId,
-                        AddChildNodeFetchTrans::VaultNodeRefsFetched,
+                        AddChildNodeTrans::VaultNodeRefsFetched,
                         trans
                     );
                 }
                 if (!parentLink->node->GetNodeType()) {
-                    ++trans->opCount;
+                    trans->AddOp();
                     NetCliAuthVaultNodeFetch(
                         parentId,
-                        AddChildNodeFetchTrans::VaultNodeFetched,
+                        AddChildNodeTrans::VaultNodeFetched,
                         trans
                     );
-                    ++trans->opCount;
+                    trans->AddOp();
                     NetCliAuthVaultFetchNodeRefs(
                         parentId,
-                        AddChildNodeFetchTrans::VaultNodeRefsFetched,
+                        AddChildNodeTrans::VaultNodeRefsFetched,
                         trans
                     );
                 }
             }
-            else {
-                // We have both nodes already, so make the callback now.
-                if (callback) {
-                    callback(kNetSuccess, param);
-                    madeCallback = true;
-                }
-            }
-        }
-    }
-    else {
-        // Parent doesn't exist locally (and we may not want it to), just make the callback now.
-        if (callback) {
-            callback(kNetSuccess, param);
-            madeCallback = true;
         }
     }
 
-    // Send it on up to the vault. The db server filters out duplicate and
+    // Send it off to the vault. Note that we're reusing the old fetch trans's opCount junk
-    // circular node relationships. We send the request up even if we think
+    // to ensure that we only ever callback once. May Jesus be with us all.
-    // the relationship would be circular since the db does a universal
+    if (trans)
-    // check and is the only real authority in this matter.
+        trans->AddOp();
     NetCliAuthVaultNodeAdd(
         parentId,
         childId,
         ownerId,
-        madeCallback ? nil : callback,
+        trans ? AddChildNodeTrans::VaultNodeAdded : callback,
-        madeCallback ? nil : param
+        trans ? trans : param
     );
 }