Race condition with data loss in ReadWritePart

[David Faure]

On Tuesday 26 February 2008, Martin Pley wrote:
> Am Sonntag 24 Februar 2008 19:36:18 schrieb Ingo Klöcker:
> > On Sunday 24 February 2008, Martin Pley wrote:
> > > Am Sonntag, 24. Februar 2008 13:18:17 schrieb Ingo Klöcker:
> > > > On Saturday 23 February 2008, Martin Pley wrote:
> > > > > > I noticed a race condition with data loss in ReadWritePart:
> > > > > >
> > > > > > Try to open a non-local-file from a slow or offline server.
> > > > > > Then, instead of waiting for the file, just open another local
> > > > > > file. Now, when you delete the part, the file is deleted(!).
> > > > > >
> > > > > > If I see it correctly, this is because m_bTemp is set to "true"
> > > > > > while opening the remote file. And when you open the local file
> > > > > > it will not be set to false again.
> > > > >
> > > > > It's actually not a race condition. I reimplemented
> > > > > ReadWritePart::closeURL() without setting m_bTemp to false. And
> > > > > when I delete the part, ~ReadOnlyPart calls
> > > > > ReadOnlyPart::closeURL(), which deletes the file.
> > > > >
> > > > > Perhaps one should add a warning to the documentation of
> > > > > closeURL().

Yes, at least.

> > > > There's no need for adding a warning. We just need to call
> > > > closeURL() from ~ReadWritePart.

There's a comment against that in the code already.
ReadWritePart::~ReadWritePart()
{
    // parent destructor will delete temp file
    // we can't call our own closeUrl() here, because
    // "cancel" wouldn't cancel anything. We have to assume
    // the app called closeUrl() before destroying us.
}

> >
> > After having a quick look at the code to me the problem seems to be that
> > m_bTemp is not set to false in ReadOnlyPart::openURL() in
> >
> >   if ( d->m_url.isLocalFile() )
> >   { ... }
> >
> > while it's set to true in the corresponding else-branch. This would help
> > in your case, right?
> 
> That would have helped to avoid data loss, yes.

Done now. Better late than never...

> But how about writing a ReadWritePart::openURL(), which calls 
> ReadWritePart::closeURL() first? Then ReadOnlyPart::openURL() may just call 
> ReadOnlyPart::closeURL(). What do you think?

I don't understand this one. ReadOnlyPart::openUrl() already calls closeUrl
which is virtual so it does call ReadWritePart::closeUrl() [or your reimplementation of it].

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