rP15568 introduced a memory leak that (m_ScriptInterface not deleted in case of early return).
This was fixed in rP15569 by adding a struct Autofree that deletes the pointer in its dtor when the early return or function/scope end was reached.
That deleted too soon, before JSAutoRequest was deleted, which was fixed in rP15572.
rP20035 fixed the MapReader not being deleted in case of mapgen abort, which leaked its CMapGeneratorWorker which leaked its m_ScriptInterface, which triggered a debug breakpoint mandating all runtimes to be deleted prior to something else being deleted.
Details
Details
The CMapGeneratorWorker thread either succeeds and reaches the delete point, or it is abandoned and reaches the delete point.
The CMapGeneratorWorker dtor waits for the thread to finish one way or another.
The crash of rP15572 is not triggered because the JSAutoRequest is deleted when exiting the Run function prior to deleting m_ScriptInterface.
rP20035 ensures that the CMapGeneratorWorker dtor is always called.
What is the obvious I missed, or can the struct construction and its warnings be safely removed?
Diff Detail
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
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/95/display/redirect
Comment Actions
All I can think of are exceptions. What if an exception is thrown in CMapGeneratorWorker::Run()?
Comment Actions
If a mapgen script throws an error, or if the user hits alt+f4 (SQL quit event), then LoadGlobalScriptFile (it's JS::Evaluate call`) and subsequently Run will return false.
Inserting a throw new Error("blerp"); to new_rms_test.js for instance:
Generating New RMS Test of size 256 and 4 players. ERROR: JavaScript error: maps/random/new_rms_test.js line 6 Error: blerp @maps/random/new_rms_test.js:6:7 ERROR: CMapGeneratorWorker::Run: Failed to load RMS 'maps/random/new_rms_test.js'
and handing over a sigterm:
Creating sand patches...WARNING: Quit requested! WARNING: JavaScript warning: maps/random/rmgen/Area.js line 11 Script terminated by timeout at: Area@maps/random/rmgen/Area.js:11:15 createArea@maps/random/rmgen/library.js:198:13 createAreas/placeFunc@maps/random/rmgen/library.js:133:10 retryPlacing@maps/random/rmgen/library.js:102:16 createAreas@maps/random/rmgen/library.js:136:9 @maps/random/neareastern_badlands.js:115:1 WARNING: Quit requested! WARNING: JavaScript warning: maps/random/rmgen/library.js line 136 Script terminated by timeout at: createAreas@maps/random/rmgen/library.js:136:9 @maps/random/neareastern_badlands.js:115:1
the latter comes from rP21264.
Comment Actions
WOuldn't it be better to make m_ScriptInterface an std::unique_ptr (and still delete)? That would be exception-safe, right?
Comment Actions
For a C++ exception in Run() the following happens (with the currently committed code without this patch):
terminate called after throwing an instance of 'std::invalid_argument'
what(): blurp
Thread 27 "pyrogenesis" received signal SIGABRT, Aborted.
[Switching to Thread 0x7fffa4bff700 (LWP 2118)]
0x00007ffff6137755 in raise () from /usr/lib/libc.so.6
(gdb) info stack
#0 0x00007ffff6137755 in raise () from /usr/lib/libc.so.6
#1 0x00007ffff6122851 in abort () from /usr/lib/libc.so.6
#2 0x00007ffff64d881f in __gnu_cxx::__verbose_terminate_handler () at /build/gcc/src/gcc/libstdc++-v3/libsupc++/vterminate.cc:95
#3 0x00007ffff64e530a in __cxxabiv1::__terminate (handler=<optimized out>) at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_terminate.cc:47
#4 0x00007ffff64e5367 in std::terminate () at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_terminate.cc:57
#5 0x00007ffff64e55bd in __cxxabiv1::__cxa_throw (obj=obj@entry=0x7fff84ef6b80, tinfo=0x7ffff661d458 <typeinfo for std::invalid_argument>, dest=0x7ffff64fb720 <std::invalid_argument::~invalid_argument()>)
at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_throw.cc:95
#6 0x00005555555ced13 in CMapGeneratorWorker::Run (this=0x7fffdc014750) at ../../../source/graphics/MapGenerator.cpp:175
#7 0x00005555559a4765 in CMapGeneratorWorker::RunThread (data=0x7fffdc014750) at ../../../source/graphics/MapGenerator.cpp:101
#8 0x00007ffff62c957f in start_thread () from /usr/lib/libpthread.so.0
#9 0x00007ffff61f90e3 in clone () from /usr/lib/libc.so.6(09:01:24 PM) historicbruno: elexis, yeah something like CMapGeneratorWorker::LoadMapTerrain, it can throw a couple of exceptions
Equally triggering the throw PSERROR_File_InvalidVersion(); in this function (jebel barkal random map) triggers the same.
Comment Actions
You mean making it a pointer in the tests and passing a pointer? I don't see the need to make it a pointer, unless there are reasons the AutoFree must be there.
As far as I see there should by design not be any exceptions thrown in the MapGenerator thread.
If there is an error in mapgen stage, it is passed as progress = -1 to the MapReader in CMapReader::GenerateMap.
One could do some effort to pass over the exception string.
I've created D2089 to change the two exceptions relevant in this file to JS Report Errors and catch the ones in MapReader unrelated from the code here.
Unless we want to throw exceptions in the MapGenerator class, this should be independent to this diff and the delete call would always be called (or the program terminated if there is a command throwing an uncaught exception).
| source/graphics/MapGenerator.cpp | ||
|---|---|---|
| 108 ↗ | (On Diff #8929) | It's not equal to SAFE_DELETE, you don't do self->m_ScriptInterface = nullptr. Is that expected? |
| source/graphics/MapGenerator.cpp | ||
|---|---|---|
| 108 ↗ | (On Diff #8929) | I chose delete over SAFE_DELETE because it's an error if self->m_ScriptInterface is null but it doesn't complain about it. Implied assert. |
| source/graphics/MapGenerator.cpp | ||
|---|---|---|
| 108 ↗ | (On Diff #8929) | We should check for a nullptr in a nicer way than that (not rely on mysterious crashes). According to current coding conventions: If deleting a pointer, and it's not in a destructor, and it's not being immediately assigned a new value, use "SAFE_DELETE(p)" (which is equivalent to "delete p; p = nullptr;") to avoid dangling pointers to deleted memory. I guess it could be argued whether it's essentially in a destructor or not, but I think an assignment is cheap, safer, and might prevent future errors. |
Comment Actions
Set to NULL pointer after delete.
| source/graphics/MapGenerator.cpp | ||
|---|---|---|
| 108 ↗ | (On Diff #8929) |
Actually it doesn't:
So SAFE_DELETE is superior indeed. |
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/108/display/redirect
| source/graphics/MapGenerator.cpp | ||
|---|---|---|
| 108 ↗ | (On Diff #8929) | Sorry, I meant your point that m_ScriptInterface might be null (fail to create somehow). Then if we access it, there will be a crash. Or did you not mean that? |
| source/graphics/MapGenerator.cpp | ||
|---|---|---|
| 108 ↗ | (On Diff #8929) | I thought delete null; crashes, but it doesn't. |
Comment Actions
You mean those tests: D2085 ?
I don't understand your comment, m_ScriptInterface is already a pointer? If instead the script interface is only needed in Run(), it should be defined there as a non-pointer.
Comment Actions
It's a pointer in MapGenerator but not test_MapGenerator, and the the Run() function can't be changed to consume the scriptInterface as an argument since the script functions will also access m_ScriptInterface, for example LoadScripts, as those will operate on a scriptinterface again. I suppose that could be pulled from the private context instead of the member, but it doesn't look more clear to me if the functions will use a scriptinterface that is different from the member one. (Unless you propose to drop MapGenerator.cpp completely somehow from the testcase)
Comment Actions
Right. As said on D2085 I don't think you need to use the map worker's script interface to unit test map script stuff though.
(Unless you propose to drop MapGenerator.cpp completely somehow from the testcase)
Indeed that is what I do