> On June 16, 2016, 8:49 a.m., David Faure wrote:
> > Looking at https://git.reviewboard.kde.org/r/128150 (to basically read the
> > libkerfuffle code), I see what the problem is.
> >
> > You're mixing KJob and QThread, which is only fine if none of the KJob API
> > is called from the secondary thread. I.e. the KJob should only sit on top
> > of the thread and report its progress (e.g. via signals emitted by the
> > thread). This is almost what you're doing, except for this:
> > connect(archiveInterface(), &ReadOnlyArchiveInterface::finished, this,
> > &Job::onFinished, Qt::DirectConnection);
> > Due to the DirectConnection, this will call onFinished, and therefore
> > KJob::emitResult, from the secondary thread. This leads to race conditions
> > inside KJob, and it leads to deletion of the job happening in the wrong
> > thread.
> > This should definitely not be a direct connection.
> >
> > This would also fix the race on m_isRunning that your code currently has
> > (read/written in main thread (isRunning() and start()), but set to true in
> > secondary thread in emitResult()).
> >
> > You should use helgrind (http://www.kdab.com/~dfaure/helgrind.html) on that
> > code to make sure there are no other race conditions.
> >
> > Conclusion from this analysis: I don't think there's a bug in kjobwidgets,
> > until proven otherwise (preferably with an actual test or unittest that
> > leads to a crash).
>
> Anthony Fieroni wrote:
> unregisterJob cannot be called more than ones or it trys to disconect
> self more than ones. So Qt must be smart enough, but it shouldn't. Fix is
> pretty easy, check must be *bofore* parent call and job must be removed from
> list.
> if (!d->progressJobView.contains(job))
> ...
> KJobTrackerInterface::unregisterJob(job);
> You're mixing KJob and QThread, which is only fine if none of the KJob API is
> called from the secondary thread. I.e. the KJob should only sit on top of the
> thread and report its progress (e.g. via signals emitted by the thread). This
> is almost what you're doing, except for this:
connect(archiveInterface(), &ReadOnlyArchiveInterface::finished, this,
&Job::onFinished, Qt::DirectConnection);
Due to the DirectConnection, this will call onFinished, and therefore
KJob::emitResult, from the secondary thread. This leads to race conditions
inside KJob, and it leads to deletion of the job happening in the wrong thread.
This should definitely not be a direct connection.
The direct connection was added by this commit:
https://quickgit.kde.org/?p=ark.git&a=commit&h=acb455da04c473da39a5d99d4212f1d9c88abee5
Last time I checked, it was still needed to not re-introduce the related bug.
I'll have another look (Ark master changed *a lot*)...
- Elvis
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128113/#review96567
-----------------------------------------------------------
On June 10, 2016, 5:07 a.m., Anthony Fieroni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> -----------------------------------------------------------
>
> (Updated June 10, 2016, 5:07 a.m.)
>
>
> Review request for KDE Frameworks and David Faure.
>
>
> Repository: kjobwidgets
>
>
> Description
> -------
>
> + Fix memory leak in finished
>
>
> Diffs
> -----
>
> src/kuiserverjobtracker.cpp ebed3a5
> tests/kjobtrackerstest.cpp 7ef9e07
>
> Diff: https://git.reviewboard.kde.org/r/128113/diff/
>
>
> Testing
> -------
>
> ark --changetofirstpath --add --autofilename zip kjobwidgets
> Crash before, fix with patch
>
>
> File Attachments
> ----------------
>
> backtrace
>
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/5b87387e-7e7b-4982-b91b-a18f72414509__backtrace
> memcheck
>
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/07/ba4b0150-5e01-4cc1-8776-7530d053d6f0__memcheck
> memcheck 7 errorrs
>
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/07/f8813ccc-8835-4255-842c-29c57c2dea23__memcheck2
>
>
> Thanks,
>
> Anthony Fieroni
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
