Page MenuHomeWildfire Games

Fix assertion failure that may happen when closing the game during the loading screen
ClosedPublic

Authored by Sandarac on Jun 27 2017, 6:24 PM.

Details

Reviewers
vladislavbelov
leper
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP20035: Always delete CMapReader. Patch by Sandarac. Fixes #4154.
Trac Tickets
#4154
Summary

There is a chance that the CMapReader object created in either RegisterInit() or RegisterInitRMS() in World.cpp will leak when closing the game during the loading screen.
This ends up causing the assertion failure ScriptEngine.h(43): Assertion failed: "m_Runtimes.size() == 0 && "All runtimes must be destroyed before calling JS_ShutDown!"", because the MapGenerator thread used in CMapReader contains a JS runtime that isn't deallocated if CMapReader leaks.

CMapreader objects delete themselves in DelayLoadFinished() when progressive loading has finished ("self deleting" can be considered bad practice), but this may not happen if the game is closed while loading. Removing this self deleting behavior (i.e. the method DelayLoadFinished()) and holding the CMapReader pointer as a member variable in CWorld allows to better manage its lifetime. This ensures that CMapReader is properly deleted in the destructor of CWorld (or SAFE_DELETE()ed when loading is finished), as the destructor of CWorld is guaranteed to be called when closing the game during loading.

A method could be added to CWorld to explicitly delete (SAFE_DELETE) the mapreader member var once map loading has finished completely (to be called from CGame::ReallyStartGame()). This is not done currently in the diff, but it may be preferable to do this, as the destructor of CWorld is not called until the game is closed/ended, and the map reader will never be used after the start of the game. m_MapReader is now SAFE_DELETE()ed when loading is finished.

The only other use of the CMapReader class is in Simulation2.cpp (for serialization tests and rejoin tests), and in some disabled tests in test_Serializer.h and test_Pathfinder.h, so those have been updated. Using unique_ptrs in the tests (it may be preferable to use one for m_MapReader as well instead of
a raw pointer) allows to mimic the "self deleting" behavior originally intended for CMapReader.

Test Plan

Read through the diff to see that the destructor of m_MapReader (which will in turn delete the CMapGenerator/CMapGeneratorWorker) is guaranteed to be called whenever closing the game during the loading screen. Notice that the map-loading process itself is not changed.

The actual assertion failure itself can be difficult to reproduce by just trying to close the game while loading, but the destructor of the autoFree struct in CMapGeneratorWorker::Run() can be commented-out to prove that the failure to deallocate m_ScriptInterface is what causes the assertion failure (it will happen when exiting the game).

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Sandarac created this revision.Jun 27 2017, 6:24 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jun 27 2017, 6:24 PM
Vulcan added a subscriber: Vulcan.Jun 27 2017, 7:13 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1640/ for more details.

vladislavbelov added inline comments.
source/ps/World.cpp
61 ↗(On Diff #2717)

Why not on the new line like for others?

119 ↗(On Diff #2717)

It's not good, because the map reader is big enough and stores many data.

Sandarac updated this revision to Diff 2742.Jun 28 2017, 11:23 PM
Sandarac edited the summary of this revision. (Show Details)

SAFE_DELETE() m_MapReader once all map-loading tasks have been finished.

It seems the two additional potential memory leaks pointed out by Yves in the trac ticket (in LoaderThunks.h) are somewhat unrelated, as even if those are fixed, there is still the possibility that DelayLoadFinished() will never be called (unless it is decided that the game should hang for some time when being closed during the loading screen in order to call all the remaining functions in LoadRequests).

Sandarac marked 2 inline comments as done.Jun 28 2017, 11:24 PM
vladislavbelov added a comment.EditedJun 28 2017, 11:30 PM

SAFE_DELETE() m_MapReader once all map-loading tasks have been finished.

I suggest to use your way (delete in the world dtor), but make DelayLoadingFinished not a method, but a static function or a method in the world class (which is even better), which takes 1 param (pointer to the map reader) or gets from the current world object, and safe deletes it. So, you don't need to change other logics (RealStartGame, etc) and it works good, I think.

So it'd be like:

// CWorld class
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(),
    m_pGame->GetView() ? m_pGame->GetView()->GetCinema() : NULL,
    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::DelayLoadFinished, L"CWorld::DelayLoadFinished", 5);

Another profit of this way is that you delete an object in a scope/place where it was allocated.

It seems the two additional potential memory leaks pointed out by Yves in the trac ticket (in LoaderThunks.h) are somewhat unrelated, as even if those are fixed, there is still the possibility that DelayLoadFinished() will never be called (unless it is decided that the game should hang for some time when being closed during the loading screen in order to call all the remaining functions in LoadRequests).

I could fix Loader's leaks, if noone already mind this.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1654/ for more details.

Thank you Vladislav for the comments!

SAFE_DELETE() m_MapReader once all map-loading tasks have been finished.

I suggest to use your way (delete in the world dtor), but make DelayLoadingFinished not a method, but a static function or a method in the world class (which is even better), which takes 1 param (pointer to the map reader) or gets from the current world object, and safe deletes it.

One thing to consider is that "Loader code" (i.e. RegMemFun()) is not currently used directly in CWorld; it is only used in GameView.cpp, MapReader.cpp, and Game.cpp. So it perhaps seems out-of-place and awkward to have this isolated call to RegMemFun() in the CWorld class, when every single other call to this function that adds to the loader is done elsewhere (and where it is called, it is called multiple times in batches) (and there would have to be two calls added because of the two different functions for loading random and non-random maps, of course). It may be less invasive to have the one call in CGame::ReallyStartGame that causes the SAFE_DELETE (the class which already solely manages the only CWorld instance m_World), and then keep CWorld free of loader code.

So, you don't need to change other logics (RealStartGame, etc) and it works good, I think.

The only "logics" changed outside of CWorld/CMapReader is the single method call added in CGame::ReallyStartGame().

I could fix Loader's leaks, if noone already mind this.

Yes, if you could fix that, that would be good, but it seems fixing those potential leaks may involve re-writing some or much of the Loader/LoaderThunks code.

vladislavbelov added a comment.EditedJun 29 2017, 6:09 PM

I mean you don't need to modify other code (which isn't good), but just instead of in CGame:

m_World->DeleteMapReader();

add to the CWorld:

RegMemFun(this, &CWorld::DeleteMapReader, L"CWorld::DeleteMapReader", 5);

The other code is the same. I think it's better for logics.

Sandarac updated this revision to Diff 2762.Jun 29 2017, 8:23 PM
Sandarac added a reviewer: vladislavbelov.

Use Vladislav's recommendation and add the function that deletes m_MapReader to the LoadRequests to be called when all map loading tasks are finished, instead of calling it from CGame.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1667/ for more details.

leper added reviewers: Restricted Owners Package, Restricted Owners Package.Jul 10 2017, 2:12 AM
vladislavbelov accepted this revision.Aug 11 2017, 8:49 PM

After the merge conflict fix, it compiles and works on Windows 8.1.

This revision is now accepted and ready to land.Aug 11 2017, 8:49 PM
elexis added a subscriber: elexis.Aug 13 2017, 6:06 AM

Rebased

source/ps/World.cpp
61 ↗(On Diff #2717)

parntheses, can be done in alphabetical order which avoids changing one line incidentally

source/ps/World.h
70 ↗(On Diff #2762)

Guess we don't really need a comment, the other ones don't explain anything relevant either

vladislavbelov added inline comments.Aug 13 2017, 8:55 AM
source/ps/World.cpp
61 ↗(On Diff #2717)

What do you mean? We shouldn't change the order of the initialization list without changing the order of the definition, because we will get warnings.

leper accepted this revision.Aug 25 2017, 2:34 AM
This revision was automatically updated to reflect the committed changes.