<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/4126/">http://svn.reviewboard.kde.org/r/4126/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 3rd, 2010, 10:44 p.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I was initially against this kind of thing (too many problems with "intelligent" destructors), this is why it hasn't been done before.
But after long consideration, I think this one is safe and, as you say, somewhat expected by users of the framework, so I'm ok with it.
I'll commit it to trunk, at least, we'll see if it proves stable enough for 4.5.0.
Thanks a lot for the patch, and sorry for the delay.</pre>
</blockquote>
<p>On November 16th, 2010, 12:25 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Turns out I was right, the "intelligent destructor" creates trouble - bug 246652 :-)
I think I'll make it a bit less intelligent (it should make the factory forget this client, but it should not unplug actions one by one).</pre>
</blockquote>
<p>On November 17th, 2010, 8:16 a.m., <b>Thomas Friedrichsmeier</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hm. I didn't look too closely at what ContainerNode::destruct() does. But on first glance this sounds like there will be a memory leak if a client is deleted without being removed, first, now.
*If* so, that's still better than causing a seemingly unrelated crash a long time later. But perhaps there should be a debug message telling developers to fix their object destruction order, whenever a client still has a factory while being deleted (and for KDE 5, we might consider reducing the smartness even further to simply crashing right there and then).</pre>
</blockquote>
<p>On November 17th, 2010, 11:16 a.m., <b>Matthias Fuchs</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Forgive me my complete ignorance -- as I haven't looked at the code -- but couldn't smart pointers solve some of the problems encountered here?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Matthias: let's actually identify problems before we start talking about solutions :)
Thomas: I just checked with kxmlgui_unittest, and there is no leak (now that I fixed a unittest-specific issue with the parent of the KMenus), once the mainwindow is destroyed.
The ContainerNode themselves are destroyed in ~KXMLGUIFactoryPrivate.
I agree though, that there is a small risk for a leak, in more corner cases, like standalone popupmenus which get no parent widget. And I agree too that the best solution is to just leave that in, but warn when someone deletes a client without removing it from the factory first -- which crashed before, so a warning is already much better :) I'll do that.</pre>
<br />
<p>- David</p>
<br />
<p>On June 1st, 2010, 7:15 a.m., Thomas Friedrichsmeier wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs, Christoph Feck and David Faure.</div>
<div>By Thomas Friedrichsmeier.</div>
<p style="color: grey;"><i>Updated 2010-06-01 07:15:25</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=170806">170806</a>,
<a href="https://bugs.kde.org/show_bug.cgi?id=183338">183338</a>,
<a href="https://bugs.kde.org/show_bug.cgi?id=205625">205625</a>,
<a href="https://bugs.kde.org/show_bug.cgi?id=212397">212397</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/KDE/kdelibs/kdeui/tests/kxmlgui_unittest.h <span style="color: grey">(1129152)</span></li>
<li>trunk/KDE/kdelibs/kdeui/tests/kxmlgui_unittest.cpp <span style="color: grey">(1129152)</span></li>
<li>trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiclient.cpp <span style="color: grey">(1129152)</span></li>
<li>trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguifactory.cpp <span style="color: grey">(1129152)</span></li>
<li>trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiwindow.cpp <span style="color: grey">(1129152)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/4126/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>