Core/Threat: Custom ThreatListIterator that transparently iterates over the backing map (instead of using heap iterators) when working with the unsorted threat list. This greatly reduces the range of actions that can cause iterator invalidation.

Also some minor adjustments to .debug threat, which no longer hides certain invalid states from view.

2 files changed, 65 insertions, 38 deletions

diff --git a/src/server/game/Combat/ThreatManager.h b/src/server/game/Combat/ThreatManager.h
index d17120392d1..0ac3470d838 100644
--- a/src/server/game/Combat/ThreatManager.h
+++ b/src/server/game/Combat/ThreatManager.h
@@ -69,8 +69,9 @@ class SpellInfo;
  * The current (= last selected) victim can be accessed using GetCurrentVictim. SelectVictim selects a (potentially new) victim.                        *
  * Beyond that, ThreatManager has a variety of helpers and notifiers, which are documented inline below.                                                *
  *                                                                                                                                                      *
- * SPECIAL NOTE: Please be aware that any heap iterator may be invalidated if you modify a ThreatReference. The heap holds const pointers for a reason. *
- *               If you need to modify multiple ThreatReference objects, then use GetModifiableThreatList(), which is safe to modify!                   *
+ * SPECIAL NOTE: Please be aware that any iterator may be invalidated if you modify a ThreatReference. The heap holds const pointers for a reason, but  *
+ *                 that doesn't mean you're scot free. A variety of actions (casting spells, teleporting units, and so forth) can cause changes to      *
+ *                 the threat list. Use with care - or default to GetModifiableThreatList(), which inherently copies entries.                           *
 \********************************************************************************************************************************************************/
 
 class ThreatReference;
@@ -85,6 +86,7 @@ class TC_GAME_API ThreatManager
 {
     public:
         typedef boost::heap::fibonacci_heap<ThreatReference const*, boost::heap::compare<CompareThreatLessThan>> threat_list_heap;
+        class ThreatListIterator;
         static const uint32 CLIENT_THREAT_UPDATE_INTERVAL = 1000u;
 
         static bool CanHaveThreatList(Unit const* who);
@@ -121,11 +123,13 @@ class TC_GAME_API ThreatManager
         float GetThreat(Unit const* who, bool includeOffline = false) const;
         size_t GetThreatListSize() const { return _sortedThreatList.size(); }
         // fastest of the three threat list getters - gets the threat list in "arbitrary" order
-        Trinity::IteratorPair<threat_list_heap::const_iterator> GetUnsortedThreatList() const { return { _sortedThreatList.begin(), _sortedThreatList.end() }; }
+        // iterators will invalidate on adding/removing entries from the threat list; slightly less finicky than GetSorted.
+        Trinity::IteratorPair<ThreatListIterator> GetUnsortedThreatList() const { return { _myThreatListEntries.begin(), _myThreatListEntries.end() }; }
         // slightly slower than GetUnsorted, but, well...sorted - only use it if you need the sorted property, of course
+        // this iterator pair will invalidate on any modification (even indirect) of the threat list; spell casts and similar can all induce this!
         // note: current tank is NOT guaranteed to be the first entry in this list - check GetCurrentVictim separately if you want that!
         Trinity::IteratorPair<threat_list_heap::ordered_iterator> GetSortedThreatList() const { return { _sortedThreatList.ordered_begin(), _sortedThreatList.ordered_end() }; }
-        // slowest of the three threat list getters (by far), but lets you modify the threat references
+        // slowest of the three threat list getters (by far), but lets you modify the threat references - this is also sorted
         std::vector<ThreatReference*> GetModifiableThreatList() const;
 
         // does any unit have a threat list entry with victim == this.owner?
@@ -217,6 +221,20 @@ class TC_GAME_API ThreatManager
         ThreatManager(ThreatManager const&) = delete;
         ThreatManager& operator=(ThreatManager const&) = delete;
 
+        class ThreatListIterator
+        {
+            private:
+                decltype(_myThreatListEntries)::const_iterator _it;
+
+            public:
+                ThreatReference const* operator*() const { return _it->second; }
+                ThreatReference const* operator->() const { return _it->second; }
+                ThreatListIterator& operator++() { ++_it; return *this; }
+                bool operator==(ThreatListIterator const& o) const { return _it == o._it; }
+                bool operator!=(ThreatListIterator const& o) const { return _it != o._it; }
+                ThreatListIterator(decltype(_it) it) : _it(it) {}
+        };
+
     friend class ThreatReference;
     friend struct CompareThreatLessThan;
     friend class debug_commandscript;
diff --git a/src/server/scripts/Commands/cs_debug.cpp b/src/server/scripts/Commands/cs_debug.cpp
index 0a2db46d6a0..56bfd14de66 100644
--- a/src/server/scripts/Commands/cs_debug.cpp
+++ b/src/server/scripts/Commands/cs_debug.cpp
@@ -551,7 +551,7 @@ public:
         if (!target)
             return false;
 
-        handler->PSendSysMessage("Loot recipient for creature %s (GUID %u, DB GUID %u) is %s",
+        handler->PSendSysMessage("Loot recipient for creature %s (GUID %u, SpawnID %u) is %s",
             target->GetName().c_str(), target->GetGUID().GetCounter(), target->GetSpawnId(),
             target->hasLootRecipient() ? (target->GetLootRecipient() ? target->GetLootRecipient()->GetName().c_str() : "offline") : "no loot recipient");
         return true;
@@ -870,50 +870,59 @@ public:
             for (auto const& pair : threatenedByMe)
             {
                 Unit* unit = pair.second->GetOwner();
-                handler->PSendSysMessage("   %u.   %s   (current guid %u, DB guid %u)  - threat %f", ++count, unit->GetName().c_str(), unit->GetGUID().GetCounter(), unit->GetTypeId() == TYPEID_UNIT ? unit->ToCreature()->GetSpawnId() : 0, pair.second->GetThreat());
+                handler->PSendSysMessage("   %u.   %s   (GUID %u, SpawnID %u)  - threat %f", ++count, unit->GetName().c_str(), unit->GetGUID().GetCounter(), unit->GetTypeId() == TYPEID_UNIT ? unit->ToCreature()->GetSpawnId() : 0, pair.second->GetThreat());
             }
             handler->SendSysMessage("End of threatened-by-me list.");
         }
 
-        if (!mgr.CanHaveThreatList())
-            handler->PSendSysMessage("%s (guid %u) cannot have a threat list.", target->GetName().c_str(), target->GetGUID().GetCounter());
-        else if (mgr.IsEngaged())
+        if (mgr.CanHaveThreatList())
         {
-            count = 0;
-            handler->PSendSysMessage("Threat list of %s (guid %u, spawnID %u)", target->GetName().c_str(), target->GetGUID().GetCounter(), target->GetTypeId() == TYPEID_UNIT ? target->ToCreature()->GetSpawnId() : 0);
-            for (ThreatReference const* ref : mgr.GetSortedThreatList())
+            if (!mgr.IsThreatListEmpty(true))
             {
-                Unit* unit = ref->GetVictim();
-                char const* onlineStr;
-                switch (ref->GetOnlineState())
-                {
-                    case ThreatReference::ONLINE_STATE_SUPPRESSED:
-                        onlineStr = " [SUPPRESSED]";
-                        break;
-                    case ThreatReference::ONLINE_STATE_OFFLINE:
-                        onlineStr = " [OFFLINE]";
-                        break;
-                    default:
-                        onlineStr = "";
-                }
-                char const* tauntStr;
-                switch (ref->GetTauntState())
+                if (mgr.IsEngaged())
+                    handler->PSendSysMessage("Threat list of %s (GUID %u, SpawnID %u):", target->GetName().c_str(), target->GetGUID().GetCounter(), target->GetTypeId() == TYPEID_UNIT ? target->ToCreature()->GetSpawnId() : 0);
+                else
+                    handler->PSendSysMessage("%s (GUID %u, SpawnID %u) is not engaged, but still has a threat list? Well, here it is:", target->GetName().c_str(), target->GetGUID().GetCounter(), target->GetTypeId() == TYPEID_UNIT ? target->ToCreature()->GetSpawnId() : 0);
+
+                count = 0;
+                for (ThreatReference const* ref : mgr.GetSortedThreatList())
                 {
-                    case ThreatReference::TAUNT_STATE_TAUNT:
-                        tauntStr = " [TAUNT]";
-                        break;
-                    case ThreatReference::TAUNT_STATE_DETAUNT:
-                        tauntStr = " [DETAUNT]";
-                        break;
-                    default:
-                        tauntStr = "";
+                    Unit* unit = ref->GetVictim();
+                    char const* onlineStr;
+                    switch (ref->GetOnlineState())
+                    {
+                        case ThreatReference::ONLINE_STATE_SUPPRESSED:
+                            onlineStr = " [SUPPRESSED]";
+                            break;
+                        case ThreatReference::ONLINE_STATE_OFFLINE:
+                            onlineStr = " [OFFLINE]";
+                            break;
+                        default:
+                            onlineStr = "";
+                    }
+                    char const* tauntStr;
+                    switch (ref->GetTauntState())
+                    {
+                        case ThreatReference::TAUNT_STATE_TAUNT:
+                            tauntStr = " [TAUNT]";
+                            break;
+                        case ThreatReference::TAUNT_STATE_DETAUNT:
+                            tauntStr = " [DETAUNT]";
+                            break;
+                        default:
+                            tauntStr = "";
+                    }
+                    handler->PSendSysMessage("   %u.   %s   (GUID %u)  - threat %f%s%s", ++count, unit->GetName().c_str(), unit->GetGUID().GetCounter(), ref->GetThreat(), tauntStr, onlineStr);
                 }
-                handler->PSendSysMessage("   %u.   %s   (guid %u)  - threat %f%s%s", ++count, unit->GetName().c_str(), unit->GetGUID().GetCounter(), ref->GetThreat(), tauntStr, onlineStr);
+                handler->SendSysMessage("End of threat list.");
             }
-            handler->SendSysMessage("End of threat list.");
+            else if (!mgr.IsEngaged())
+                handler->PSendSysMessage("%s (GUID %u, SpawnID %u) is not currently engaged.", target->GetName().c_str(), target->GetGUID().GetCounter(), target->GetTypeId() == TYPEID_UNIT ? target->ToCreature()->GetSpawnId() : 0);
+            else
+                handler->PSendSysMessage("%s (GUID %u, SpawnID %u) seems to be engaged, but does not have a threat list??", target->GetName().c_str(), target->GetGUID().GetCounter(), target->GetTypeId() == TYPEID_UNIT ? target->ToCreature()->GetSpawnId() : 0);
         }
         else
-            handler->PSendSysMessage("%s (guid %u, spawnID %u) is not currently engaged.", target->GetName().c_str(), target->GetGUID().GetCounter(), target->GetTypeId() == TYPEID_UNIT ? target->ToCreature()->GetSpawnId() : 0);
+            handler->PSendSysMessage("%s (GUID %u) cannot have a threat list.", target->GetName().c_str(), target->GetGUID().GetCounter());
         return true;
     }