Fix xml file saving on Windows
ClosedPublic

Authored by ocoole on Jun 10 2018, 12:20 PM.

Details

Test Plan

Only tested on Windows/msvc2017. (Shouldn't break other platforms though.)

  1. Open an existing kmy/xml file. Made some changes to it. And Save.

Save operation should complete without error.

  1. Verify that the file is present in its location.

Previously save operation on Windows only succeeds every other time. The issue lies in calling QFile::rename() on a QTemporaryFile:

xmlstorage.cpp
void XMLStorage::saveToLocalFile(const QString& localFile, IMyMoneyOperationsFormat* pWriter, bool plaintext, const QString& keyList) 
{ 
...
  // Create a temporary file if needed
  QString writeFile = localFile;
  QTemporaryFile tmpFile;
  if (QFile::exists(localFile)) {
    tmpFile.open();
    writeFile = tmpFile.fileName();
    tmpFile.close();
  }
...
  if (writeFile != localFile) {
    // [writeFile is in effect tmpFile in this context]
    if (!QFile::remove(localFile) || !QFile::rename(writeFile, localFile))
      throw MYMONEYEXCEPTION(QString::fromLatin1("Failure while writing to '%1'").arg(localFile));
  }

(As explained inline in the patch) QTemporaryFile actually keeps the file handle open even after close(). QFile::rename(src, dst) only succeeds when src is 'renamed' (or 'moved' in this context) to dst. As src (i.e. tmpFile) is kept open by QTemporaryFile, the function fails with an error "Cannot remove source file".

This appears to be a bug specific to Windows as Windows does not allow moving files in use.

A quick search reveals it's been a subject of several bug reports: https://bugreports.qt.io/browse/QTBUG-13388, https://bugreports.qt.io/browse/QTBUG-10856, https://bugreports.qt.io/browse/QTBUG-5978; all of which were resolved saying it's "by design".

The fix I adopt is relatively simple: I turn to QFile::copy() instead (which doesn't remove the source file), and just let the QTemporaryFile (the 'source file') deletes itself when it falls out of scope.

Though simple the fix might seem, the consequence of the bug itself could be grave: as shown in the above snippet, when the error is thrown, the original file is already deleted and never restored—a great risk of data loss.

Pressing Save again succeeds though—since the original file doesn't exist and no QTemporaryFile is used. (So Save succeeds on every second time.)

So this patch, I think, should also be backported to the 5.x branch (though there hasn't a Windows release yet)—a Windows release for kmm5 should, in any event, include this patch to prevent some inadvertent data loss.

Diff Detail

Repository
R261 KMyMoney
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ocoole requested review of this revision.Jun 10 2018, 12:20 PM
ocoole created this revision.
tbaumgart accepted this revision.Jun 10 2018, 1:53 PM
tbaumgart added a subscriber: tbaumgart.

Said behavior of MS-Windows based operating systems is a PITA and known for many moons. Nevertheless, we always get caught by it. I added habackeras subscriber so that he may check if this also needs to be backported to the 4.8 branch.

This revision is now accepted and ready to land.Jun 10 2018, 1:53 PM

Thanks Thomas! (I actually thought I should have discovered this earlier; apparently I didn't test it out very comprehensively back then.)

(I also used kmm 4.8.0 (not the latest 4.8.2 though) on Windows—which seems to work fine.)

(Just by way of background (might be useful reference for you or Ralf)—the original review / diff of the relevant part of the code when it first landed)

I added habackeras subscriber so that he may check if this also needs to be backported to the 4.8 branch.

4.8 branch is not affected - It uses KTemporaryFile for the temporary file and KIO to copy the file to the destination (see https://cgit.kde.org/kmymoney.git/tree/kmymoney/views/kmymoneyview.cpp?h=4.8#n1275)

(I also used kmm 4.8.0 (not the latest 4.8.2 though) on Windows—which seems to work fine.)

4.8.2 is also not affected, see D13465#276986

Question regarding backporting: Is it correct that this does not affect 4.8 branch at all, or just not the Windows build? I don't know if any Linux distros will need another 4.8 release before being able to fully migrate to 5.x Also, does this need backporting to 5.0 branch or will next release be 5,1 and not 5.0.2?

Sorry - I see this is only supposed to affect Windows, so questions about Linux are unnecessary.

@ostroffjh you answered the Windows part yourself already. In fact, I am thinking we should stabilize 5.1 and release it next. It has some improvements regarding the plugins so that is probably worth it. Nevertheless, I planned to cherry-pick onto the 5.0 branch once it landed. @ocoole Do you have write access to the repo or shall I land it?

This revision was automatically updated to reflect the committed changes.