Review Request 128113: [kuiserverjobtracker] Fix crash when unregister job

[Anthony Fieroni]

> On Юни 16, 2016, 10:57 преди обяд, David Faure wrote:
> > tests/kjobtrackerstest.cpp, line 173
> > <https://git.reviewboard.kde.org/r/128113/diff/4/?file=468220#file468220line173>
> >
> >     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.
> >     
> >     Is there a way to see the bug without ark?
> >     
> >     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...
> 
> Anthony Fieroni wrote:
>     Ark calls in this order
>     terminate
>     unregister
>     terminate
>     unregister
>     Yes, make it 2 times
> 
> Anthony Fieroni wrote:
>     Or KJob' pointer is danglig when try to disconnect it cause a crash.
> 
> Elvis Angelaccio wrote:
>     > 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...
>     
>     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?
> 
> David Faure wrote:
>     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.
>     
>     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).
>     
>     Remove that direct connection, debug bug 193908 again, use helgrind, then we can come back to this bug if it's still there.

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);


- Anthony


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128113/#review96558
-----------------------------------------------------------


On Юни 10, 2016, 8:07 преди обяд, Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> -----------------------------------------------------------
> 
> (Updated Юни 10, 2016, 8:07 преди обяд)
> 
> 
> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160616/653fd61d/attachment.html>