Core/Collision: Fix race conditions in vmaps

Fix a race condition in VMapManager2 happening whenever a map was unloaded. GridUnload enabled was required to trigger the race condition.

(cherry picked from commit ca7f31c6cdceb7e2921d5d83e1ae22ccb247988d)

4 files changed, 54 insertions, 11 deletions

diff --git a/src/server/collision/Management/VMapFactory.cpp b/src/server/collision/Management/VMapFactory.cpp
index e3e211268e0..4c2750a9e5c 100644
--- a/src/server/collision/Management/VMapFactory.cpp
+++ b/src/server/collision/Management/VMapFactory.cpp
@@ -27,7 +27,7 @@ namespace VMAP
     // just return the instance
     IVMapManager* VMapFactory::createOrGetVMapManager()
     {
-        if (gVMapManager == 0)
+        if (gVMapManager == nullptr)
             gVMapManager= new VMapManager2();                // should be taken from config ... Please change if you like :-)
         return gVMapManager;
     }
diff --git a/src/server/collision/Management/VMapManager2.cpp b/src/server/collision/Management/VMapManager2.cpp
index 0c3ff1a9bfa..b44611e0574 100644
--- a/src/server/collision/Management/VMapManager2.cpp
+++ b/src/server/collision/Management/VMapManager2.cpp
@@ -37,6 +37,7 @@ namespace VMAP
     {
         GetLiquidFlagsPtr = &GetLiquidFlagsDummy;
         IsVMAPDisabledForPtr = &IsVMAPDisabledForDummy;
+        thread_safe_environment = true;
     }
 
     VMapManager2::~VMapManager2(void)
@@ -51,6 +52,15 @@ namespace VMAP
         }
     }
 
+    void VMapManager2::InitializeThreadUnsafe(const std::vector<uint32>& mapIds)
+    {
+        // the caller must pass the list of all mapIds that will be used in the VMapManager2 lifetime
+        for (const uint32& mapId : mapIds)
+            iInstanceMapTrees.insert(InstanceTreeMap::value_type(mapId, nullptr));
+
+        thread_safe_environment = false;
+    }
+
     Vector3 VMapManager2::convertPositionToInternalRep(float x, float y, float z) const
     {
         Vector3 pos;
@@ -86,12 +96,31 @@ namespace VMAP
         return result;
     }
 
+    InstanceTreeMap::const_iterator VMapManager2::GetMapTree(uint32 mapId) const
+    {
+        // return the iterator if found or end() if not found/NULL
+        InstanceTreeMap::const_iterator itr = iInstanceMapTrees.find(mapId);
+        if (itr != iInstanceMapTrees.cend() && !itr->second)
+            itr = iInstanceMapTrees.cend();
+
+        return itr;
+    }
+
     // load one tile (internal use only)
     bool VMapManager2::_loadMap(unsigned int mapId, const std::string& basePath, uint32 tileX, uint32 tileY)
     {
         InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(mapId);
         if (instanceTree == iInstanceMapTrees.end())
         {
+            if (thread_safe_environment)
+                instanceTree = iInstanceMapTrees.insert(InstanceTreeMap::value_type(mapId, nullptr)).first;
+            else
+                ASSERT(false, "Invalid mapId %u tile [%u, %u] passed to VMapManager2 after startup in thread unsafe environment",
+                mapId, tileX, tileY);
+        }
+
+        if (!instanceTree->second)
+        {
             std::string mapFileName = getMapFileName(mapId);
             StaticMapTree* newTree = new StaticMapTree(mapId, basePath);
             if (!newTree->InitMap(mapFileName, this))
@@ -99,7 +128,7 @@ namespace VMAP
                 delete newTree;
                 return false;
             }
-            instanceTree = iInstanceMapTrees.insert(InstanceTreeMap::value_type(mapId, newTree)).first;
+            instanceTree->second = newTree;
         }
 
         return instanceTree->second->LoadMapTile(tileX, tileY, this);
@@ -108,13 +137,13 @@ namespace VMAP
     void VMapManager2::unloadMap(unsigned int mapId)
     {
         InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(mapId);
-        if (instanceTree != iInstanceMapTrees.end())
+        if (instanceTree != iInstanceMapTrees.end() && instanceTree->second)
         {
             instanceTree->second->UnloadMap(this);
             if (instanceTree->second->numLoadedTiles() == 0)
             {
                 delete instanceTree->second;
-                iInstanceMapTrees.erase(mapId);
+                instanceTree->second = nullptr;
             }
         }
     }
@@ -122,13 +151,13 @@ namespace VMAP
     void VMapManager2::unloadMap(unsigned int mapId, int x, int y)
     {
         InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(mapId);
-        if (instanceTree != iInstanceMapTrees.end())
+        if (instanceTree != iInstanceMapTrees.end() && instanceTree->second)
         {
             instanceTree->second->UnloadMapTile(x, y, this);
             if (instanceTree->second->numLoadedTiles() == 0)
             {
                 delete instanceTree->second;
-                iInstanceMapTrees.erase(mapId);
+                instanceTree->second = nullptr;
             }
         }
     }
@@ -138,7 +167,7 @@ namespace VMAP
         if (!isLineOfSightCalcEnabled() || IsVMAPDisabledForPtr(mapId, VMAP_DISABLE_LOS))
             return true;
 
-        InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(mapId);
+        InstanceTreeMap::const_iterator instanceTree = GetMapTree(mapId);
         if (instanceTree != iInstanceMapTrees.end())
         {
             Vector3 pos1 = convertPositionToInternalRep(x1, y1, z1);
@@ -160,7 +189,7 @@ namespace VMAP
     {
         if (isLineOfSightCalcEnabled() && !IsVMAPDisabledForPtr(mapId, VMAP_DISABLE_LOS))
         {
-            InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(mapId);
+            InstanceTreeMap::const_iterator instanceTree = GetMapTree(mapId);
             if (instanceTree != iInstanceMapTrees.end())
             {
                 Vector3 pos1 = convertPositionToInternalRep(x1, y1, z1);
@@ -190,7 +219,7 @@ namespace VMAP
     {
         if (isHeightCalcEnabled() && !IsVMAPDisabledForPtr(mapId, VMAP_DISABLE_HEIGHT))
         {
-            InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(mapId);
+            InstanceTreeMap::const_iterator instanceTree = GetMapTree(mapId);
             if (instanceTree != iInstanceMapTrees.end())
             {
                 Vector3 pos = convertPositionToInternalRep(x, y, z);
@@ -209,7 +238,7 @@ namespace VMAP
     {
         if (!IsVMAPDisabledForPtr(mapId, VMAP_DISABLE_AREAFLAG))
         {
-            InstanceTreeMap::const_iterator instanceTree = iInstanceMapTrees.find(mapId);
+            InstanceTreeMap::const_iterator instanceTree = GetMapTree(mapId);
             if (instanceTree != iInstanceMapTrees.end())
             {
                 Vector3 pos = convertPositionToInternalRep(x, y, z);
@@ -227,7 +256,7 @@ namespace VMAP
     {
         if (!IsVMAPDisabledForPtr(mapId, VMAP_DISABLE_LIQUIDSTATUS))
         {
-            InstanceTreeMap::const_iterator instanceTree = iInstanceMapTrees.find(mapId);
+            InstanceTreeMap::const_iterator instanceTree = GetMapTree(mapId);
             if (instanceTree != iInstanceMapTrees.end())
             {
                 LocationInfo info;
diff --git a/src/server/collision/Management/VMapManager2.h b/src/server/collision/Management/VMapManager2.h
index e13d5ab952b..af04fa0d970 100644
--- a/src/server/collision/Management/VMapManager2.h
+++ b/src/server/collision/Management/VMapManager2.h
@@ -80,6 +80,7 @@ namespace VMAP
             // Tree to check collision
             ModelFileMap iLoadedModelFiles;
             InstanceTreeMap iInstanceMapTrees;
+            bool thread_safe_environment;
             // Mutex for iLoadedModelFiles
             std::mutex LoadedModelFilesLock;
 
@@ -89,6 +90,8 @@ namespace VMAP
             static uint32 GetLiquidFlagsDummy(uint32) { return 0; }
             static bool IsVMAPDisabledForDummy(uint32 /*entry*/, uint8 /*flags*/) { return false; }
 
+            InstanceTreeMap::const_iterator GetMapTree(uint32 mapId) const;
+
         public:
             // public for debug
             G3D::Vector3 convertPositionToInternalRep(float x, float y, float z) const;
@@ -97,6 +100,7 @@ namespace VMAP
             VMapManager2();
             ~VMapManager2(void);
 
+            void InitializeThreadUnsafe(const std::vector<uint32>& mapIds);
             int loadMap(const char* pBasePath, unsigned int mapId, int x, int y) override;
 
             void unloadMap(unsigned int mapId, int x, int y) override;
diff --git a/src/server/game/World/World.cpp b/src/server/game/World/World.cpp
index 7746607d9e6..948729c6d70 100644
--- a/src/server/game/World/World.cpp
+++ b/src/server/game/World/World.cpp
@@ -1471,6 +1471,16 @@ void World::SetInitialWorldSettings()
 
     sSpellMgr->LoadPetFamilySpellsStore();
 
+    if (VMAP::VMapManager2* vmmgr2 = dynamic_cast<VMAP::VMapManager2*>(VMAP::VMapFactory::createOrGetVMapManager()))
+    {
+        std::vector<uint32> mapIds;
+        for (uint32 mapId = 0; mapId < sMapStore.GetNumRows(); mapId++)
+            if (sMapStore.LookupEntry(mapId))
+                mapIds.push_back(mapId);
+
+        vmmgr2->InitializeThreadUnsafe(mapIds);
+    }
+
     TC_LOG_INFO("server.loading", "Loading SpellInfo store...");
     sSpellMgr->LoadSpellInfoStore();