Do not call into things we don't own in destructors

- Remove improper accesses in cChunk destructor
* Fixes #4894

src/BlockEntities/JukeboxEntity.cpp

@@ -24,19 +24,6 @@ cJukeboxEntity::cJukeboxEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Ve
 
 
 
-cJukeboxEntity::~cJukeboxEntity()
-{
-	if (m_World && IsPlayingRecord())
-	{
-		// Stop playing music when destroyed by any means
-		m_World->BroadcastSoundParticleEffect(EffectID::SFX_RANDOM_PLAY_MUSIC_DISC, GetPos(), 0);
-	}
-}
-
-
-
-
-
 void cJukeboxEntity::Destroy(void)
 {
 	ASSERT(m_World != nullptr);

src/BlockEntities/JukeboxEntity.h

@@ -22,7 +22,6 @@ public:  // tolua_export
 	BLOCKENTITY_PROTODEF(cJukeboxEntity)
 
 	cJukeboxEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World);
-	virtual ~cJukeboxEntity() override;
 
 	// tolua_begin
 

src/Chunk.cpp

@@ -103,24 +103,9 @@ cChunk::cChunk(
 
 cChunk::~cChunk()
 {
-	cPluginManager::Get()->CallHookChunkUnloaded(*m_World, m_PosX, m_PosZ);
-
 	// LOGINFO("### delete cChunk() (%i, %i) from %p, thread 0x%x ###", m_PosX, m_PosZ, this, GetCurrentThreadId());
 
-	m_BlockEntities.clear();
-
-	// Remove and destroy all entities that are not players:
-	cEntityList Entities;
-	std::swap(Entities, m_Entities);  // Need another list because cEntity destructors check if they've been removed from chunk
-	for (auto & Entity : Entities)
-	{
-		if (!Entity->IsPlayer())
-		{
-			// Scheduling a normal destruction is neither possible (Since this chunk will be gone till the schedule occurs) nor necessary.
-			Entity->DestroyNoScheduling(false);  // No point in broadcasting in an unloading chunk. Chunks unload when no one is nearby.
-		}
-	}
-
+	// Inform our neighbours that we're no longer valid:
 	if (m_NeighborXM != nullptr)
 	{
 		m_NeighborXM->m_NeighborXP = nullptr;
@@ -137,6 +122,7 @@ cChunk::~cChunk()
 	{
 		m_NeighborZP->m_NeighborZM = nullptr;
 	}
+
 	delete m_WaterSimulatorData;
 	m_WaterSimulatorData = nullptr;
 	delete m_LavaSimulatorData;
@@ -221,6 +207,25 @@ bool cChunk::CanUnloadAfterSaving(void) const
 
 
 
+void cChunk::OnUnload()
+{
+	// Note: this is only called during normal operation, not during shutdown
+
+	// Notify all entities of imminent unload:
+	for (auto & Entity : m_Entities)
+	{
+		// Chunks cannot be unloaded when they still contain players:
+		ASSERT(!Entity->IsPlayer());
+
+		// Notify the entity:
+		Entity->OnRemoveFromWorld(*Entity->GetWorld());
+	}
+}
+
+
+
+
+
 void cChunk::MarkSaving(void)
 {
 	m_IsSaving = true;

src/Chunk.h

@@ -90,6 +90,11 @@ public:
 	/** Returns true if the chunk could have been unloaded if it weren't dirty */
 	bool CanUnloadAfterSaving(void) const;
 
+	/** Called when the chunkmap unloads unused chunks.
+	Notifies contained entities that they are being unloaded and should for example, broadcast a destroy packet.
+	Not called during server shutdown; such cleanup during shutdown is unnecessary. */
+	void OnUnload();
+
 	bool IsLightValid(void) const {return m_IsLightValid; }
 
 	/*

src/ChunkMap.cpp

@@ -1717,10 +1717,17 @@ void cChunkMap::UnloadUnusedChunks(void)
 	for (auto itr = m_Chunks.begin(); itr != m_Chunks.end();)
 	{
 		if (
-			(itr->second.CanUnload()) &&  // Can unload
+			itr->second.CanUnload() &&  // Can unload
 			!cPluginManager::Get()->CallHookChunkUnloading(*GetWorld(), itr->first.ChunkX, itr->first.ChunkZ)  // Plugins agree
 		)
 		{
+			// First notify plugins:
+			cPluginManager::Get()->CallHookChunkUnloaded(*m_World, itr->first.ChunkX, itr->first.ChunkZ);
+
+			// Notify entities within the chunk, while everything's still valid:
+			itr->second.OnUnload();
+
+			// Kill the chunk:
 			itr = m_Chunks.erase(itr);
 		}
 		else

src/ClientHandle.cpp

@@ -129,7 +129,6 @@ cClientHandle::~cClientHandle()
 		{
 			RemoveFromAllChunks();
 			m_Player->GetWorld()->RemoveClientFromChunkSender(this);
-			m_Player->DestroyNoScheduling(true);
 		}
 		// Send the Offline PlayerList packet:
 		cRoot::Get()->BroadcastPlayerListsRemovePlayer(*m_Player);

src/Entities/Entity.cpp

@@ -250,21 +250,6 @@ void cEntity::Destroy()
 
 
 
-void cEntity::DestroyNoScheduling(bool a_ShouldBroadcast)
-{
-	SetIsTicking(false);
-	if (a_ShouldBroadcast)
-	{
-		m_World->BroadcastDestroyEntity(*this);
-	}
-
-	Destroyed();
-}
-
-
-
-
-
 void cEntity::TakeDamage(cEntity & a_Attacker)
 {
 	int RawDamage = a_Attacker.GetRawDamageAgainst(*this);

src/Entities/Entity.h

@@ -341,9 +341,6 @@ public:
 	initialize clients with our position. */
 	Vector3d GetLastSentPosition(void) const { return m_LastSentPosition; }
 
-	/** Destroy the entity without scheduling memory freeing. This should only be used by cChunk or cClientHandle for internal memory management. */
-	void DestroyNoScheduling(bool a_ShouldBroadcast);
-
 	/** Makes this entity take damage specified in the a_TDI.
 	The TDI is sent through plugins first, then applied.
 	If it returns false, the entity hasn't receive any damage. */

src/World.cpp

@@ -226,6 +226,7 @@ cWorld::cWorld(
 
 	cFile::CreateFolderRecursive(m_DataPath);
 
+	// TODO: unique ptr unnecessary
 	m_ChunkMap = std::make_unique<cChunkMap>(this);
 	m_ChunkMap->TrackInDeadlockDetect(a_DeadlockDetect, m_WorldName);
 
@@ -964,11 +965,6 @@ void cWorld::Stop(cDeadlockDetect & a_DeadlockDetect)
 
 		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.
-	// TODO: destructors should only be used for releasing resources, not doing extra work
-	m_ChunkMap.reset();
 }