Crash - All runtimes must be destroyed before calling JS_ShutDown

[elexis]

When I start the random map script "Syria" and quit the game while the loading screen is about 20%, chances are high that it runs into a breakpoint:

ScriptEngine.h(43): Assertion failed: "m_Runtimes.size() == 0 && "All runtimes must be destroyed before calling JS_ShutDown!""
Assertion failed: "m_Runtimes.size() == 0 && "All runtimes must be destroyed before calling JS_ShutDown!""

Could it be related to r18584?

[Yves]

r18584 added that assertion and made the problem appear, but I guess it was there before. The proper order is to call JS_Init first, then create one or more runtimes, use them, destroy all of them and then call JS_Shutdown. We have added this assertion to ensure that no script runtimes are remaining when JS_Shutdown gets called. Apparently there still are active script runtimes in this particular case (or our test is broken).

I'll have a look.

[Yves]

I've figured out where the problem most likely is.

In CMapGeneratorWorker::RunThread, we create a ScriptInterface and delete it again when this function has completed. So we must ensure that it completes before we destroy the parent runtime in the main thread. That should be ensured by the destructor of CMapGeneratorWorker because it joins the threads using pthread_join (so the worker thread terminates before the main thread does).

However, this destructor does not get called because there is a memory leak in the case when loading is aborted by closing the window. The lifetime of ~CMapGeneratorWorker is tied to CMapReader (~CMapReader > ~CMapGenerator > CMapGeneratorWorker). And CMapReader leaks because it's deleted at the end of the loading process inside CMapReader::DelayLoadFinished, which never happens in this case.

So we need to properly delete the CMapReader object when loading is cancelled.

EDIT: Actually there are two additional leaks in ps/LoaderThunks.h (the two "new" statements) if the registered functions are never called.

[elexis]

Putting this into the list of tickets few people look at rarely. Doesn't have to stay.

[leper]

In 20035:

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

[leper]

Actually there are two additional leaks in ps/LoaderThunks.h (the two "new" statements) if the registered functions are never called.

I'm not sure if it is worth creating a ticket for those, since those are just leaking if we never do any delay loading (but we still could later, so not a leak), or if we terminate the game (which means the OS takes care of that).