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

[Nemanja Hirsl]

Hi Gaël,

> 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.

No worries. Hope your family and you will be happy(ier) in a new home. I don't 
have any deadline to meet, so it is just important to discuss possible 
solution and get the best out of it.

> 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)" ?

Both calls for the issue I try to solve are in the same thread. 
removeAllPlayers will call ~KPlayer through qDeleteAll. It just calls 
destructor for each list element. If the player is AI and playing, then 
thread.terminate() and thread.wait() are called before destruction of the 
player. 
I must admit I'm not sure if it is possible to reach this part of code from 
multiple threads (destroying KPlayer while someone else is using it, or the 
list), but if it is possible then current code (nor previous also) wouldn't 
help to prevent crash  because these containers are not thread safe and we 
would need to implement some kind of guard for the whole list. In that case, 
as you said, my patch will only reduce the chance for the crash. However, in 
tests I did I couldn't find the case when multiple threads conflicts on KPlayer 
nor the list. 
 
I've created simple example to demonstrate what exactly is happening in the 
code. Take a look at http://pastebin.com/KcyfenQc for details about example. 
Useful link is also http://stackoverflow.com/questions/8657244/does-qts-qlist-
save-iterators-after-containers-modifications

> 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 ?

I agree with you that I've created one extra (unnecessary) test. Motivation 
for this was to make the solution bullet-proof and if in the future someone 
changes KPlayer destructor not to remove it from the list, my code will 
continue to work taking the next element of the list, while code: 
while (playerList()->begin() != playerList()->end()) 
{
    delete (*(playerList()->begin())); 
}
will probably crash on second attempt to delete first element. I can change it 
and just make a comment to describe situation?


I'll prepare the review during the weekend and will show you some debug 
printouts there. See you soon on reviewboard.


Best regards,
Nemanja


On Friday, 21. September 2012. 23.27.39 Kleag wrote:
> 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