From c15cea0c814789888eea5af34042d4d3937df2de Mon Sep 17 00:00:00 2001 From: agatho Date: Wed, 4 Feb 2026 06:00:55 +0100 Subject: [PATCH] fix(session): Add state validation for STATUS_TRANSFER deferred packets Prevents assertion failure in Map::RemovePlayerFromMap (Map.cpp:935) when bot receives CMSG_WORLD_PORT_RESPONSE while in inconsistent state. Root cause: Deferred packet processing for STATUS_TRANSFER packets was calling handlers without validating player state. If a bot is IsInWorld() but NOT IsInGrid(), the handler would trigger the assertion: ASSERT(remove) // fails when remove=false and not in grid The fix adds state validation before processing STATUS_TRANSFER packets: - Checks player exists - Validates player is NOT in world (correct state for transfer) - Logs critical warning if player is in world but not in grid - Skips packet processing to prevent crash Crash context: Map 727 (BG), InstanceId 1, Difficulty 0 Co-Authored-By: Claude Opus 4.5 Signed-off-by: luis --- src/modules/Playerbot/Session/BotSession.cpp | 59 ++++++++++++++++++-- 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/src/modules/Playerbot/Session/BotSession.cpp b/src/modules/Playerbot/Session/BotSession.cpp index fb29d40a3..de42080ec 100644 --- a/src/modules/Playerbot/Session/BotSession.cpp +++ b/src/modules/Playerbot/Session/BotSession.cpp @@ -359,7 +359,7 @@ BotSession::BotSession(uint32 bnetAccountId) bnetAccountId, // Use battlenet account as account ID for now std::string(""), // Empty username (generated by Trinity) bnetAccountId, // BattleNet account ID - std::string(""), // BattleNet account email (new in 11.2.7) + std::string(""), // BattleNet account email (new in 12.0.7) std::shared_ptr(), // No socket (empty shared_ptr for bots) SEC_PLAYER, // Security level EXPANSION_LEVEL_CURRENT, // Current expansion @@ -755,7 +755,7 @@ void BotSession::SendPacket(WorldPacket const* packet, bool forced) _outgoingPackets.push(::std::move(packetCopy)); } -// TrinityCore 11.2: Override for new WorldSession::QueuePacket signature +// TrinityCore 12.0: Override for new WorldSession::QueuePacket signature void BotSession::QueuePacket(WorldPacket&& packet) { // Store the packet in incoming queue @@ -1585,7 +1585,7 @@ void BotSession::ProcessBotPackets() try { // Process packet through WorldSession's standard queue system // This is safe to call without locks - // TrinityCore 11.2: QueuePacket now takes WorldPacket&& instead of WorldPacket* + // TrinityCore 12.0: QueuePacket now takes WorldPacket&& instead of WorldPacket* WorldSession::QueuePacket(std::move(*packet)); } catch (::std::exception const& e) @@ -1692,13 +1692,62 @@ uint32 BotSession::ProcessDeferredPackets() } case STATUS_LOGGEDIN_OR_RECENTLY_LOGGOUT: - case STATUS_TRANSFER: case STATUS_AUTHED: { opHandle->Call(this, *packet); break; } + case STATUS_TRANSFER: + { + // STATUS_TRANSFER packets (like CMSG_WORLD_PORT_RESPONSE) should only be processed + // when the player is in a valid transfer state: BeingTeleportedFar AND NOT in world. + // If player IS in world while BeingTeleportedFar, there's a state inconsistency + // that could cause ASSERT failure in Map::RemovePlayerFromMap (Map.cpp:935) + // when trying to remove from map with player not in grid. + Player* player = GetPlayer(); + if (!player) + { + TC_LOG_WARN("playerbot.packets.deferred", + "Bot {} deferred STATUS_TRANSFER packet {} but player is nullptr", + GetPlayerName(), opHandle->Name); + break; + } + + // Validate transfer state: Should be teleporting far and NOT in world + // (TeleportTo calls RemoveFromWorld before setting the far teleport semaphore) + if (player->IsInWorld()) + { + // State inconsistency detected - player is in world but receiving transfer packet + // This could happen if player was re-added to world between TeleportTo and + // processing this deferred packet. Skip to prevent crash in RemovePlayerFromMap. + TC_LOG_ERROR("playerbot.packets.deferred", + "Bot {} deferred STATUS_TRANSFER packet {} SKIPPED - player in world but " + "receiving transfer packet (IsInWorld={}, IsBeingTeleportedFar={}, Map={})", + GetPlayerName(), opHandle->Name, player->IsInWorld(), + player->IsBeingTeleportedFar(), player->GetMapId()); + + // If player is in world and grid, they're already on a map - don't process transfer + // If player is in world but NOT in grid, that's a critical bug - log and skip + if (!player->IsInGrid()) + { + TC_LOG_ERROR("playerbot.packets.deferred", + "CRITICAL: Bot {} is in world but NOT in grid (Map {}) - skipping " + "STATUS_TRANSFER to prevent assertion failure", + GetPlayerName(), player->GetMapId()); + } + break; + } + + // Player is NOT in world - safe to process transfer packet + opHandle->Call(this, *packet); + + TC_LOG_DEBUG("playerbot.packets.deferred", + " Bot {} executed deferred STATUS_TRANSFER opcode {} on main thread", + GetPlayerName(), opHandle->Name); + break; + } + case STATUS_NEVER: case STATUS_UNHANDLED: case STATUS_IGNORED: @@ -1960,7 +2009,7 @@ void BotSession::HandleBotPlayerLogin(BotLoginQueryHolder const& holder) pCurrChar->LearnDefaultSkills(); pCurrChar->UpdateSkillsForLevel(); // Ensures skill values match current level - // Verify spell learning - in modern WoW (11.2), ALL combat spells are automatic + // Verify spell learning - in modern WoW (12.0), ALL combat spells are automatic // TrinityCore's LearnDefaultSkills() + LearnSpecializationSpells() should teach everything // This call is a safety net that re-calls the native methods in case of timing issues sBotPostLoginConfigurator->ApplyClassSpells(pCurrChar);