<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 13th, 2014, 1:08 a.m. CEST, <b>Kevin Funk</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 just a closer look at the code. It looks indeed unsafe to delete patch source which are <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">no</em> LocalPatchSources, (because others may access dangling pointers then, see 'showVcsDiff' function).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But: Is the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater</code> still necessary now? Is checking against <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">qobject_cast<LocalPatchSource*></code> and then just delete'ing not enough?</p></pre>
</blockquote>
<p>On September 13th, 2014, 1:49 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;">TBH, that's a bit over my head ... what does <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">qobject_cast<LocalPatchSource*></code> do? The code currently checks if it returns true to delete/deleteLater <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">m_patch</code>, isn't that the case when <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">m_patch</code> IS a LocalPatchSource?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, if access through a dangling pointer, e.g. by showVcsDiff, were to explain the crashes I've seen, why would it not crash on Linux too? Unless that's a result of differences in event handling, which kind of takes us back to my own hunch...</p></pre>
</blockquote>
<p>On September 13th, 2014, 6:23 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;">Yes, it could crash on Linux, too. We've seen many reports about crashes in this code base so it could easily be that there are even more issues hidden in it. Generally though, it might be a race or memory corruption which might or might not crash randomly. Generally, we could try to cleanup the codebase and use smartpointers or direct QObject parent/child relationships to prevent such issues.</p></pre>
</blockquote>
<p>On September 24th, 2014, 4:57 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;">Some fresh observations on this issue, in the hope they might give one of you a brilliant idea ;)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I keep getting unpredictable crashes when closing the patchreview toolview through the "Finish Review" button, but mostly (or even only) when having exported the patch (to file or RB).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It appears however that the crash doesn't occur when I first close all the open review windows, and then use the "back" button at the top left of the pinkish main window. That would suggest that whatever happens has to do with those windows : closing them through the window manager is likely to propagate the close event "from the bottom up", with objects being deleted after and because the window has been closed. It also leads me to think that those pending events I mentioned so often above could actually be events that are due to sending the window a close message.</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;">In case anyone is still interested:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a near-identical crash occurs when hitting the "Commit" button in the git->commit... toolview. I evidently didn't try to reproduce that crash (and end up with multiple commits in my working copy), but I did notice that here too it was enough to close all edit windows (Command-Alt-W) before hitting the button.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Which raises the question: is there a way for the toolview plugin to send close messages to the edit windows it opened in the button's callback, potentially wait for those events to have been handled, and then continue with the regular processing? I'm specifically talking about the signal/message they would receive from the window manager, and not closing by deleting the corresponding objects.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On September 12th, 2014, 10:30 p.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, KDevelop and Olivier Goffart.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Sept. 12, 2014, 10:30 p.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>