MantisBT - VCMI
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0002048VCMIGUI - PreGamepublic2015-01-19 23:392016-11-11 08:22
ReporterFay 
Assigned ToFay 
PrioritynormalSeveritycrashReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS VersionWin7x64
Product Version0.97b 
Target VersionFixed in Version0.97c 
Summary0002048: reloading game causes crash if there was a path visible
DescriptionReproducibility is actually "almost always", as sometimes it works; most probably caused by memory corruption.
Steps To Reproduce* Load any map,
* Click somewhere to define a short (1-3 tiles) path with your hero; (it seems that longer path rarely crash the game),
* Press menu -> load -> confirm -> choose another map,
* Game should crash during loading.

Reloading the same map generally doesn't crash the game -- possibly some pointers still reference "old" path from previous map?
Additional InformationStack:
0 CTerrainRect::showPath CAdvmapInterface.cpp 180 0x4544a1
1 CTerrainRect::show CAdvmapInterface.cpp 278 0x4547c8
2 CAdvMapInt::show CAdvmapInterface.cpp 751 0x45a116
3 CAdvMapInt::showAll CAdvmapInterface.cpp 673 0x459f28
4 CGuiHandler::totalRedraw CGuiHandler.cpp 157 0x42ab43
5 CGuiHandler::pushInt CGuiHandler.cpp 122 0x42cb82
6 CPlayerInterface::playerStartsTurn CPlayerInterface.cpp 2556 0x4d3f95
7 YourTurn::applyCl NetPacksClient.cpp 794 0x5113ea
8 CApplyOnCL<YourTurn>::applyOnClAfter Client.cpp 78 0x5b6363
9 CClient::handlePack Client.cpp 647 0x4b33cc
10 CClient::run Client.cpp 172 0x4b374b
11 boost::(anonymous namespace)::thread_start_function(void*)@4 0x5368d2
12 boost::(anonymous namespace)::ThreadProxy(void*)@4 0x5352e3
13 KERNEL32!BaseThreadInitThunk C:\Windows\syswow64\kernel32.dll 0x76e9338a
14 ntdll!RtlInitializeExceptionChain C:\Windows\SysWOW64\ntdll.dll 0x77d09f72
15 ntdll!RtlInitializeExceptionChain C:\Windows\SysWOW64\ntdll.dll 0x77d09f45
16 ??

In the code, nextPos variable has broken values and indexes go waaay out of bounds.
TagsNo tags attached.
Relationships
related to 0001954closed SXX vcmiclient crash after same save loaded 2nd time 
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2015-01-19 23:39FayNew Issue
2015-01-19 23:39FayStatusnew => assigned
2015-01-19 23:39FayAssigned To => Tow
2015-02-10 22:43SXXNote Added: 0005445
2015-02-13 10:24FayNote Added: 0005453
2015-02-13 10:24FayAssigned ToTow => Fay
2015-02-13 11:03FayStatusassigned => resolved
2015-02-13 11:03FayResolutionopen => fixed
2015-02-13 11:03FayFixed in Version => 0.97c
2015-02-13 11:05FayNote Added: 0005455
2015-02-13 11:15WarmongerNote Added: 0005456
2015-02-13 11:40FayNote Added: 0005457
2015-02-13 12:32SXXNote Added: 0005458
2015-02-13 12:36SXXNote Added: 0005459
2015-02-13 12:37SXXNote Edited: 0005459bug_revision_view_page.php?bugnote_id=5459#r2817
2015-02-13 13:08FayNote Added: 0005460
2015-02-13 13:10FayNote Added: 0005461
2015-02-13 13:35WarmongerNote Added: 0005462
2015-02-13 14:43SXXNote Added: 0005463
2015-02-13 15:42FayNote Added: 0005465
2015-02-13 16:26AVSNote Added: 0005467
2015-02-13 16:39AVSNote Added: 0005468
2015-11-18 03:15SXXRelationship addedrelated to 0001954
2016-11-11 08:22SXXStatusresolved => closed

Notes
(0005445)
SXX   
2015-02-10 22:43   
This one really annoying and likely have multiple duplicates, e.g: 0001771, 0001672 and likely many more include some my crash report.

I also guess this problem have same nature with other crash inside "CPathfinder::calculatePaths" on loading maps as those two likely use same features.
(0005453)
Fay   
2015-02-13 10:24   
Fix in this PR seems to fix the problem for me. https://github.com/vcmi/vcmi/pull/87 [^]

Haven't checked other reports, but it's likely that these are the same issues.
(0005455)
Fay   
2015-02-13 11:05   
Should I add 'related to' for these other similar issues and/or add notes about possibly being fixed there?
(0005456)
Warmonger   
2015-02-13 11:15   
Maybe, but after merge I can reproduce the issue just the same :/
(0005457)
Fay   
2015-02-13 11:40   
Strange. I was able to reproduce it nearly 100% of time and after that fix I no longer get this crash.

However, I've just found another problem related to reloading while testing now. When I reload between 2 saves (not any saves, just these two), it crashes 100% of time because of broken tile surface ptr. Not related to paths.

Does your crash point to showPath()?
(0005458)
SXX   
2015-02-13 12:32   
I'm sorry, but are you sure that setting nullptr there is actually correct fix for this problem?

Of course it's fix the crash itself, but is that pointer have to be there after loading at all? Isn't this kind a hacky solution?
(0005459)
SXX   
2015-02-13 12:36   
(edited on: 2015-02-13 12:37)
What I mean: there is multiple similar problems where game state incorrect after exiting to menu and back to game, e.g one with "CPathfinder::calculatePaths" where pointer's aren't the same. So I just worry there is some bigger flaw somewhere around and only fixing it's consequences may be not that good idea.

So I wonder may be there have to be feature that restore client state to the point like it's was just after startup to menu. Or something like that.
(0005460)
Fay   
2015-02-13 13:08   
This crash is/was caused by the same problem as 0001953. When you reload the game (load a save after playing a map), player interface reuses CAdvMapInt instance instead of creating new one.

Current path isn't set until it's created by user or deserialized from save state (later I think). That's why after reload, terrain.currentPath points to "path" from previous game.
(0005461)
Fay   
2015-02-13 13:10   
The other solution would be to recreate adv map interface every time you start the game. I don't know if there's a valid reason to keep and reuse an instance (probably high cost of creating new one?).
(0005462)
Warmonger   
2015-02-13 13:35   
It's easier to manage if you create new interface every time. This can't be costly as you do that on every new game anyway.
(0005463)
SXX   
2015-02-13 14:43   
I think it's better solution to create as much as technically possible from scratch to avoid such conflicts. Crashes like this one are way too annoying and sometimes hard to debug so better to avoid them completely.
(0005465)
Fay   
2015-02-13 15:42   
I can try changing it (probably not today though).
But still, I'm not sure if it's safe to recreate the instance; sounds risky. ;)
(0005467)
AVS   
2015-02-13 16:26   
If we decide to continue reusing advMapInt objects, then we should once more review CAdvMapInt::restoreState().
(0005468)
AVS   
2015-02-13 16:39   
And I also don`t like the existence of terrain.currentPath (as variable) at all. This will surely cause more crashes in future.