0001149: Crash(es) on visiting Shipwreck survivor

(0003212)
Warmonger (administrator)
2012-11-15 18:13
edited on: 2012-11-15 18:15

I recently noticed random crashes related to artifact location, this may be one of them. Possibly there is a bug in loading artifacts from config.

This is certainly not related to 0001051, as the other issue is about Quest serialization.

(0003215)
Warmonger (administrator)
2012-11-15 18:52

It looks like addNewArtifactInstance() (among others) is present in both CMap class and CMapService. Beegee, could you please explain what I look at?

(0003228)
beegee (developer)
2012-12-04 19:20
edited on: 2012-12-04 19:35

@Warmonger:
Oops. Those methods belong to CMap only. I'll fix it with my next commit.

When visiting a shipwreck survivor it comes always to a deadlock situation. No crash occured in server nor client. With rev3007 even everything is fine. So it shouldn't has to do anything with faulty map loading. IMO only rev 3012 can has to do sth. with that bug. This revision fixed 0001148 which involved some changes to the moveHero method of CCGameHandler of vcmiserver.

When I stop execution during the deadlock the thread situation in vcmiclient is as follows: (thread numbers are not thread IDs!)
Thread 1: CMT (listen to "main" events)
Thread 2: Console thread
Thread 3: CClient / waits for result of vcmiserver (this client has started the server)
Thread 4 / 5: SDL stuff
Thread 6: bool CPlayerInterface::moveHero( const CGHeroInstance *h, CGPath path )
These lines:
while(stillMoveHero.data != STOP_MOVE && stillMoveHero.data != CONTINUE_MOVE)
stillMoveHero.cond.wait(un);

Thread 7:
- void CPlayerInterface::heroMoved(const TryMoveHero & details)
-- void CPlayerInterface::waitWhileDialog
These lines:
while(showingDialog->data)
showingDialog->cond.wait(un);

This part of VCMI seems to be really tricky and complicated. Changes on vcmiserver/vcmiclient communication needs sadly much testing. Perhaps some sort of unit testing could help here, but definitely not so easy to implement.

(0003229)
Warmonger (administrator)
2012-12-05 07:00

Maybe these are just two separate issues. I will start another report then.

(0003230)
Ivan (developer)
2012-12-05 10:13
edited on: 2012-12-05 10:24

deadlock in moveHero + showing dialog is quite old and was supposed to be fixed multiple times already - I doubt that it connected with rev 3012.

Moreover - I got crash with similar or identical trace using "all spells
+ spellbook" cheat - clearly unrelated to moveHero on server.

EDIT: I can confirm deadlock but this is definitely a different bug.

(0003231)
Ivan (developer)
2012-12-05 10:52

Ok, deadlock is fixed and so far I can't reproduce original bug.

BTW - first Warmonger's comment regarding "random crashes" was done before my moveHero changes and stacktrace is too specific to artifacts - not sure if original bug is fixed.

(0003232)
beegee (developer)
2012-12-05 17:16

Yeah, deadlock is fixed.

(0003265)
Warmonger (administrator)
2012-12-21 09:33

All the issues seem fixed.

Issue History
Date Modified	Username	Field	Change
2012-11-15 18:11	Ivan	New Issue
2012-11-15 18:12	Ivan	Relationship added	related to 0001051
2012-11-15 18:13	Warmonger	Note Added: 0003212
2012-11-15 18:15	Warmonger	Note Edited: 0003212	View Revisions
2012-11-15 18:32	Warmonger	Assigned To	=> Warmonger
2012-11-15 18:32	Warmonger	Status	new => assigned
2012-11-15 18:52	Warmonger	Note Added: 0003215
2012-12-01 08:39	Warmonger	Relationship added	related to 0001154
2012-12-04 19:20	beegee	Note Added: 0003228
2012-12-04 19:22	beegee	Note Edited: 0003228	View Revisions
2012-12-04 19:26	beegee	Note Edited: 0003228	View Revisions
2012-12-04 19:35	beegee	Note Edited: 0003228	View Revisions
2012-12-05 07:00	Warmonger	Note Added: 0003229
2012-12-05 10:13	Ivan	Note Added: 0003230
2012-12-05 10:24	Ivan	Note Edited: 0003230	View Revisions
2012-12-05 10:52	Ivan	Note Added: 0003231
2012-12-05 17:16	beegee	Note Added: 0003232
2012-12-05 17:50	Warmonger	Relationship added	related to 0001157
2012-12-21 09:32	Warmonger	Relationship added	parent of 0001158
2012-12-21 09:33	Warmonger	Relationship deleted	related to 0001157
2012-12-21 09:33	Warmonger	Relationship added	parent of 0001157
2012-12-21 09:33	Warmonger	Note Added: 0003265
2012-12-21 09:34	Warmonger	Status	assigned => resolved
2012-12-21 09:34	Warmonger	Fixed in Version	=> 0.91
2012-12-21 09:34	Warmonger	Resolution	open => fixed
2012-12-21 09:34	Warmonger	Assigned To	Warmonger => Ivan
2014-05-30 17:41	beegee	Status	resolved => closed

Notes
(0003212) Warmonger (administrator) 2012-11-15 18:13 edited on: 2012-11-15 18:15	I recently noticed random crashes related to artifact location, this may be one of them. Possibly there is a bug in loading artifacts from config. This is certainly not related to 0001051, as the other issue is about Quest serialization.

(0003215) Warmonger (administrator) 2012-11-15 18:52	It looks like addNewArtifactInstance() (among others) is present in both CMap class and CMapService. Beegee, could you please explain what I look at?

(0003228) beegee (developer) 2012-12-04 19:20 edited on: 2012-12-04 19:35	@Warmonger: Oops. Those methods belong to CMap only. I'll fix it with my next commit. When visiting a shipwreck survivor it comes always to a deadlock situation. No crash occured in server nor client. With rev3007 even everything is fine. So it shouldn't has to do anything with faulty map loading. IMO only rev 3012 can has to do sth. with that bug. This revision fixed 0001148 which involved some changes to the moveHero method of CCGameHandler of vcmiserver. When I stop execution during the deadlock the thread situation in vcmiclient is as follows: (thread numbers are not thread IDs!) Thread 1: CMT (listen to "main" events) Thread 2: Console thread Thread 3: CClient / waits for result of vcmiserver (this client has started the server) Thread 4 / 5: SDL stuff Thread 6: bool CPlayerInterface::moveHero( const CGHeroInstance *h, CGPath path ) These lines: while(stillMoveHero.data != STOP_MOVE && stillMoveHero.data != CONTINUE_MOVE) stillMoveHero.cond.wait(un); Thread 7: - void CPlayerInterface::heroMoved(const TryMoveHero & details) -- void CPlayerInterface::waitWhileDialog These lines: while(showingDialog->data) showingDialog->cond.wait(un); This part of VCMI seems to be really tricky and complicated. Changes on vcmiserver/vcmiclient communication needs sadly much testing. Perhaps some sort of unit testing could help here, but definitely not so easy to implement.

(0003229) Warmonger (administrator) 2012-12-05 07:00	Maybe these are just two separate issues. I will start another report then.

(0003230) Ivan (developer) 2012-12-05 10:13 edited on: 2012-12-05 10:24	deadlock in moveHero + showing dialog is quite old and was supposed to be fixed multiple times already - I doubt that it connected with rev 3012. Moreover - I got crash with similar or identical trace using "all spells + spellbook" cheat - clearly unrelated to moveHero on server. EDIT: I can confirm deadlock but this is definitely a different bug.

(0003231) Ivan (developer) 2012-12-05 10:52	Ok, deadlock is fixed and so far I can't reproduce original bug. BTW - first Warmonger's comment regarding "random crashes" was done before my moveHero changes and stacktrace is too specific to artifacts - not sure if original bug is fixed.

(0003232) beegee (developer) 2012-12-05 17:16	Yeah, deadlock is fixed.

(0003265) Warmonger (administrator) 2012-12-21 09:33	All the issues seem fixed.