<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, 12:26 p.m. UTC, <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, 12:34 p.m. UTC, <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, 1:51 p.m. UTC, <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, 2:03 p.m. UTC, <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, 2:29 p.m. UTC, <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, 3:50 p.m. UTC, <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, 3:56 p.m. UTC, <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>
</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;">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>
<br />
<p>- Milian</p>
<br />
<p>On September 7th, 2014, 8:43 a.m. UTC, 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, 8: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>