5 files changed, 57 insertions, 31 deletions

diff --git a/src/ClientHandle.cpp b/src/ClientHandle.cpp
index 8ae81f645..f4aa3e0c5 100644
--- a/src/ClientHandle.cpp
+++ b/src/ClientHandle.cpp
@@ -104,24 +104,7 @@ cClientHandle::~cClientHandle()
 {
 	ASSERT(m_State == eState::csDestroyed);  // Has Destroy() been called?
 
-	if (m_Player != nullptr)
-	{
-		cWorld * World = m_Player->GetWorld();
-
-		// Upon clienthandle destruction, the world pointer of a valid player SHALL NOT be null.
-		// The only time where it can be null is after player construction but before cEntity::Initialize is called in the queued task in Authenticate.
-		// During this time, it is guaranteed that the clienthandle is not deleted.
-		ASSERT(World != nullptr);
-
-		World->QueueTask(
-			[this](cWorld & a_World)
-			{
-				a_World.RemovePlayer(m_Player);
-				a_World.BroadcastPlayerListRemovePlayer(*m_Player);
-				m_Player->Destroy();
-			}
-		);
-	}
+	// Note: don't handle player destruction here because we don't own them, so problems arise during shutdown
 
 	LOGD("Deletied client \"%s\" at %p", GetUsername().c_str(), static_cast<void *>(this));
 }
@@ -144,6 +127,38 @@ void cClientHandle::Destroy(void)
 		m_Link.reset();
 	}
 
+	if (m_Player != nullptr)
+	{
+		cWorld * World = m_Player->GetWorld();
+
+		// Upon Destroy, the world pointer of a valid player SHALL NOT be null.
+		// Guaranteed by Destroy and Authenticate being called in the same thread
+		ASSERT(World != nullptr);
+
+		// We do not leak a player object as Authenticate checks to see if Destroy was called before creating a player
+		World->QueueTask(
+			[this](cWorld & a_World)
+			{
+				if (m_Player->GetParentChunk() == nullptr)
+				{
+					// Player not yet initialised; calling Destroy is not valid
+					LOGWARN("ParentChunk null");
+					return;
+				}
+
+				if (!cRoot::Get()->GetPluginManager()->CallHookPlayerDestroyed(*m_Player))
+				{
+					cRoot::Get()->BroadcastChatLeave(Printf("%s has left the game", m_Player->GetName().c_str()));
+					LOGINFO("Player %s has left the game", m_Player->GetName().c_str());
+				}
+
+				a_World.RemovePlayer(m_Player);
+				a_World.BroadcastPlayerListRemovePlayer(*m_Player);
+				m_Player->Destroy();
+			}
+		);
+	}
+
 	RemoveFromWorld();
 
 	LOGD("%s: client %p, \"%s\"", __FUNCTION__, static_cast<void *>(this), m_Username.c_str());
@@ -309,8 +324,11 @@ void cClientHandle::Kick(const AString & a_Reason)
 
 void cClientHandle::Authenticate(const AString & a_Name, const AString & a_UUID, const Json::Value & a_Properties)
 {
+	ASSERT(cRoot::Get()->GetServer()->IsInTickThread());
+
 	if (m_State != eState::csAuthenticating)
 	{
+		// TODO: is this necessary?
 		return;
 	}
 
@@ -348,12 +366,22 @@ void cClientHandle::Authenticate(const AString & a_Name, const AString & a_UUID,
 	}
 	m_Player->SetIP(m_IPString);
 
+	// So Destroy always has a valid world for a valid player
+	// Additionally, plugins expect world to be set
+	m_Player->SetWorld(World);  
+
 	cpp14::move_on_copy_wrapper<decltype(Player)> PlayerPtr(std::move(Player));
 	World->QueueTask(
 		[World, Player = m_Player, PlayerPtr, Client](cWorld & a_World) mutable
 		{
-			// Plugins expect world to be set:
-			Player->SetWorld(World);
+			// We're in the task to create the player - any other QueueTask will execute in the next tick
+			// So, check to see cClientHandle::Destroy has not been called (State >= IsDestroying where State is monotonic)
+			if (Client->IsDestroying() || Client->IsDestroyed())
+			{
+				// Destroy called - proceeding now means we may create a player object that's never deleted
+				LOGWARN("Destroyed");
+				return;
+			}
 
 			if (!cRoot::Get()->GetPluginManager()->CallHookPlayerJoined(*Player))
 			{
@@ -386,9 +414,7 @@ void cClientHandle::Authenticate(const AString & a_Name, const AString & a_UUID,
 			cRoot::Get()->BroadcastPlayerListsAddPlayer(*Player);
 			cRoot::Get()->SendPlayerLists(Player);
 
-			// Note: cEnttiy::Initialize takes ownership of the player object, however:
-			// 1. It is guaranteed to exist whilst the server is running and this clienthandle is alive
-			// 2. In the case that the server is stopping, it is guaranteed that we are destroyed before the player object is destroyed
+			// Note: cEnttiy::Initialize takes ownership of the player object
 			Player->Initialize(std::move(PlayerPtr.value), *World);
 
 			World->AddPlayer(Player);
diff --git a/src/ClientHandle.h b/src/ClientHandle.h
index 0e60d1f0a..44261827d 100644
--- a/src/ClientHandle.h
+++ b/src/ClientHandle.h
@@ -455,6 +455,8 @@ private:
 	/** Protects m_Link against multithreaded access. */
 	cCriticalSection m_CSLink;
 
+	/** Non-owning pointer to the player object we are associated with
+	It is guaranteed to exist whilst the server is running and this clienthandle is alive */
 	cPlayer * m_Player;
 
 	/** Number of ticks since the last network packet was received (increased in Tick(), reset in OnReceivedData()) */
diff --git a/src/Entities/Player.cpp b/src/Entities/Player.cpp
index babe31978..390e4ad52 100644
--- a/src/Entities/Player.cpp
+++ b/src/Entities/Player.cpp
@@ -158,12 +158,6 @@ cPlayer::cPlayer(std::weak_ptr<cClientHandle> a_Client, const AString & a_Player
 
 cPlayer::~cPlayer(void)
 {
-	if (!cRoot::Get()->GetPluginManager()->CallHookPlayerDestroyed(*this))
-	{
-		cRoot::Get()->BroadcastChatLeave(Printf("%s has left the game", GetName().c_str()));
-		LOGINFO("Player %s has left the game", GetName().c_str());
-	}
-
 	LOGD("Deleting cPlayer \"%s\" at %p, ID %d", GetName().c_str(), static_cast<void *>(this), GetUniqueID());
 
 	// Notify the server that the player is being destroyed
diff --git a/src/Root.cpp b/src/Root.cpp
index ec31e57a2..3749aaf64 100644
--- a/src/Root.cpp
+++ b/src/Root.cpp
@@ -572,7 +572,7 @@ bool cRoot::ForEachWorld(cWorldListCallback & a_Callback)
 {
 	for (const auto & World : m_WorldsByName)
 	{
-		if (a_Callback.Item(World.second.get()))
+		if (a_Callback.Item(World.second))
 		{
 			return false;
 		}
@@ -960,7 +960,7 @@ void cRoot::LogChunkStats(cCommandOutputCallback & a_Output)
 	int SumMem = 0;
 	for (const auto & WorldEntry : m_WorldsByName)
 	{
-		auto World = WorldEntry.second.get();
+		auto World = WorldEntry.second;
 		int NumInGenerator = World->GetGeneratorQueueLength();
 		int NumInSaveQueue = static_cast<int>(World->GetStorageSaveQueueLength());
 		int NumInLoadQueue = static_cast<int>(World->GetStorageLoadQueueLength());
diff --git a/src/World.cpp b/src/World.cpp
index 242345f02..a9d6b2b13 100644
--- a/src/World.cpp
+++ b/src/World.cpp
@@ -253,6 +253,10 @@ cWorld::~cWorld()
 	Serializer.Save();
 
 	m_MapManager.SaveMapData();
+
+	// Explicitly destroy the chunkmap, so that it's guaranteed to be destroyed before the other internals
+	// This fixes crashes on stopping the server, because chunk destructor deletes entities and those access the world.
+	m_ChunkMap.reset();
 }