Merge pull request #3185 from IvanSavenko/memleak_fix

Memory leak fixes

AI/Nullkiller/Pathfinding/AINodeStorage.cpp

@@ -90,7 +90,7 @@ void AINodeStorage::initialize(const PathfinderOptions & options, const CGameSta
 
 	//TODO: fix this code duplication with NodeStorage::initialize, problem is to keep `resetTile` inline
 	const PlayerColor fowPlayer = ai->playerID;
-	const auto fow = static_cast<const CGameInfoCallback *>(gs)->getPlayerTeam(fowPlayer)->fogOfWarMap;
+	const auto & fow = static_cast<const CGameInfoCallback *>(gs)->getPlayerTeam(fowPlayer)->fogOfWarMap;
 	const int3 sizes = gs->getMapSize();
 
 	//Each thread gets different x, but an array of y located next to each other in memory

client/CMT.cpp

@@ -346,7 +346,6 @@ int main(int argc, char * argv[])
 	session["autoSkip"].Bool()  = vm.count("autoSkip");
 	session["oneGoodAI"].Bool() = vm.count("oneGoodAI");
 	session["aiSolo"].Bool() = false;
-	std::shared_ptr<CMainMenu> mmenu;
 	
 	if(vm.count("testmap"))
 	{
@@ -362,7 +361,7 @@ int main(int argc, char * argv[])
 	}
 	else
 	{
-		mmenu = CMainMenu::create();
+		auto mmenu = CMainMenu::create();
 		GH.curInt = mmenu.get();
 	}
 	
@@ -399,7 +398,7 @@ int main(int argc, char * argv[])
 		//start lobby immediately
 		names.push_back(session["username"].String());
 		ESelectionScreen sscreen = session["gamemode"].Integer() == 0 ? ESelectionScreen::newGame : ESelectionScreen::loadGame;
-		mmenu->openLobby(sscreen, session["host"].Bool(), &names, ELoadMode::MULTI);
+		CMM->openLobby(sscreen, session["host"].Bool(), &names, ELoadMode::MULTI);
 	}
 	
 	// Restore remote session - start game immediately
@@ -472,6 +471,12 @@ static void quitApplication()
 			CCS->musich->release();
 			CCS->soundh->release();
 
+			delete CCS->consoleh;
+			delete CCS->curh;
+			delete CCS->videoh;
+			delete CCS->musich;
+			delete CCS->soundh;
+
 			vstd::clear_pointer(CCS);
 		}
 		CMessage::dispose();
@@ -479,6 +484,7 @@ static void quitApplication()
 		vstd::clear_pointer(graphics);
 	}
 
+	vstd::clear_pointer(CSH);
 	vstd::clear_pointer(VLC);
 
 	vstd::clear_pointer(console);// should be removed after everything else since used by logging

client/CMusicHandler.h

@@ -23,8 +23,9 @@ protected:
 	bool initialized;
 	int volume;					// from 0 (mute) to 100
 
-public:
 	CAudioBase(): initialized(false), volume(0) {};
+	~CAudioBase() = default;
+public:
 	virtual void init() = 0;
 	virtual void release() = 0;
 
@@ -32,7 +33,7 @@ public:
 	ui32 getVolume() const { return volume; };
 };
 
-class CSoundHandler: public CAudioBase
+class CSoundHandler final : public CAudioBase
 {
 private:
 	//update volume on configuration change
@@ -124,7 +125,7 @@ public:
 	bool stop(int fade_ms=0);
 };
 
-class CMusicHandler: public CAudioBase
+class CMusicHandler final: public CAudioBase
 {
 private:
 	//update volume on configuration change

client/CServerHandler.cpp

@@ -1021,7 +1021,8 @@ void CServerHandler::threadRunServer()
 void CServerHandler::onServerFinished()
 {
 	threadRunLocalServer.reset();
-	CSH->campaignServerRestartLock.setn(false);
+	if (CSH)
+		CSH->campaignServerRestartLock.setn(false);
 }
 
 void CServerHandler::sendLobbyPack(const CPackForLobby & pack) const

client/CVideoHandler.h

@@ -31,6 +31,7 @@ public:
 class IMainVideoPlayer : public IVideoPlayer
 {
 public:
+	virtual ~IMainVideoPlayer() = default;
 	virtual void update(int x, int y, SDL_Surface *dst, bool forceRedraw, bool update = true, std::function<void()> restart = nullptr){}
 	virtual bool openAndPlayVideo(const VideoPath & name, int x, int y, bool stopOnKey = false, bool scale = false)
 	{
@@ -39,7 +40,7 @@ public:
 	virtual std::pair<std::unique_ptr<ui8 []>, si64> getAudio(const VideoPath & videoToOpen) { return std::make_pair(nullptr, 0); };
 };
 
-class CEmptyVideoPlayer : public IMainVideoPlayer
+class CEmptyVideoPlayer final : public IMainVideoPlayer
 {
 public:
 	int curFrame() const override {return -1;};
@@ -64,7 +65,7 @@ VCMI_LIB_NAMESPACE_BEGIN
 class CInputStream;
 VCMI_LIB_NAMESPACE_END
 
-class CVideoPlayer : public IMainVideoPlayer
+class CVideoPlayer final : public IMainVideoPlayer
 {
 	int stream;					// stream index in video
 	AVFormatContext *format;

client/eventsSDL/InputHandler.cpp

@@ -316,6 +316,8 @@ void InputHandler::handleUserEvent(const SDL_UserEvent & current)
 	auto heapFunctor = static_cast<std::function<void()>*>(current.data1);
 
 	(*heapFunctor)();
+
+	delete heapFunctor;
 }
 
 const Point & InputHandler::getCursorPosition() const

client/mapView/MapRenderer.cpp

@@ -134,8 +134,10 @@ std::shared_ptr<IImage> MapTileStorage::find(size_t fileIndex, size_t rotationIn
 MapRendererTerrain::MapRendererTerrain()
 	: storage(VLC->terrainTypeHandler->objects.size())
 {
+	logGlobal->debug("Loading map terrains");
 	for(const auto & terrain : VLC->terrainTypeHandler->objects)
 		storage.load(terrain->getIndex(), terrain->tilesFilename, EImageBlitMode::OPAQUE);
+	logGlobal->debug("Done loading map terrains");
 }
 
 void MapRendererTerrain::renderTile(IMapRendererContext & context, Canvas & target, const int3 & coordinates)
@@ -173,8 +175,10 @@ uint8_t MapRendererTerrain::checksum(IMapRendererContext & context, const int3 &
 MapRendererRiver::MapRendererRiver()
 	: storage(VLC->riverTypeHandler->objects.size())
 {
+	logGlobal->debug("Loading map rivers");
 	for(const auto & river : VLC->riverTypeHandler->objects)
 		storage.load(river->getIndex(), river->tilesFilename, EImageBlitMode::COLORKEY);
+	logGlobal->debug("Done loading map rivers");
 }
 
 void MapRendererRiver::renderTile(IMapRendererContext & context, Canvas & target, const int3 & coordinates)
@@ -208,8 +212,10 @@ uint8_t MapRendererRiver::checksum(IMapRendererContext & context, const int3 & c
 MapRendererRoad::MapRendererRoad()
 	: storage(VLC->roadTypeHandler->objects.size())
 {
+	logGlobal->debug("Loading map roads");
 	for(const auto & road : VLC->roadTypeHandler->objects)
 		storage.load(road->getIndex(), road->tilesFilename, EImageBlitMode::COLORKEY);
+	logGlobal->debug("Done loading map roads");
 }
 
 void MapRendererRoad::renderTile(IMapRendererContext & context, Canvas & target, const int3 & coordinates)

client/mapView/MapRendererContextState.cpp

@@ -34,8 +34,10 @@ MapRendererContextState::MapRendererContextState()
 
 	objects.resize(boost::extents[mapSize.z][mapSize.x][mapSize.y]);
 
+	logGlobal->debug("Loading map objects");
 	for(const auto & obj : CGI->mh->getMap()->objects)
 		addObject(obj);
+	logGlobal->debug("Done loading map objects");
 }
 
 void MapRendererContextState::addObject(const CGObjectInstance * obj)

lib/CGameInfoCallback.cpp

@@ -726,12 +726,6 @@ CGameInfoCallback::CGameInfoCallback(CGameState * GS):
 {
 }
 
-std::shared_ptr<const boost::multi_array<ui8, 3>> CPlayerSpecificInfoCallback::getVisibilityMap() const
-{
-	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
-	return gs->getPlayerTeam(*getPlayerID())->fogOfWarMap;
-}
-
 int CPlayerSpecificInfoCallback::howManyTowns() const
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);

lib/CGameInfoCallback.h

@@ -247,8 +247,6 @@ public:
 
 	virtual int getResourceAmount(GameResID type) const;
 	virtual TResources getResourceAmount() const;
-	virtual std::shared_ptr<const boost::multi_array<ui8, 3>> getVisibilityMap() const; //returns visibility map
-	//virtual const PlayerSettings * getPlayerSettings(PlayerColor color) const;
 };
 
 VCMI_LIB_NAMESPACE_END

lib/CPlayerState.h

@@ -98,7 +98,7 @@ public:
 	TeamID id; //position in gameState::teams
 	std::set<PlayerColor> players; // members of this team
 	//TODO: boost::array, bool if possible
-	std::shared_ptr<boost::multi_array<ui8, 3>> fogOfWarMap; //[z][x][y] true - visible, false - hidden
+	std::unique_ptr<boost::multi_array<ui8, 3>> fogOfWarMap; //[z][x][y] true - visible, false - hidden
 
 	TeamState();
 	TeamState(TeamState && other) noexcept;

lib/CTownHandler.cpp

@@ -317,7 +317,7 @@ CTownHandler::CTownHandler():
 
 CTownHandler::~CTownHandler()
 {
-	delete randomTown;
+	delete randomFaction; // will also delete randomTown
 }
 
 JsonNode readBuilding(CLegacyConfigParser & parser)

lib/ObstacleHandler.cpp

@@ -106,8 +106,6 @@ ObstacleInfo * ObstacleHandler::loadFromJson(const std::string & scope, const Js
 	info->isAbsoluteObstacle = json["absolute"].Bool();
 	info->isForegroundObstacle = json["foreground"].Bool();
 
-	objects.emplace_back(info);
-
 	return info;
 }
 

lib/VCMI_Lib.cpp

@@ -259,6 +259,11 @@ void LibClasses::clear()
 	delete battlefieldsHandler;
 	delete generaltexth;
 	delete identifiersHandler;
+	delete obstacleHandler;
+	delete terrainTypeHandler;
+	delete riverTypeHandler;
+	delete roadTypeHandler;
+	delete settingsHandler;
 	makeNull();
 }
 
@@ -282,6 +287,11 @@ void LibClasses::makeNull()
 #endif
 	battlefieldsHandler = nullptr;
 	identifiersHandler = nullptr;
+	obstacleHandler = nullptr;
+	terrainTypeHandler = nullptr;
+	riverTypeHandler = nullptr;
+	roadTypeHandler = nullptr;
+	settingsHandler = nullptr;
 }
 
 LibClasses::LibClasses()

lib/gameState/CGameState.cpp

@@ -166,6 +166,8 @@ CGameState::~CGameState()
 	// explicitly delete all ongoing battles first - BattleInfo destructor requires valid CGameState
 	currentBattles.clear();
 	map.dellNull();
+	scenarioOps.dellNull();
+	initialOpts.dellNull();
 }
 
 void CGameState::preInit(Services * services)
@@ -700,7 +702,7 @@ void CGameState::initFogOfWar()
 	int layers = map->levels();
 	for(auto & elem : teams)
 	{
-		auto fow = elem.second.fogOfWarMap;
+		auto & fow = elem.second.fogOfWarMap;
 		fow->resize(boost::extents[layers][map->width][map->height]);
 		std::fill(fow->data(), fow->data() + fow->num_elements(), 0);
 
@@ -1952,7 +1954,7 @@ bool RumorState::update(int id, int extra)
 TeamState::TeamState()
 {
 	setNodeType(TEAM);
-	fogOfWarMap = std::make_shared<boost::multi_array<ui8, 3>>();
+	fogOfWarMap = std::make_unique<boost::multi_array<ui8, 3>>();
 }
 
 TeamState::TeamState(TeamState && other) noexcept:

lib/mapObjects/CGHeroInstance.cpp

@@ -327,13 +327,21 @@ void CGHeroInstance::initHero(CRandomGenerator & rand)
 	{
 		// hero starts with default spellbook presence status
 		if(!getArt(ArtifactPosition::SPELLBOOK) && type->haveSpellBook)
-			putArtifact(ArtifactPosition::SPELLBOOK, ArtifactUtils::createNewArtifactInstance(ArtifactID::SPELLBOOK));
+		{
+			auto artifact = ArtifactUtils::createNewArtifactInstance(ArtifactID::SPELLBOOK);
+			cb->gameState()->map->addNewArtifactInstance(artifact);
+			putArtifact(ArtifactPosition::SPELLBOOK, artifact);
+		}
 	}
 	else
 		spells -= SpellID::SPELLBOOK_PRESET;
 
 	if(!getArt(ArtifactPosition::MACH4))
-		putArtifact(ArtifactPosition::MACH4, ArtifactUtils::createNewArtifactInstance(ArtifactID::CATAPULT)); //everyone has a catapult
+	{
+		auto artifact = ArtifactUtils::createNewArtifactInstance(ArtifactID::CATAPULT);
+		cb->gameState()->map->addNewArtifactInstance(artifact);
+		putArtifact(ArtifactPosition::MACH4, artifact); //everyone has a catapult
+	}
 
 	if(!hasBonus(Selector::sourceType()(BonusSource::HERO_BASE_SKILL)))
 	{
@@ -448,7 +456,11 @@ void CGHeroInstance::initArmy(CRandomGenerator & rand, IArmyDescriptor * dst)
 				ArtifactPosition slot = art->getPossibleSlots().at(ArtBearer::HERO).front();
 
 				if(!getArt(slot))
-					putArtifact(slot, ArtifactUtils::createNewArtifactInstance(aid));
+				{
+					auto artifact = ArtifactUtils::createNewArtifactInstance(aid);
+					cb->gameState()->map->addNewArtifactInstance(artifact);
+					putArtifact(slot, artifact);
+				}
 				else
 					logGlobal->warn("Hero %s already has artifact at %d, omitting giving artifact %d", getNameTranslated(), slot.toEnum(), aid.toEnum());
 			}

lib/mapping/CMap.cpp

@@ -181,6 +181,9 @@ CMap::~CMap()
 	for(auto quest : quests)
 		quest.dellNull();
 
+	for(auto artInstance : artInstances)
+		artInstance.dellNull();
+
 	resetStaticData();
 }
 

lib/networkPacks/NetPacksBase.h

@@ -20,7 +20,9 @@ class ICPackVisitor;
 
 struct DLL_LINKAGE CPack
 {
-	std::shared_ptr<CConnection> c; // Pointer to connection that pack received from
+	/// Pointer to connection that pack received from
+	/// Only set & used on server
+	std::shared_ptr<CConnection> c;
 
 	CPack() = default;
 	virtual ~CPack() = default;

lib/networkPacks/NetPacksLib.cpp

@@ -932,7 +932,7 @@ void SetMovePoints::applyGs(CGameState * gs) const
 void FoWChange::applyGs(CGameState *gs)
 {
 	TeamState * team = gs->getPlayerTeam(player);
-	auto fogOfWarMap = team->fogOfWarMap;
+	auto & fogOfWarMap = team->fogOfWarMap;
 	for(const int3 & t : tiles)
 		(*fogOfWarMap)[t.z][t.x][t.y] = mode != ETileVisibility::HIDDEN;
 
@@ -1327,7 +1327,7 @@ void TryMoveHero::applyGs(CGameState *gs)
 		gs->map->addBlockVisTiles(h);
 	}
 
-	auto fogOfWarMap = gs->getPlayerTeam(h->getOwner())->fogOfWarMap;
+	auto & fogOfWarMap = gs->getPlayerTeam(h->getOwner())->fogOfWarMap;
 	for(const int3 & t : fowRevealed)
 		(*fogOfWarMap)[t.z][t.x][t.y] = 1;
 }

lib/pathfinder/NodeStorage.cpp

@@ -27,7 +27,7 @@ void NodeStorage::initialize(const PathfinderOptions & options, const CGameState
 	int3 pos;
 	const PlayerColor player = out.hero->tempOwner;
 	const int3 sizes = gs->getMapSize();
-	const auto fow = static_cast<const CGameInfoCallback *>(gs)->getPlayerTeam(player)->fogOfWarMap;
+	const auto & fow = static_cast<const CGameInfoCallback *>(gs)->getPlayerTeam(player)->fogOfWarMap;
 
 	//make 200% sure that these are loop invariants (also a bit shorter code), let compiler do the rest(loop unswitching)
 	const bool useFlying = options.useFlying;

lib/pathfinder/PathfinderUtil.h

@@ -19,11 +19,11 @@ VCMI_LIB_NAMESPACE_BEGIN
 
 namespace PathfinderUtil
 {
-	using FoW = std::shared_ptr<const boost::multi_array<ui8, 3>>;
+	using FoW = std::unique_ptr<boost::multi_array<ui8, 3>>;
 	using ELayer = EPathfindingLayer;
 
 	template<EPathfindingLayer::Type layer>
-	EPathAccessibility evaluateAccessibility(const int3 & pos, const TerrainTile & tinfo, FoW fow, const PlayerColor player, const CGameState * gs)
+	EPathAccessibility evaluateAccessibility(const int3 & pos, const TerrainTile & tinfo, const FoW & fow, const PlayerColor player, const CGameState * gs)
 	{
 		if(!(*fow)[pos.z][pos.x][pos.y])
 			return EPathAccessibility::BLOCKED;

lib/serializer/BinaryDeserializer.h

@@ -140,7 +140,6 @@ public:
 	si32 fileVersion;
 
 	std::map<ui32, void*> loadedPointers;
-	std::map<ui32, const std::type_info*> loadedPointersTypes;
 	std::map<const void*, std::shared_ptr<void>> loadedSharedPointers;
 	bool smartPointerSerialization;
 	bool saving;
@@ -279,7 +278,6 @@ public:
 			{
 				// We already got this pointer
 				// Cast it in case we are loading it to a non-first base pointer
-				assert(loadedPointersTypes.count(pid));
 				data = static_cast<T>(i->second);
 				return;
 			}
@@ -313,10 +311,7 @@ public:
 	void ptrAllocated(const T *ptr, ui32 pid)
 	{
 		if(smartPointerSerialization && pid != 0xffffffff)
-		{
-			loadedPointersTypes[pid] = &typeid(T);
 			loadedPointers[pid] = (void*)ptr; //add loaded pointer to our lookup map; cast is to avoid errors with const T* pt
-		}
 	}
 
 	template<typename Base, typename Derived> void registerType(const Base * b = nullptr, const Derived * d = nullptr)

lib/serializer/Connection.cpp

@@ -269,13 +269,7 @@ CPack * CConnection::retrievePack()
 	iser & pack;
 	logNetwork->trace("Received CPack of type %s", (pack ? typeid(*pack).name() : "nullptr"));
 	if(pack == nullptr)
-	{
 		logNetwork->error("Received a nullptr CPack! You should check whether client and server ABI matches.");
-	}
-	else
-	{
-		pack->c = this->shared_from_this();
-	}
 
 	enableBufferedRead = false;
 

lib/spells/AdventureSpellMechanics.cpp

@@ -600,7 +600,7 @@ ESpellCastResult ViewMechanics::applyAdventureEffects(SpellCastEnvironment * env
 
 	const auto spellLevel = parameters.caster->getSpellSchoolLevel(owner);
 
-	const auto fowMap = env->getCb()->getPlayerTeam(parameters.caster->getCasterOwner())->fogOfWarMap;
+	const auto & fowMap = env->getCb()->getPlayerTeam(parameters.caster->getCasterOwner())->fogOfWarMap;
 
 	for(const CGObjectInstance * obj : env->getMap()->objects)
 	{

server/CGameHandler.cpp

@@ -860,7 +860,7 @@ void CGameHandler::onNewTurn()
 				fw.mode = ETileVisibility::REVEALED;
 				fw.player = player;
 				// find all hidden tiles
-				const auto fow = getPlayerTeam(player)->fogOfWarMap;
+				const auto & fow = getPlayerTeam(player)->fogOfWarMap;
 
 				auto shape = fow->shape();
 				for(size_t z = 0; z < shape[0]; z++)

server/CVCMIServer.cpp

@@ -445,6 +445,7 @@ void CVCMIServer::threadHandleClient(std::shared_ptr<CConnection> c)
 		try
 		{
 			pack = c->retrievePack();
+			pack->c = c;
 		}
 		catch(boost::system::system_error & e)
 		{