From ce580f0de0b7174484582cea5471e8b333eb0c74 Mon Sep 17 00:00:00 2001 From: leper Date: Fri, 25 Aug 2017 00:37:48 +0000 Subject: [PATCH] Always delete CMapReader. Patch by Sandarac. Fixes #4154. This fixes an assertion failure in ScriptEngine that can occur when closing the game while in the loading screen. Reviewed By: vladislavbelov, leper Differential Revision: https://code.wildfiregames.com/D684 This was SVN commit r20035. --- source/graphics/MapReader.cpp | 14 ------------ source/graphics/MapReader.h | 4 ---- source/ps/World.cpp | 22 +++++++++++-------- source/ps/World.h | 8 +++++++ source/simulation2/Simulation2.cpp | 2 +- .../components/tests/test_Pathfinder.h | 8 +++---- source/simulation2/tests/test_Serializer.h | 4 ++-- 7 files changed, 28 insertions(+), 34 deletions(-) diff --git a/source/graphics/MapReader.cpp b/source/graphics/MapReader.cpp index 91b9024e05..701e378650 100644 --- a/source/graphics/MapReader.cpp +++ b/source/graphics/MapReader.cpp @@ -65,7 +65,6 @@ void CMapReader::LoadMap(const VfsPath& pathname, JSRuntime* rt, JS::HandleValu CLightEnv *pLightEnv_, CGameView *pGameView_, CCinemaManager* pCinema_, CTriggerManager* pTrigMan_, CPostprocManager* pPostproc_, CSimulation2 *pSimulation2_, const CSimContext* pSimContext_, int playerID_, bool skipEntities) { - // latch parameters (held until DelayedLoadFinished) pTerrain = pTerrain_; pLightEnv = pLightEnv_; pGameView = pGameView_; @@ -140,8 +139,6 @@ void CMapReader::LoadMap(const VfsPath& pathname, JSRuntime* rt, JS::HandleValu // load map settings script (must be done after reading map) RegMemFun(this, &CMapReader::LoadMapSettings, L"CMapReader::LoadMapSettings", 5); - - RegMemFun(this, &CMapReader::DelayLoadFinished, L"CMapReader::DelayLoadFinished", 5); } // LoadRandomMap: try to load the map data; reinitialise the scene to new data if successful @@ -150,7 +147,6 @@ void CMapReader::LoadRandomMap(const CStrW& scriptFile, JSRuntime* rt, JS::Handl CLightEnv *pLightEnv_, CGameView *pGameView_, CCinemaManager* pCinema_, CTriggerManager* pTrigMan_, CPostprocManager* pPostproc_, CSimulation2 *pSimulation2_, int playerID_) { - // latch parameters (held until DelayedLoadFinished) m_ScriptFile = scriptFile; pSimulation2 = pSimulation2_; pSimContext = pSimulation2 ? &pSimulation2->GetSimContext() : NULL; @@ -202,8 +198,6 @@ void CMapReader::LoadRandomMap(const CStrW& scriptFile, JSRuntime* rt, JS::Handl // load map settings script (must be done after reading map) RegMemFun(this, &CMapReader::LoadMapSettings, L"CMapReader::LoadMapSettings", 5); - - RegMemFun(this, &CMapReader::DelayLoadFinished, L"CMapReader::DelayLoadFinished", 5); } // UnpackMap: unpack the given data from the raw data stream into local variables @@ -1201,14 +1195,6 @@ int CMapReader::ReadXMLEntities() return ret; } -int CMapReader::DelayLoadFinished() -{ - // we were dynamically allocated by CWorld::Initialize - delete this; - - return 0; -} - //////////////////////////////////////////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/source/graphics/MapReader.h b/source/graphics/MapReader.h index 43e5041ed8..8c9f824b53 100644 --- a/source/graphics/MapReader.h +++ b/source/graphics/MapReader.h @@ -86,9 +86,6 @@ private: // read entity data from the XML file int ReadXMLEntities(); - // clean up everything used during delayed load - int DelayLoadFinished(); - // Copy random map settings over to sim int LoadRMSettings(); @@ -128,7 +125,6 @@ private: CMapGenerator* m_MapGen; - // state latched by LoadMap and held until DelayedLoadFinished CFileUnpacker unpacker; CTerrain* pTerrain; WaterManager* pWaterMan; diff --git a/source/ps/World.cpp b/source/ps/World.cpp index d5d9e475b3..8c87b4e508 100644 --- a/source/ps/World.cpp +++ b/source/ps/World.cpp @@ -57,7 +57,8 @@ CLightEnv g_LightEnv; CWorld::CWorld(CGame *pGame): m_pGame(pGame), m_Terrain(new CTerrain()), - m_UnitManager(new CUnitManager()) + m_UnitManager(new CUnitManager()), + m_MapReader(new CMapReader) { } @@ -70,13 +71,11 @@ void CWorld::RegisterInit(const CStrW& mapFile, JSRuntime* rt, JS::HandleValue s if (mapFile.length()) { VfsPath mapfilename = VfsPath(mapFile).ChangeExtension(L".pmp"); - CMapReader* reader = 0; try { - reader = new CMapReader; CTriggerManager* pTriggerManager = NULL; - reader->LoadMap(mapfilename, rt, settings, m_Terrain, + m_MapReader->LoadMap(mapfilename, rt, settings, m_Terrain, CRenderer::IsInitialised() ? g_Renderer.GetWaterManager() : NULL, CRenderer::IsInitialised() ? g_Renderer.GetSkyManager() : NULL, &g_LightEnv, m_pGame->GetView(), @@ -84,10 +83,11 @@ void CWorld::RegisterInit(const CStrW& mapFile, JSRuntime* rt, JS::HandleValue s pTriggerManager, CRenderer::IsInitialised() ? &g_Renderer.GetPostprocManager() : NULL, m_pGame->GetSimulation2(), &m_pGame->GetSimulation2()->GetSimContext(), playerID, false); // fails immediately, or registers for delay loading + RegMemFun(this, &CWorld::DeleteMapReader, L"CWorld::DeleteMapReader", 5); } catch (PSERROR_File& err) { - delete reader; + SAFE_DELETE(m_MapReader); LOGERROR("Failed to load map %s: %s", mapfilename.string8(), err.what()); throw PSERROR_Game_World_MapLoadFailed("Failed to load map.\nCheck application log for details."); } @@ -97,11 +97,8 @@ void CWorld::RegisterInit(const CStrW& mapFile, JSRuntime* rt, JS::HandleValue s void CWorld::RegisterInitRMS(const CStrW& scriptFile, JSRuntime* rt, JS::HandleValue settings, int playerID) { // If scriptFile is empty, a blank map will be generated using settings (no RMS run) - CMapReader* reader = 0; - - reader = new CMapReader; CTriggerManager* pTriggerManager = NULL; - reader->LoadRandomMap(scriptFile, rt, settings, m_Terrain, + m_MapReader->LoadRandomMap(scriptFile, rt, settings, m_Terrain, CRenderer::IsInitialised() ? g_Renderer.GetWaterManager() : NULL, CRenderer::IsInitialised() ? g_Renderer.GetSkyManager() : NULL, &g_LightEnv, m_pGame->GetView(), @@ -109,8 +106,14 @@ void CWorld::RegisterInitRMS(const CStrW& scriptFile, JSRuntime* rt, JS::HandleV pTriggerManager, CRenderer::IsInitialised() ? &g_Renderer.GetPostprocManager() : NULL, m_pGame->GetSimulation2(), playerID); // registers for delay loading + RegMemFun(this, &CWorld::DeleteMapReader, L"CWorld::DeleteMapReader", 5); } +int CWorld::DeleteMapReader() +{ + SAFE_DELETE(m_MapReader); + return 0; +} /** * Destructor. @@ -120,4 +123,5 @@ CWorld::~CWorld() { delete m_Terrain; delete m_UnitManager; + delete m_MapReader; } diff --git a/source/ps/World.h b/source/ps/World.h index 7e0a2587e1..d563c809c3 100644 --- a/source/ps/World.h +++ b/source/ps/World.h @@ -38,6 +38,7 @@ class CGame; class CUnitManager; class CTerrain; class CStrW; +class CMapReader; /** * CWorld is a general data class containing whatever is needed to accurately represent the world. @@ -61,6 +62,8 @@ class CWorld **/ CUnitManager *m_UnitManager; + CMapReader* m_MapReader; + public: CWorld(CGame *pGame); ~CWorld(); @@ -75,6 +78,11 @@ public: */ void RegisterInitRMS(const CStrW& scriptFile, JSRuntime* rt, JS::HandleValue settings, int playerID); + /** + * Explicitly delete m_MapReader once the map has finished loading. + **/ + int DeleteMapReader(); + /** * Get the pointer to the terrain object. * diff --git a/source/simulation2/Simulation2.cpp b/source/simulation2/Simulation2.cpp index 3a24887427..3a72a72eea 100644 --- a/source/simulation2/Simulation2.cpp +++ b/source/simulation2/Simulation2.cpp @@ -410,7 +410,7 @@ void CSimulation2Impl::Update(int turnLength, const std::vector mapReader(new CMapReader); std::string mapType; scriptInterface.GetProperty(m_InitAttributes, "mapType", mapType); diff --git a/source/simulation2/components/tests/test_Pathfinder.h b/source/simulation2/components/tests/test_Pathfinder.h index 0b505ac439..0a1d98f830 100644 --- a/source/simulation2/components/tests/test_Pathfinder.h +++ b/source/simulation2/components/tests/test_Pathfinder.h @@ -1,4 +1,4 @@ -/* Copyright (C) 2016 Wildfire Games. +/* Copyright (C) 2017 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -72,7 +72,7 @@ public: sim2.LoadDefaultScripts(); sim2.ResetState(); - CMapReader* mapReader = new CMapReader(); // it'll call "delete this" itself + std::unique_ptr mapReader(new CMapReader()); LDR_BeginRegistering(); mapReader->LoadMap(L"maps/skirmishes/Median Oasis (2).pmp", @@ -183,7 +183,7 @@ public: sim2.LoadDefaultScripts(); sim2.ResetState(); - CMapReader* mapReader = new CMapReader(); // it'll call "delete this" itself + std::unique_ptr mapReader(new CMapReader()); LDR_BeginRegistering(); mapReader->LoadMap(L"maps/scenarios/Peloponnese.pmp", @@ -238,7 +238,7 @@ public: sim2.LoadDefaultScripts(); sim2.ResetState(); - CMapReader* mapReader = new CMapReader(); // it'll call "delete this" itself + std::unique_ptr mapReader(new CMapReader()); LDR_BeginRegistering(); mapReader->LoadMap(L"maps/scenarios/Peloponnese.pmp", diff --git a/source/simulation2/tests/test_Serializer.h b/source/simulation2/tests/test_Serializer.h index d9aee50552..3c41517939 100644 --- a/source/simulation2/tests/test_Serializer.h +++ b/source/simulation2/tests/test_Serializer.h @@ -1,4 +1,4 @@ -/* Copyright (C) 2016 Wildfire Games. +/* Copyright (C) 2017 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -839,7 +839,7 @@ public: sim2.LoadDefaultScripts(); sim2.ResetState(); - CMapReader* mapReader = new CMapReader(); // it'll call "delete this" itself + std::unique_ptr mapReader(new CMapReader()); LDR_BeginRegistering(); mapReader->LoadMap(L"maps/skirmishes/Greek Acropolis (2).pmp",