Calling finished() after error() in KIO slaves causes data corruption and crashes

[David Faure]

On Tuesday 18 March 2008, Vlad wrote:
> Hi,
> 
> --- David Faure <faure at kde.org> wrote:
> > On Sunday 16 March 2008, Vlad wrote:
> > > Another solution would be to transform the documentation guidance
> > into
> > > a rule and try to enforce it.
> > 
> > That's how we handled it until now.
> > All non-crashing slaves do exactly that: emitting nothing else after
> > error().
> 
> Makes sense, but I think the documentation for SlaveBase::error()
> should be stricter, such as:
> 
> "This also finishes the job, so you must not call finished after
> calling this."
> 
> I'll update this message if that's OK. I'll also add a comment to
> SlaveBase::finished().

Yes please.

> > > However, that is going to be hard 
> > > because finding existing instances where finished() is called
> > after
> > > error() is not trivial. Frankly, I can see how the instinct of KIO
> > > slave writers will be to still call finished() after error(), just
> > to
> > > be safe and make sure it's finished.
> > 
> > Yes, we could add some robustness there inside slavebase, but I
> > don't
> > really like "noop" calls, so I guess it would have to print a
> > warning
> > about "don't do that", too.
> 
> Agreed, ignoring a call without printing a warning would just hide
> this bug. So how do we know when to print a warning? One possibility
> is to correlate each SlaveBase method with the job that requested it.
> This could be done by passing the requester job's unique ID (ie.
> memory address) as a parameter to the SlaveBase method. Whenever the
> SlaveBase method reports back by emitting a signal such as finished()
> or error(), it will pass the requester job's ID as a parameter in that
> signal. If we detect that the recipient job doesn't match the
> requester job's ID, we block the signal and print a warning.

I think this is far too complex.

It should be enough in SlaveBase to have a member var with three states:
enum { Idle, InsideMethod, FinishedCalled, ErrorCalled } m_state;
d->m_state would be set to InsideMethod before calling get(),
and in both finished() you would say
 if (d->m_state == FinishedCalled) {
  kWarning() << "finished() called twice!";
 } else if (d->m_state == ErrorCalled) {
  kWarning() << "finished() called after error()!";
 }
and similar in error().
And when get() returns, you set the state to Idle again. You could even check
that the state was FinishedCalled or ErrorCalled.

The point is that get() (and all other operations) is a blocking method call. So there's no
need for any job ID: you know that it's done when it returns, and there can only be one
operation at a time.

-- 
David Faure, faure at kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).