<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/120420/">https://git.reviewboard.kde.org/r/120420/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 21st, 2014, 10:41 p.m. CET, <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120420/diff/4/?file=333844#file333844line323" style="color: black; font-weight: bold; text-decoration: underline;">shell/sessioncontroller.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">323</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="n">currentRecoveryFiles</span><span class="p">.</span><span class="n">contains</span><span class="p">(</span><span class="n">document</span><span class="o">-></span><span class="n">url</span><span class="p">()))</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">323</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="hl"> </span><span class="n"><span class="hl">document</span></span><span class="hl"> </span><span class="o"><span class="hl">&&</span></span><span class="hl"> </span><span class="n">currentRecoveryFiles</span><span class="p">.</span><span class="n">contains</span><span class="p">(</span><span class="n">document</span><span class="o">-></span><span class="n">url</span><span class="p">()))</span></pre></td>
</tr>
</tbody>
</table>
<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;">under what circumstances would you get here without a valid document? since the arg is not a smart pointer, a queued connection would still see a "non-zero" document pointer, even when the document got destroyed. was this what you try to prevent here? won't work though.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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. I've had crashes here, and although I never managed to figure out exactly why, a null pointer dereference seems the "logical" explanation (and the easiest thing to check against).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Shouldn't have found its way into this RR, though.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 21st, 2014, 10:41 p.m. CET, <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120420/diff/4/?file=333845#file333845line206" style="color: black; font-weight: bold; text-decoration: underline;">vcs/vcspluginhelper.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void VcsPluginHelper::disposeEventually(KTextEditor::Document *)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">206</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">context</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<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;">again, can this really happen? when? why now, and not before?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">same thing, though in this case it's almost impossible to say whether the crashes I saw were related or not to the issue at hand.
When and why, I have no idea. As I said, I haven't been able to debug the crashes post-mortem, and it's not an option to run the IDE with full debug and zero optimisation for something that happens only occasionally and without a well-defined trigger.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 21st, 2014, 10:41 p.m. CET, <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120420/diff/4/?file=333845#file333845line214" style="color: black; font-weight: bold; text-decoration: underline;">vcs/vcspluginhelper.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void VcsPluginHelper::disposeEventually(KTextEditor::Document *)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">211</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="o">!</span><span class="n">item</span><span class="o">-></span><span class="n">target</span><span class="p">())</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">214</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="n"><span class="hl">item</span></span><span class="hl"> </span><span class="o"><span class="hl">&&</span></span><span class="hl"> </span><span class="o">!</span><span class="n">item</span><span class="o">-></span><span class="n">target</span><span class="p">())</span></pre></td>
</tr>
</tbody>
</table>
<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;">also looks wrong. the list should never contain zero items. if it does, something else is wrong</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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, I'd hope the null pointer check was omitted with good reason.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This (and the issues above) is one of those cases where I'm banging my head against the impossibility to check a pointer's validity without resorting to expensive things like exceptions or signal trapping. The fact something went wrong somewhere leading to weird arguments elsewhere doesn't mean the code shouldn't try to exit gracefully...</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On December 19th, 2014, 6:52 p.m. CET, 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 Dec. 19, 2014, 6:52 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;">This RR replaces https://git.reviewboard.kde.org/r/120150/, itself a follow-up to RR https://reviewboard.kde.org/r/120081/ . In that RR I proposed an (accepted & submitted ) approach to prevent kdevelop from crashing after closing the patch review ("git/show differences") toolview.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This turned out to be insufficient, the most likely culprit being a nested event loop. In my most recent bug hunting I noticed that <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">closeReview</code> is called through Qt's mouse-eventh handling function when the "Finish Review" button (or "Commit", when doing a git->commit) is pressed. The crash I have been experiencing occurs in that function (or at least not far down the call stack from that function).
I had already noticed that the crash never occurred when I closed all document views first (the patch file and all modified files), e.g. by using the "Close All" menu action.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So:
- <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">closeReview</code> closes all open documents (minus the patchfile) via <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">qDeleteAll(m_highlighters)</code> or by calling <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">m_highlighters.erase(iterator)</code> repeatedly. Documents are thus closed by deleting the "content object" that represents them, letting that fact filter back to the UI
- closing documents via the UI takes the opposite route, and is more in line with a modern GUI framework (at least OS X/Cocoa) where everything happens because of an event in the user interface. You receive an event (message) when the user closes a document, the framework handles most UI-related stuff for you, and if you have to dispose of document-related non-gui data you can do that for example just before the widget actually closes (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">windowWillClose</code>, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">menuWillClose</code>, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">applicationWillClose</code> etc. messages in Cocoa).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This led to the current patch, which cycles through and closes the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Sublime::View</code>s associated with the window area, and then lets Qt send and process all events that are pending and/or result from closing those windows. Only then does it proceed the regular course of action (calling <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">removeHighLighting()</code> which ought to be no longer necessary).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The only change for the user is that the pinkish patch review plugin background (with the "back to code" button) is visible briefly before the toolview is closed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I understand that this crash (or a closely related one) does not only occur on OS X but also on Linux, so I did not put the new code in a conditional block.</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 with kdelibs 4.14.1</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/grepview/grepviewplugin.cpp <span style="color: grey">(7e3d933)</span></li>
<li>plugins/patchreview/patchreview.cpp <span style="color: grey">(18b63db)</span></li>
<li>shell/sessioncontroller.cpp <span style="color: grey">(ae4e355)</span></li>
<li>vcs/vcspluginhelper.cpp <span style="color: grey">(26ab57c)</span></li>
<li>vcs/widgets/vcsdiffpatchsources.cpp <span style="color: grey">(c3e2dae)</span></li>
<li>vcs/widgets/vcsdiffwidget.cpp <span style="color: grey">(54787b9)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120420/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>