[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Make sure to remove KXMLGUIClients from factory on
From:       "Thomas Friedrichsmeier" <thomas.friedrichsmeier () ruhr-uni-bochum ! de>
Date:       2010-06-01 7:15:25
Message-ID: 20100601071525.28256.96377 () localhost
[Download RAW message or body]


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4126/
-----------------------------------------------------------

(Updated 2010-06-01 07:15:25.426790)


Review request for kdelibs, Christoph Feck and David Faure.


Changes
-------

If anything is left of bug 205625, it is very likely also due to this cause. According to \
comment #14 on that bug, Christoph and David, you have discussed pointers to dead clients in \
the factory's list of clients, before. Therefore, I'm adding you as reviewers.


Summary
-------

Under some circumstances, dangling pointers to deleted KXMLGUIClients may remain in the list of \
a KXMLGUIFactory's clients. Usually, KXMLGUIClients are also KParts, and managed by a \
KPartManager. If everything goes as expected, the KPartManager detects when the active KPart is \
destroyed, and the KParts::MainWindow takes care of removing it from the factory. When \
XMLGUIClients are nested, things may not always go as smoothly. KXMLGUIFactory::removeClient() \
removes clients recursively, however, if one of the child clients gets destroyed before the \
parent KParts was removed from the factory, it will be taken out of the parent client's list of \
children, but not out of the factory's list of clients. Possibly, there are other subtle cases, \
where dangling pointer remain in KXMLGUIFactory::clients(). Naturally this will lead to \
crashes, when code tries to do something with all clients of the factory. The listed bugs are \
symptoms of this cause (at least potentially; with non-reproducible bugs it's always hard to \
tell, whether there was only a single cause).

Probably this issue is addressable in the respectively parts / applications, by making sure \
that any child clients are not deleted before the toplevel part is removed from the factory. \
However, it is possible, and sensible to add code in the KXMLGUIClient d'tor to make sure that \
the factory will never keep dangling pointers. This patch does so. Some additional code is \
needed to make sure that this does not cause trouble during application exit, by making sure \
that the KXMLGUIFactory is deleted before the KXMLGUIBuilder, and remaining clients no longer \
assume a factory.

Note:
Bug #170806 contains a valgrind log, and reproducible instructions on crashing kontact, which \
can probably be mapped to other affected applications.


This addresses bugs 170806, 183338, 205625, and 212397.
    https://bugs.kde.org/show_bug.cgi?id=170806
    https://bugs.kde.org/show_bug.cgi?id=183338
    https://bugs.kde.org/show_bug.cgi?id=205625
    https://bugs.kde.org/show_bug.cgi?id=212397


Diffs
-----

  trunk/KDE/kdelibs/kdeui/tests/kxmlgui_unittest.h 1129152 
  trunk/KDE/kdelibs/kdeui/tests/kxmlgui_unittest.cpp 1129152 
  trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiclient.cpp 1129152 
  trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguifactory.cpp 1129152 
  trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiwindow.cpp 1129152 

Diff: http://reviewboard.kde.org/r/4126/diff


Testing
-------

Fixes crashes during toolbar / shortcut configuration for RKWard.

Results of existing unit tests are not affected. New unit test added.

Tested on trunk, only. If this should go into 4.4, I'd like someone else to do the commit on \
the branch.


Thanks,

Thomas


[prev in list] [next in list] [prev in thread] [next in thread]