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

[David Faure]

> On June 16, 2016, 7:57 a.m., 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?

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.


- David


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


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

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