Re: 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
> > 
> >
> > 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.
> 
> Anthony Fieroni wrote:
> 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);

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.


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Anthony Fieroni]

> On Юни 16, 2016, 10:57 преди обяд, David Faure wrote:
> > tests/kjobtrackerstest.cpp, line 173
> > 
> >
> > 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Elvis Angelaccio]

> 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(), ::finished, this, 
> > ::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(), ::finished, this, 
::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=commit=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

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

[Elvis Angelaccio]

> On June 16, 2016, 7:57 a.m., David Faure wrote:
> > tests/kjobtrackerstest.cpp, line 173
> > 
> >
> > 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.

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


- Elvis


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Anthony Fieroni]

> On Юни 16, 2016, 10:57 преди обяд, David Faure wrote:
> > tests/kjobtrackerstest.cpp, line 173
> > 
> >
> > 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

Or KJob' pointer is danglig when try to disconnect it cause a crash.


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Anthony Fieroni]

> On Юни 16, 2016, 11:49 преди обяд, 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(), ::finished, this, 
> > ::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).

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


- Anthony


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Anthony Fieroni]

> On Юни 16, 2016, 10:57 преди обяд, David Faure wrote:
> > tests/kjobtrackerstest.cpp, line 173
> > 
> >
> > 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...

Ark calls in this order
terminate
unregister
terminate
unregister
Yes, make it 2 times


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[David Faure]

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



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(), ::finished, this, 
::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).

- David Faure


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

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

[David Faure]

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




tests/kjobtrackerstest.cpp (line 173)


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


- David Faure


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

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

[Albert Astals Cid]

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



You mention a memory leak fix, but all i can see is delete -> deleteLater, how 
does that fix a leak?

- Albert Astals Cid


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

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

[Anthony Fieroni]

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



Ping, this is surly a bug.

- Anthony Fieroni


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Anthony Fieroni]

> On Юни 9, 2016, 6:22 преди обяд, Anthony Fieroni wrote:
> > Ping.
> 
> Elvis Angelaccio wrote:
> This doesn't seem to fix the crash with Ark 16.04.1
> 
> Anthony Fieroni wrote:
> Backtrace?
> 
> Elvis Angelaccio wrote:
> > Backtrace?
> 
> https://paste.kde.org/pne0suscq

kjobswidgets not present in backtrace, how can patch affects it's?


- Anthony


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Elvis Angelaccio]

> On June 9, 2016, 3:22 a.m., Anthony Fieroni wrote:
> > Ping.
> 
> Elvis Angelaccio wrote:
> This doesn't seem to fix the crash with Ark 16.04.1
> 
> Anthony Fieroni wrote:
> Backtrace?

> Backtrace?

https://paste.kde.org/pne0suscq


- Elvis


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


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

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

[Anthony Fieroni]

> On Юни 9, 2016, 6:22 преди обяд, Anthony Fieroni wrote:
> > Ping.
> 
> Elvis Angelaccio wrote:
> This doesn't seem to fix the crash with Ark 16.04.1

Backtrace?


- Anthony


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Anthony Fieroni]

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


Changes
---

+ Test case


Repository: kjobwidgets


Description
---

+ Fix memory leak in finished


Diffs (updated)
-

  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

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

[Aleix Pol Gonzalez]

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



Would it maybe be possible to get an autotest case for this?

- Aleix Pol Gonzalez


On June 8, 2016, 6:17 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> ---
> 
> (Updated June 8, 2016, 6:17 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> + Fix memory leak in finished
> 
> 
> Diffs
> -
> 
>   src/kuiserverjobtracker.cpp ebed3a5 
> 
> 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

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

[Elvis Angelaccio]

> On June 9, 2016, 3:22 a.m., Anthony Fieroni wrote:
> > Ping.

This doesn't seem to fix the crash with Ark 16.04.1


- Elvis


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


On June 8, 2016, 4:17 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> ---
> 
> (Updated June 8, 2016, 4:17 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> + Fix memory leak in finished
> 
> 
> Diffs
> -
> 
>   src/kuiserverjobtracker.cpp ebed3a5 
> 
> 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

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

[Anthony Fieroni]

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



Ping.

- Anthony Fieroni


On Юни 8, 2016, 7:17 преди обяд, Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> ---
> 
> (Updated Юни 8, 2016, 7:17 преди обяд)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> + Fix memory leak in finished
> 
> 
> Diffs
> -
> 
>   src/kuiserverjobtracker.cpp ebed3a5 
> 
> 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

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

[Anthony Fieroni]

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

(Updated Юни 8, 2016, 7:17 преди обяд)


Review request for KDE Frameworks and David Faure.


Repository: kjobwidgets


Description
---

+ Fix memory leak in finished


Diffs (updated)
-

  src/kuiserverjobtracker.cpp ebed3a5 

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

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

[Anthony Fieroni]

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

(Updated Юни 7, 2016, 10:24 след обяд)


Review request for KDE Frameworks and David Faure.


Changes
---

David, the problem is KUiServerJobTracker::unregisterJob is not reentrant. 
KJobTrackerInterface::unregisterJob(job) trys to disconnect a job twice. Now 
issues is completely fixed, 20/20 not crash. Elvis, can you try against ark 
master?


Repository: kjobwidgets


Description
---

+ Fix memory leak in finished


Diffs (updated)
-

  src/kuiserverjobtracker.cpp ebed3a5 

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

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

[Anthony Fieroni]

> On Юни 7, 2016, 12:03 преди обяд, Elvis Angelaccio wrote:
> > Are we talking about this bug? https://bugs.kde.org/show_bug.cgi?id=362977
> > 
> > If yes, it's already fixed on Ark master.
> 
> Anthony Fieroni wrote:
> It's not this, it happens on all backend.
> 
> Elvis Angelaccio wrote:
> It happens with all CLI-based backends (zip, rar, ...) but it's the same 
> bug. We fixed this on master by reworking how Ark handles external processes.

I still think patch is relevant. finished is needed to delete object cause a 
leak, and due to async nature, dbus interface makes non block call, it 
shouldn't be delete obj. Maybe it's better to be
*QObject::connect(job, ::finished, jobView, ::deleteLater);*


- Anthony


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


On Юни 7, 2016, 6:28 преди обяд, Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> ---
> 
> (Updated Юни 7, 2016, 6:28 преди обяд)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> + Fix memory leak in finished
> 
> 
> Diffs
> -
> 
>   src/kuiserverjobtracker.cpp ebed3a5 
> 
> 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

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

[Elvis Angelaccio]

> On June 6, 2016, 9:03 p.m., Elvis Angelaccio wrote:
> > Are we talking about this bug? https://bugs.kde.org/show_bug.cgi?id=362977
> > 
> > If yes, it's already fixed on Ark master.
> 
> Anthony Fieroni wrote:
> It's not this, it happens on all backend.

It happens with all CLI-based backends (zip, rar, ...) but it's the same bug. 
We fixed this on master by reworking how Ark handles external processes.


- Elvis


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


On June 7, 2016, 3:28 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> ---
> 
> (Updated June 7, 2016, 3:28 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> + Fix memory leak in finished
> 
> 
> Diffs
> -
> 
>   src/kuiserverjobtracker.cpp ebed3a5 
> 
> 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

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

[Anthony Fieroni]

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

(Updated Юни 7, 2016, 6:28 преди обяд)


Review request for KDE Frameworks and David Faure.


Repository: kjobwidgets


Description
---

+ Fix memory leak in finished


Diffs
-

  src/kuiserverjobtracker.cpp ebed3a5 

Diff: https://git.reviewboard.kde.org/r/128113/diff/


Testing
---

ark --changetofirstpath --add --autofilename zip kjobwidgets
Crash before, fix with patch


File Attachments (updated)


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

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

[Anthony Fieroni]

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

(Updated Юни 7, 2016, 6:25 преди обяд)


Review request for KDE Frameworks and David Faure.


Repository: kjobwidgets


Description
---

+ Fix memory leak in finished


Diffs
-

  src/kuiserverjobtracker.cpp ebed3a5 

Diff: https://git.reviewboard.kde.org/r/128113/diff/


Testing
---

ark --changetofirstpath --add --autofilename zip kjobwidgets
Crash before, fix with patch


File Attachments (updated)


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


Thanks,

Anthony Fieroni

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Anthony Fieroni]

> On Юни 6, 2016, 11:40 след обяд, David Faure wrote:
> > I'm not fully convinced by the fix, because it was crashing in disconnect, 
> > which sounds like the pointer was dangling, therefore deleteLater could 
> > crash too.
> > In addition I can't reproduce the fix here.
> > Finally, this code was already in kdelibs4, so -something- else must have 
> > changed.
> > 
> > This is worth a more through investigation. If you can still reproduce the 
> > crash after reverting your change, can you run that command in valgrind, so 
> > we know exactly what's happening?
> > 
> > I recommend this alias:
> > alias memcheck="QT_ENABLE_REGEXP_JIT=0 valgrind --tool=memcheck 
> > --partial-loads-ok=yes  --read-var-info=yes"

Added memcheck.


- Anthony


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


On Юни 7, 2016, 6:25 преди обяд, Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> ---
> 
> (Updated Юни 7, 2016, 6:25 преди обяд)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> + Fix memory leak in finished
> 
> 
> Diffs
> -
> 
>   src/kuiserverjobtracker.cpp ebed3a5 
> 
> 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
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Anthony Fieroni]

> On Юни 7, 2016, 12:03 преди обяд, Elvis Angelaccio wrote:
> > Are we talking about this bug? https://bugs.kde.org/show_bug.cgi?id=362977
> > 
> > If yes, it's already fixed on Ark master.

It's not this, it happens on all backend.


- Anthony


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


On Юни 6, 2016, 10:53 след обяд, Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> ---
> 
> (Updated Юни 6, 2016, 10:53 след обяд)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> + Fix memory leak in finished
> 
> 
> Diffs
> -
> 
>   src/kuiserverjobtracker.cpp ebed3a5 
> 
> 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
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Elvis Angelaccio]

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



Are we talking about this bug? https://bugs.kde.org/show_bug.cgi?id=362977

If yes, it's already fixed on Ark master.

- Elvis Angelaccio


On June 6, 2016, 7:53 p.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> ---
> 
> (Updated June 6, 2016, 7:53 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> + Fix memory leak in finished
> 
> 
> Diffs
> -
> 
>   src/kuiserverjobtracker.cpp ebed3a5 
> 
> 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
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[David Faure]

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



I'm not fully convinced by the fix, because it was crashing in disconnect, 
which sounds like the pointer was dangling, therefore deleteLater could crash 
too.
In addition I can't reproduce the fix here.
Finally, this code was already in kdelibs4, so -something- else must have 
changed.

This is worth a more through investigation. If you can still reproduce the 
crash after reverting your change, can you run that command in valgrind, so we 
know exactly what's happening?

I recommend this alias:
alias memcheck="QT_ENABLE_REGEXP_JIT=0 valgrind --tool=memcheck 
--partial-loads-ok=yes  --read-var-info=yes"

- David Faure


On June 6, 2016, 7:53 p.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128113/
> ---
> 
> (Updated June 6, 2016, 7:53 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> + Fix memory leak in finished
> 
> 
> Diffs
> -
> 
>   src/kuiserverjobtracker.cpp ebed3a5 
> 
> 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
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Anthony Fieroni]

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

Review request for KDE Frameworks and David Faure.


Repository: kjobwidgets


Description
---

+ Fix memory leak in finished


Diffs
-

  src/kuiserverjobtracker.cpp ebed3a5 

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


Thanks,

Anthony Fieroni

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel