Review Request: Quit Eventloop before emitting finished and result signals

[Raphael Kubo da Costa]

On Wednesday 09 December 2009 13:57:00 Andreas Pakulat wrote:
> On 09.12.09 11:56:39, Raphael Kubo da Costa wrote:
> > On Wednesday 09 December 2009 05:22:09 Andreas Pakulat wrote:
> > > On 08.12.09 23:22:35, Raphael Kubo da Costa wrote:
> > > > On Wednesday 11 November 2009 16:38:42 Sebastian Sauer wrote:
> > > >
> > > > I believe this commit is causing bug 217836: basically,
> > > > d->eventLoop::quit() is called inside KJob::emitResult(), however the
> > > > slot connected to the result() signal is called long after the method
> > > > which called KJob::exec() finished, thus causing a segfault because
> > > > the slot tries to access data that's no longer present.
> > > >
> > > > If I use my own QEventLoop instead of calling KJob::exec() or revert
> > > > this commit, the segfault doesn't happen.
> > > >
> > > > Am I doing something unusual here?
> > >
> > > Sounds to me like you want to disable auto-deletion of the job because
> > > you need it around for longer than just the execution time.
> > >
> > > Andreas
> >
> > Why? Wouldn't it then be the case for any method that connects to a
> > KJob's result() signal and starts it with exec()? I was expecting that
> > deleteLater() would be called only after my slot had finished executing.
> 
> deleteLater() is called once the inner event loop is exited. However your
> description sounded as if your connecting the result signal to a slot via a
> queued connection (you said the slot connected to the result signal is
> called long after...). So its no wonder that in the meantime the outer
> eventloop executed and deleted the KJob. This matches the backtrace in said
> bugreport which shows that the slot is being called from the main
> applications event-loop. So if you want to access the job after the exec()
> method fininshed, you have to disable auto-deletion.

You're right, we ended up having a queued connection. I've committed a fix. So 
I guess everything is OK with this patch and there's no regression related to 
what I had reported.