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.

(cherry picked from commit 4cb13af4faff0c1f9dbda2ab6a86882b4e4201a6)

1 files changed, 22 insertions, 4 deletions

diff --git a/src/server/game/Combat/ThreatManager.h b/src/server/game/Combat/ThreatManager.h
index 14ee60aea77..09717ff66fb 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;