<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120081/">https://git.reviewboard.kde.org/r/120081/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 6th, 2014, 2:26 p.m. CEST, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Technically, it should not be a requirement. to delete every qobject via deletelater (and note that we do not do this acutally, most of the time).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please rephrase the comment to something like:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">// HACK: workaround a crash on OS X, see bug: ...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Then it's OK with me to push this patch. Also, did you test this bug on frameworks/qt5? Is it also occurring there or is the hack not required there?</p></pre>
</blockquote>
<p>On September 6th, 2014, 2:34 p.m. CEST, <b>Marko Käning</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Sorry, Milian, I haven't KF5-kdevelop avtually RUNNING on the OSX/CI system yet (due to the QStandardsPath issue). The CI system currently only checks whether projects build successfully. So, I can't say whether this is still an issue for Qt5. Perhaps we shouldn't patch KF5's kdevelop for now.</p></pre>
</blockquote>
<p>On September 6th, 2014, 3:51 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've preferred calling it a tweak rather than a hack as the patch <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">does</em> follow (official?) guidelines, even if they're apparently no technical requirement.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As Marko indicated, it is not currently feasible to test this on KF/Qt5. I planned to ask for more details on the Qt forums, but they're down (once again...)</p></pre>
</blockquote>
<p>On September 6th, 2014, 4:03 p.m. CEST, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No Rene, the comment on the Qt forum is utterly misleading. Always calling deleteLater is <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</strong> an official guideline. It is a <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">hack</em>. Please don't take everything you read on the Qt forum at face value, but with a grain of salt. If in doubt, ask on the Qt development mailing list.</p></pre>
</blockquote>
<p>On September 6th, 2014, 4:29 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't want to start an argument with someone who cannot but know Qt much better than I do, but the Qt forum is not the only source where I found the ... suggestion to use deleteLater. In fact, browsing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">qtbase/src/corelib/kernel/qobject.cpp</code> <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">from Qt5</em>, one can read:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">/*!<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Destroys the object, deleting all its child objects.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">All signals to and from the object are automatically disconnected, and
any pending posted events <span style="color: #008000; font-weight: bold">for</span> the object are removed from the event
queue. However, it is often safer to use deleteLater() rather than
deleting a QObject subclass directly.
<span style="border: 1px solid #FF0000">\</span>warning All child objects are deleted. If any of these objects
are on the stack or global, sooner or later your program will
crash. We <span style="color: #008000; font-weight: bold">do</span> not recommend holding pointers to child objects from
outside the parent. If you still <span style="color: #008000; font-weight: bold">do</span>, the destroyed() signal gives
you an opportunity to detect when an object is destroyed.
<span style="border: 1px solid #FF0000">\</span>warning Deleting a QObject <span style="color: #008000; font-weight: bold">while</span> pending events are waiting to
be delivered can cause a crash. You must not delete the QObject
directly <span style="color: #008000; font-weight: bold">if</span> it exists in a different <span style="color: #008000; font-weight: bold">thread</span> than the one currently
executing. Use deleteLater() instead, which will cause the event
loop to delete the object after all pending events have been
delivered to it.
<span style="border: 1px solid #FF0000">\</span>sa deleteLater()
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">*/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QObject::~QObject()<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
{/**/}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">/*!<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Schedules this object for deletion.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">The object will be deleted when control returns to the event
loop. If the event loop is not running when <span style="color: #008000; font-weight: bold">this</span> <span style="color: #008000; font-weight: bold">function</span> is
called (e.g. deleteLater() is called on an object before
QCoreApplication<span style="color: #666666">::</span>exec()), the object will be deleted once the
event loop is started. If deleteLater() is called after the main event loop
has stopped, the object will not be deleted.
Since Qt <span style="color: #666666">4.8</span>, <span style="color: #008000; font-weight: bold">if</span> deleteLater() is called on an object that lives <span style="color: #008000; font-weight: bold">in</span> a
thread <span style="color: #008000; font-weight: bold">with</span> no running event loop, the object will be destroyed when the
thread finishes.
Note that entering and leaving a <span style="color: #008000; font-weight: bold">new</span> event loop (e.g., by opening a modal
dialog) will <span style="border: 1px solid #FF0000">\</span>e not perform the deferred deletion; <span style="color: #008000; font-weight: bold">for</span> the object to be
deleted, the control must <span style="color: #008000; font-weight: bold">return</span> to the event loop from which
deleteLater() was called.
<span style="border: 1px solid #FF0000">\</span>b{Note<span style="color: #666666">:</span>} It is safe to call <span style="color: #008000; font-weight: bold">this</span> <span style="color: #008000; font-weight: bold">function</span> more than once; when the
first deferred deletion event is delivered, any pending events <span style="color: #008000; font-weight: bold">for</span> the
object are removed from the event queue.
<span style="border: 1px solid #FF0000">\</span>sa destroyed(), QPointer
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">*/<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
void QObject::deleteLater()<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
{<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
QCoreApplication::postEvent(this, new QDeferredDeleteEvent());<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So while it seems you're right it's not an obligation to discard QObject instances through deleteLater, it apparently is a very good idea to do so if there is a chance that "pending events are waiting to be delivered [because direct deletion] can cause a crash."<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
I really think therefore that my modification is <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> a hack; it is actualle more a precaution even than a tweak (and a necessary one at that on OS X)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">BTW, I can corroborate this with experience from Cocoa programming (and with pure X11, from longer ago), where it is indeed quite possible to end up in an object's event handler <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">after</em> having deleted ("released") the object, and thus cause a crash. Rather than checking everywhere if an object's resources are still available, one uses deferred release where necessary (and it's great Qt provides its own mechanism; Cocoa doesn't to my knowledge).</p></pre>
</blockquote>
<p>On September 6th, 2014, 5:50 p.m. CEST, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Interesting, I learned something new about QObject documentation. Anyhow, just don't write it should always be done and commit this patch :) I gave you the ship it already for that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding the above discussion, I really want to ask about it on the Qt development list. I'm personally quite confused by this, consider e.g. a QObject parent-child relationshipp such as:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">parent
<span style="color: #666666">|-</span> child1
<span style="color: #666666">|-</span> child2
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you delete parent, either direct or indirect via <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater()</code>, the children will always be deleted directly via the QObject destructor (or am I out of the loop there as well and they are also only deleted via <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater</code>?.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyhow, unrelated to this patch, really. Please push this in - or don't you have commit rights?</p></pre>
</blockquote>
<p>On September 6th, 2014, 5:56 p.m. CEST, <b>Marko Käning</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">He doesn't have the rights, Milian. Can you do it, or shall I try to do so myself?</p></pre>
</blockquote>
<p>On September 7th, 2014, 10:48 a.m. CEST, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I talked to David Faure about this yesterday btw. My initial assumption was correct - <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater()</code> is <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> what you should use by default for deleting <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QObject</code> instances. The documentation is a bit confusing there, but actually correct, it says: "Deleting a QObject while pending events are waiting to<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
be delivered can cause a crash. You must not delete the QObject directly <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">if it exists in a different thread</strong> than the one currently executing."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Here, no threads should be involved. If there are - then that is the big issue, I doubt the classes themselves are threadsafe.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is David an expert on Qt's internal functioning on OS X, including older versions like 10.6 (asking, he indeed might be)?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you google the function where the crash occurs, you'll get hits from projects other than kdevelop with near identical backtraces (i.e. on OS X), and in this particular case I went from near 100% crashes when doing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">delete m_plugin</code> to 0% doing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">m_plugin->deleteLater()</code>. That's sufficient real-world evidence ...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think the case that applies here is "while pending events are waiting to be delivered". If Qt on Linux/X11 uses something like the X11 context (XContext, XSaveContext, XFindContext, ...), it's more than possible that an object's destructor can ensure that the object being deleted receives no more events through Qt's event handler, and take care of that as the first step.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
To my knowledge, Cocoa doesn't have such a mechanism, and as I said earlier, it's very possible to find an object receiving events after you released it (to make things worse, ObjC's release mechanism isn't immediate like C++'s delete). In short, if you have an object that corresponds to a GUI widget, you shouldn't delete it as soon as the user clicks on the widget's close button (esp. if that's not a standard button provided by the OS). The whole idea behind the GUI API is that the user only has to worry about releasing his/her own resources, for which there are "messages" like windowWillClose and windowHasClosed.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This situation can be intricate enough when you're not really used to Cocoa/ObjC programming, it goes to a new level when you're writing a Cocoa backend to a cross-platform GUI framework like Qt.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I know all this sounds vague, and Qt's own comments I copied above probably could have included details like "can cause a crash on OS X but not on Linux" (which I think they left out for good reason).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">BTW, I haven't checked, but what's the thread on which the review plugin is deleted here? If it isn't the main thread, there's extra reason on OS X to take precautions...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In short, I think that even if David is correct, it is a <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">good idea</em> to use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater</code> for deleting QObjects that <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">might</em> have pending events and/or exist on a different thread (or that represent a GUI widget and are deleted off the main thread). At least on multi-platform code paths.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
OTOH, what are the drawbacks of using <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater</code>, apart from the obvious fact that resources don't get released immediately?</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On September 7th, 2014, 10:43 a.m. CEST, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Software on Mac OS X and KDevelop.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Sept. 7, 2014, 10:43 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On OS X, kdevelop would crash after clicking on the "Finish Review" button, at least when the path review tool was started through a project's git->differences. This crash occurred with the current stable release (kdevplatform 1.6.0) through the 4.7ß up to and including kdevplatform git/kde4-legacy.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The crash would be deep inside Qt, seemingly mouse action/event related. Googling the actual function in which the crash occurred led to the hypothesis a QObject derived object might be disposed of via <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">delete m_obj</code> instead of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">m_obj->deleteLate()</code> as appears to be the correct way of doing things (http://qt-project.org/forums/viewthread/38406/#162801).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">More details, with referring links and backtraces are here: https://bugs.kde.org/show_bug.cgi?id=338829</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A single line change in the PatchReviewPlugin destructor, following this knowledge, seems indeed to be the required fix to prevent the crash. I have not made this a platform-specific change as the online information I found doesn't say anything about whether or not the requirement is valid for OS X only.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I ignore to what extent Qt5 has relaxed the disposal requirements on QObjects but presume it will continue to be a good idea to use ->deleteAll() . I presume Qt has some kind of garbage collection, if so, one should leave object disposal to the GC and not do it the hard way.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Works (i.e. no more crashing) under OS X 10.6.8, with kdelibs git/4.14 and the other dependencies at KDE/MacPorts 4.12.5 . kdevplatform and kdevelop are built using gcc 4.8.3 from MacPorts (due to the C++11 requirement).<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The patch does not interfere with functionality on Linux.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>plugins/patchreview/patchreview.cpp <span style="color: grey">(2977c40)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120081/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>