<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/120150/">https://git.reviewboard.kde.org/r/120150/</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 12th, 2014, 12:59 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;">No really, this just shows that something else is wrong here. Maybe fixed the bug in Qt then. But doing this kind of hit-and-run patching, blindly trying to workaround a bug is really bad as it greatly increases the maintenance cost.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could you please ensure that no threads are involved? Add stuff like Q_ASSERT(QThread::currentThread() == thread()) to the dtor of the class.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
If that does not help, then certainly there is a bug elsewhere, maybe inside Qt, and should be fixed there then.</p></pre>
</blockquote>
<p>On September 12th, 2014, 1:27 a.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 take it you're talking about the descructor for the classes I use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater</code> on in this 2nd patch?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is there a Qt call to check if an object has pending events or (better yet?) flush events for an objects fromt the queue?</p></pre>
</blockquote>
<p>On September 12th, 2014, 2:04 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;">Yes, and also in the first patch which should be reverted, I guess.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And Qt removes pending events from the event queue when an object is deleted. <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Only</em> when threads are involved there might be race conditions. But here, there should not be any threads.</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;">We keep circling around this issue about pending events. Frankly, I shouldn't have asked about detecting or removing them, because my whole previous experience suggests that this is just not doable on OS X. That, and/or it's impossible to avoid getting what I'd recursive events, meaning you get in an event handler when in any number of previous/higher stack frames you're also in an event handler for that same object. Those are pending events too, after all - they haven't been handled and so theoretically at least they're still on the queue. And of course you cannot pop events that are already being handled.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So what would you do? Check after every function call that might have resulted in event processing if your object is still valid, while there is no feature in C/C++ to check if a pointer points to valid memory? Also, remember how I said ObjC doesn't delete objects immediately when they're deleted, but instead leaves them in the NSAutoReleasePool <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">that goes with the current event loop</em>? The crash I've been seeing occurs in a function <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">that gets called from <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">unavoidable</em> ObjC code</em>. And it's quite possible btw that the code that ought to do this "is this pointer still alive" check is part of that ObjC code.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I really think we shouldn't be second-guessing the Qt authors and their knowledge of their own product. If they put a warning about preferring <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater</code> when there are or might be pending events, it is because it's somehow impossible (reliably) to remove pending events for that object from the queue while deleting it. I'm tempted to add that <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater</code> basically schedules the delete for after the exit of the current event loop - and that loop probably isn't so different from that other "loop", the one concerning threads, that is not managed by Qt ... </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I will do the thread checking, and I indeed planned to revert my earlier patch for that. We'll see what that teaches me. That aside, I suppose I'll wrap my modifications in Q_OS_MAC tags . I frankly don't see the problem doing a delete this way or that way, and I guess the Qt authors also would have preferred to use a direct rather than a deferred delete. But sometimes you just have to go with what you have, whether or not that leaves you wondering about whether you're dealing with design bugs or features ...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">BTW, I've asked about this on stackoverflow, no answer. And the Qt forum tells me it's in maintenance as soon as I log in, so that place is apparently off limits to me.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On September 12th, 2014, 12:22 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. 12, 2014, 12:22 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;">In https://reviewboard.kde.org/r/120081/ I proposed an (accepted) approach to prevent kdevelop from crashing after closing the patch review ("git/show differences") toolview. I had another of those crashes after heavier-than-usual perusal of the toolview, despite the previous patch. The attached patch replaces all <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">delete</code>s of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QObject</code> derived class instances with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater()</code>.</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;">kdevplatform git/kde4-legacy on KDE/MacPorts OS X 10.6.8 . </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">(18b63db)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120150/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>