<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/108770/">http://git.reviewboard.kde.org/r/108770/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'd add the setParent(0), document it, and then this looks ok to me. But I'd like Kévin to be able to review it too.</pre>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/108770/diff/2/?file=112245#file112245line90" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/jobs/kcompositejob.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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="#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">90</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * Note that this will *not* delete the subjobs</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Right (but the dtor will, unlike before, due to the reparenting)....
I guess you're right, this means a setParent(0) would be better, to ensure behavior
compatibility with before. I don't think the subjobs can ever be dangling at that point, in the code I've seen:
The use case for this, in KIO and a few other places, is "kill quietly all subjobs, then clear the list" (so the jobs still exist at that point, the autodeletion is a deleteLater). Other code (found via lxr) kills all subjobs with EmitResult and then clear the list, this works as well.
</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/108770/diff/2/?file=112249#file112249line87" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/tests/kcompositejobtest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">87</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Q_ASSERT</span><span class="p">(</span><span class="n">success</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Use QVERIFY instead of Q_ASSERT, and then you can even combine the two lines:
QVERIFY(compositeJob->addSubjob(job));</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On February 5th, 2013, 3:39 p.m. UTC, Kevin Funk wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Kevin Funk.</div>
<p style="color: grey;"><i>Updated Feb. 5, 2013, 3:39 p.m.</i></p>
<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;">Fix double-free in ~KCompositeJobPrivate
In case a subjob of KCompositeJob has been deleted, this KCompositeJob
instance will crash as soon as it is being destructed, trying to delete
this subjob again. The reason for this is that KCompositeJob::addSubjob()
does not change the ownership of @p job. So, this job could be still
deleted by ~QObject() by the original parent.
Add tests for this corner case.
This fixes a bug in KDevelop.
Backtrace:
1 0x00007ffff7a3f28e in qDeleteAll<QList<KJob*>::const_iterator>
(end=..., begin=...) at /usr/include/qt4/QtCore/qalgorithms.h:322
2 qDeleteAll<QList<KJob*> > (c=QList<KJob *> = {...}) at
/usr/include/qt4/QtCore/qalgorithms.h:330 #3
KCompositeJobPrivate::~KCompositeJobPrivate (this=0x8849850,
__in_chrg=<optimized out>) at ../../kdecore/jobs/kcompositejob.cpp:29
4 0x00007ffff7a3f2c9 in KCompositeJobPrivate::~KCompositeJobPrivate
(this=0x8849850, __in_chrg=<optimized out>) at
../../kdecore/jobs/kcompositejob.cpp:30
5 0x00007ffff7a3fd70 in KJob::~KJob (this=0x880b030,
__in_chrg=<optimized out>) at
../../kdecore/jobs/kjob.cpp:73
6 0x00007ffff1a8e5d9 in KDevelop::BuilderJob::~BuilderJob
(this=0x880b030, __in_chrg=<optimized
out>) at /home/krf/devel/src/kdevplatform/project/builderjob.cpp:158
BUG: 230692
REVIEW: 108770
FIXED-IN: 4.11</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=230692">230692</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdecore/jobs/kcompositejob.h <span style="color: grey">(6ca8eed3ebf8c6f0f5c68d8843bd09a3ea928bbd)</span></li>
<li>kdecore/jobs/kcompositejob.cpp <span style="color: grey">(5ddabd71e5bbb5f0a555a201223a52950b85e786)</span></li>
<li>kdecore/tests/CMakeLists.txt <span style="color: grey">(f19e563d5d99ad2f2806140c5b21e38b20dbde0d)</span></li>
<li>kdecore/tests/kcompositejobtest.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kdecore/tests/kcompositejobtest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108770/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>