<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/128113/">https://git.reviewboard.kde.org/r/128113/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 16th, 2016, 7:57 a.m. UTC, <b>David Faure</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/128113/diff/4/?file=468220#file468220line173" style="color: black; font-weight: bold; text-decoration: underline;">tests/kjobtrackerstest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<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">173</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QObject</span><span class="o">::</span><span class="n">connect</span><span class="p">(</span><span class="n">testJob</span><span class="p">,</span> <span class="o">&</span><span class="n">KJob</span><span class="o">::</span><span class="n">finished</span><span class="p">,</span> <span class="n">tracker3</span><span class="p">,</span> <span class="o">&</span><span class="n">KUiServerJobTracker</span><span class="o">::</span><span class="n">unregisterJob</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;">Is this change supposed to trigger the bug? I added those two connects locally (without the rest of the patch) and kjobtrackerstest still works fine.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is there a way to see the bug without ark?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The ark code mixes KJob with QThread, which KJob wasn't meant for. It's an interesting experiment, but I have my doubts as to whether it's done correctly...</p></pre>
</blockquote>
<p>On June 16th, 2016, 10:05 a.m. UTC, <b>Anthony Fieroni</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;">Ark calls in this order
terminate
unregister
terminate
unregister
Yes, make it 2 times</p></pre>
</blockquote>
<p>On June 16th, 2016, 10:32 a.m. UTC, <b>Anthony Fieroni</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;">Or KJob' pointer is danglig when try to disconnect it cause a crash.</p></pre>
</blockquote>
<p>On June 16th, 2016, 12:27 p.m. UTC, <b>Elvis Angelaccio</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The ark code mixes KJob with QThread, which KJob wasn't meant for. It's an interesting experiment, but I have my doubts as to whether it's done correctly...</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've tried in the past the get rid of that QThread, but with little success. How else can you handle a long-lasting KJob without freezing the UI?</p></pre>
</blockquote>
<p>On June 16th, 2016, 12:40 p.m. UTC, <b>David Faure</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;">Most KJobs are async operations (e.g. network downloads), so such long-lasting KJobs in the main thread are fine, there's plenty of opportunity to go back to the event loop and refresh the GUI.
If this is mostly a computational job then indeed a separate thread (or process) is needed. It just shouldn't mess with the KJob, i.e. the KJob must be "one layer on top of the thread", and fully running in the main thread.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anthony: which terminate method are you talking about?
And I did explain why the KJob pointer is dangling (deleted by secondary thread due to wrong direct connection).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Remove that direct connection, debug bug 193908 again, use helgrind, then we can come back to this bug if it's still there.</p></pre>
</blockquote>
<p>On June 16th, 2016, 1:34 p.m. UTC, <b>Anthony Fieroni</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 mean:
finished
unregister
finished
unregister
So bug is fixed in the master, i offer partial fix only for ark 16.04 branch
Here for me must be:
if (!d->progressJobView.contains(job))
return;
KJobTrackerInterface::unregisterJob(job);</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;">That's still a workaround. There's no reason for finished to be emitted twice for a given job. If this is indeed what's happening, please debug why it happens.</p></pre>
<br />
<p>- David</p>
<br />
<p>On June 10th, 2016, 5:07 a.m. UTC, Anthony Fieroni 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 Frameworks and David Faure.</div>
<div>By Anthony Fieroni.</div>
<p style="color: grey;"><i>Updated June 10, 2016, 5:07 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kjobwidgets
</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;"><ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Fix memory leak in finished</li>
</ul></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;">ark --changetofirstpath --add --autofilename zip kjobwidgets
Crash before, fix with patch</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>src/kuiserverjobtracker.cpp <span style="color: grey">(ebed3a5)</span></li>
<li>tests/kjobtrackerstest.cpp <span style="color: grey">(7ef9e07)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128113/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/5b87387e-7e7b-4982-b91b-a18f72414509__backtrace">backtrace</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/07/ba4b0150-5e01-4cc1-8776-7530d053d6f0__memcheck">memcheck</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/07/f8813ccc-8835-4255-842c-29c57c2dea23__memcheck2">memcheck 7 errorrs</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>