[Kde-games-devel] Bug 304362 KsirK crashes on "new game" after one is already finished

[Kleag]

Hi Nemanja,

First of all, sorry for the long delay to answer: young children, a new home, 
lot of "real world'" work... I'm away from KsirK since a long time, even if I 
try to follow what is happening on kdegames and to see if there is new bugs I 
could handle easily.

The bug you are correcting here is an old one I was not able to tackle. You're 
right that several open bugs are probably duplicates of this one. I'm happy to 
see you working on it.

I think your analysis is probably right, but I'm not sure about the solution, 
mainly because I don't remember well how it is implemented ! 

Is the KPlayer destructor call and the removeAllPlayers calls in the same 
thread ? If so, then you're solution is right but I don't understand why the 
problem can occur. 

If there is multiple threads, then while your patch will probably reduce the 
crash occurrences, what if the thread destroying the KPlayer  removes it from 
the list between "it = playerList()->begin()" and "delete (*it)" ?

Another point, if the KPlayer destructor removes it from the list, then the 
iterator will always be  invalid and the test "(playerList()->count() != 
count) ? it = playerList()->begin() : it++;" is useless and you could do just 
"it = playerList()->begin()". What do you think ?

Thanks a lot for you interest in kdegames and ksirk in particular. See you 
soon on reviewboard !

Bye,

Gaël
Le dimanche 16 septembre 2012 21:53:41 nemhirsl at gmail.com a écrit :
> Hi Stefan, Gael,
> 
> I'm new to KDE and this is my first attempt to contribute to any project. I
> would really appreciate any feedback from you.
> 
> Recently, I've opened a bug (from the subject) and now, after a couple of
> days of playing with the code and analyzing various situations, I've came
> up with possible solution (attached, see below for explanation).
> 
> First, I would like to list all steps I did before I found possible
> solution: 1. I've pulled the code from git and successfully built it.
> 2. Enabled kdebug from kdebugdialog
> 3. run ksirk in gdb
> 4. Analyzed backtrace shown here:
> https://bugs.kde.org/show_bug.cgi?id=304362 5. Added some new kdebug
> printouts to make sure list iterators are OK: In
> GameAutomaton::removeAllPlayers all list items (KPlayer*) are OK before
> qDeleteAll call. 6. Code analysis:
>  - PlayerList is list of KPlayer*;
>  - Iterators became messed up in qDeleteAll (still crashing);
>  - pulled out deletion from qDeleteAll to my own loop (still crashing);
>  - KPlayer destructor analyzed - When object is deleted, it get pulled out
> from the game (playerList), modifying original playerList. This is the
> problem because iterators to a QList became invalid after any (insertion
> or) removal !
> 
> In KPlayer::~KPlayer() see  game()->playerDeleted(this);
> In  KGame::playerDeleted(KPlayer *player) see
> systemRemovePlayer(player,false); in KGame::systemRemovePlayer(KPlayer*
> player,bool deleteit) see  systemRemove(player,deleteit); Finally result =
> d->mPlayerList.removeAll(p);
> 
> ***
> To prevent this crash qDeleteAll should not be called for lists modified in
> destructors (like in case of Kplayer). The solution I propose is to keep
> all possible cases working. ***
> 
> Change I found in git on 2010-07-03 17:41:42 Refactoring: Simplify
> unnecessarily complicated deletion statements.
> 
> 
> There are also two more bugs (should be duplicates of each other):
> https://bugs.kde.org/show_bug.cgi?id=305000
> andhttps://bugs.kde.org/show_bug.cgi?id=303142 The reason for this crash is
> the same: qDeleteAll is called on Kplayer* list, but this time in
> libkdegames: void KGame::deletePlayers(). There I saw comment Stefan left,
> but maybe the same solution could be applied in this case?
> 
> 
> To sum up my questions:
> Do you agree with proposed solution? If not, what should I change?
> Can this be applied to bug in libkdegames?
> 
> Again, I would really appreciate any feedback.
> Thank you for your time and help.
> 
> Best regards,
> Nemanja