QFile::moveToTrash: fix error reporting on Windows

If a move-to-trash operation failed, e.g. because the file was opened by
another process (or QFile), then the moveToTrash function would still
return true.

MSDN documents the IFileOperation::PerformOperations to return whether
the operation succeeded, but evidently this is only a statement about
the execution of queued up operations, not a statement about any of the
operations' success.

If the operation succeeded is reported by an HRESULT parameter
of the IFileOperationProgressSink::PostDeleteItem implementation,
and we ignored that parameter so far.

Check it via the SUCCEEDED macro, and set a boolean sink variable based
on that, which we can inspect to return the correct value.

Augment the test case by opening those files we create ourselves, and
if that fails (which it will on Windows, but not necessarily on other
platforms), then try again after closing the file. If the first attempt
succeeded, then the source file must also be gone.

Pick-to: 6.6 6.5 6.2 5.15
Fixes: QTBUG-117383
Done-With: Thiago Macieira <thiago.macieira@intel.com>
Change-Id: Icb82a0c9d3b337585dded622d6656e07dee33d84
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

3 files changed, 140 insertions, 2 deletions

diff --git a/src/corelib/io/qfilesystemengine_win.cpp b/src/corelib/io/qfilesystemengine_win.cpp
index 49f375c4bb..3abbfa3536 100644
--- a/src/corelib/io/qfilesystemengine_win.cpp
+++ b/src/corelib/io/qfilesystemengine_win.cpp
@@ -840,9 +840,10 @@ public:
         return (dwFlags & TSF_DELETE_RECYCLE_IF_POSSIBLE) ? S_OK : E_FAIL;
     }
     HRESULT STDMETHODCALLTYPE PostDeleteItem(DWORD /* dwFlags */, IShellItem * /* psiItem */,
-                                             HRESULT /* hrDelete */,
+                                             HRESULT hrDelete,
                                              IShellItem *psiNewlyCreated) override
     {
+        deleteResult = hrDelete;
         if (psiNewlyCreated) {
             wchar_t *pszName = nullptr;
             psiNewlyCreated->GetDisplayName(SIGDN_FILESYSPATH, &pszName);
@@ -863,6 +864,7 @@ public:
     HRESULT STDMETHODCALLTYPE ResumeTimer() override { return S_OK; }
 
     QString targetPath;
+    HRESULT deleteResult = S_OK;
 private:
     ULONG ref;
 };
@@ -1833,8 +1835,12 @@ bool QFileSystemEngine::moveFileToTrash(const QFileSystemEntry &source,
     hres = pfo->PerformOperations();
     if (!SUCCEEDED(hres))
         return false;
-    newLocation = QFileSystemEntry(sink->targetPath);
 
+    if (!SUCCEEDED(sink->deleteResult)) {
+        error = QSystemError(sink->deleteResult, QSystemError::NativeError);
+        return false;
+    }
+    newLocation = QFileSystemEntry(sink->targetPath);
     return true;
 }
 
diff --git a/tests/auto/corelib/io/qfile/tst_qfile.cpp b/tests/auto/corelib/io/qfile/tst_qfile.cpp
index a668a6345b..5fda0c04e3 100644
--- a/tests/auto/corelib/io/qfile/tst_qfile.cpp
+++ b/tests/auto/corelib/io/qfile/tst_qfile.cpp
@@ -288,6 +288,8 @@ private slots:
 
     void moveToTrash_data();
     void moveToTrash();
+    void moveToTrashOpenFile_data();
+    void moveToTrashOpenFile();
 
     void stdfilesystem();
 
@@ -4057,6 +4059,86 @@ void tst_QFile::moveToTrash()
     }
 }
 
+void tst_QFile::moveToTrashOpenFile_data()
+{
+    QTest::addColumn<bool>("useStatic");
+    QTest::addColumn<bool>("success");
+
+    // QFile::moveToTrash() non-static member closes the file before trashing,
+    // so this must always succeed.
+    QTest::newRow("member") << false << true;
+
+    // QFile::moveToTrash() static member cannot close the file because it
+    // operates on another QFile, so this operation will fail on OSes that do
+    // not permit deleting open files.
+    QTest::newRow("static") << true
+#ifdef Q_OS_WIN
+                            << false;
+#else
+                            << true;
+#endif
+}
+
+void tst_QFile::moveToTrashOpenFile()
+{
+#if defined(Q_OS_ANDROID) || defined(Q_OS_WEBOS)
+    QSKIP("This platform doesn't implement a trash bin");
+#endif
+    QFETCH(bool, useStatic);
+    QFETCH(bool, success);
+    const QByteArrayView contents = "Hello, World\n";
+
+    QString newFileName, origFileName;
+    auto cleanup = qScopeGuard([&] {
+        if (!origFileName.isEmpty())
+            QFile::remove(origFileName);
+        if (!newFileName.isEmpty() && newFileName != origFileName)
+            QFile::remove(newFileName);
+    });
+
+    origFileName = []() {
+        QTemporaryFile temp(QDir::homePath() + "/tst_qfile.moveToTrashOpenFile.XXXXXX");
+        temp.setAutoRemove(false);
+        if (!temp.open())
+            qWarning("Failed to create temporary file: %ls", qUtf16Printable(temp.errorString()));
+        return temp.fileName();
+    }();
+
+    QFile f;
+    f.setFileName(origFileName);
+    QVERIFY2(f.open(QIODevice::ReadWrite | QIODevice::Unbuffered), qPrintable(f.errorString()));
+    f.write(contents.data(), contents.size());
+
+    QString errorString;
+    auto doMoveToTrash = [&](QFile *f) {
+        if (!f->moveToTrash())
+            errorString = f->errorString();
+        newFileName = f->fileName();
+    };
+    if (useStatic) {
+        // it's the same as the static QFile::moveToTrash(), but gives us
+        // the error string
+        QFile other(origFileName);
+        doMoveToTrash(&other);
+    } else {
+        doMoveToTrash(&f);
+    }
+    QCOMPARE_NE(f.fileName(), QString());
+
+    if (success) {
+        QCOMPARE(errorString, QString());
+        QCOMPARE_NE(newFileName, origFileName);         // must have changed!
+        QVERIFY(!QFile::exists(origFileName));
+        QVERIFY(QFile::exists(newFileName));
+        QCOMPARE(QFileInfo(newFileName).size(), contents.size());
+    } else {
+        QCOMPARE_NE(errorString, QString());
+        QCOMPARE(newFileName, origFileName);            // mustn't have changed!
+        QVERIFY(QFile::exists(origFileName));
+        QCOMPARE(QFileInfo(origFileName).size(), contents.size());
+    }
+}
+
 void tst_QFile::stdfilesystem()
 {
 #if QT_CONFIG(cxx17_filesystem)
diff --git a/tests/auto/corelib/io/qtemporaryfile/tst_qtemporaryfile.cpp b/tests/auto/corelib/io/qtemporaryfile/tst_qtemporaryfile.cpp
index 0191c9a3e7..f8e3a9d744 100644
--- a/tests/auto/corelib/io/qtemporaryfile/tst_qtemporaryfile.cpp
+++ b/tests/auto/corelib/io/qtemporaryfile/tst_qtemporaryfile.cpp
@@ -64,6 +64,7 @@ private slots:
     void stressTest();
     void rename();
     void renameFdLeak();
+    void moveToTrash();
     void reOpenThroughQFile();
     void keepOpenMode();
     void resetTemplateAfterError();
@@ -626,6 +627,55 @@ void tst_QTemporaryFile::renameFdLeak()
 #endif
 }
 
+void tst_QTemporaryFile::moveToTrash()
+{
+#if defined(Q_OS_ANDROID) || defined(Q_OS_WEBOS)
+    QSKIP("This platform doesn't implement a trash bin");
+#endif
+#ifdef Q_OS_WIN
+    // QTemporaryFile won't really close the file with close(), so this is
+    // expected to fail with a sharing violation error.
+    constexpr bool expectSuccess = false;
+#else
+    constexpr bool expectSuccess = true;
+#endif
+    const QByteArrayView contents = "Hello, World\n";
+
+    QTemporaryFile f(QDir::homePath() + "/tst_qtemporaryfile.moveToTrash.XXXXXX");
+    QString origFileName;
+    auto cleanup = qScopeGuard([&] {
+        if (!origFileName.isEmpty())
+            QFile::remove(origFileName);
+        if (QString fn = f.fileName(); !fn.isEmpty() && fn != origFileName)
+            QFile::remove(fn);
+    });
+
+    if (!f.open())
+        QSKIP("Failed to create temporary file");
+    f.write(contents.data(), contents.size());
+
+    // we need an actual file name:
+    // 1) so we can delete it in the clean-up guard in case we fail to trash
+    // 2) so that the file exists on Linux in the first place (no sense in
+    //    trashing an unnamed file)
+    origFileName = f.fileName();
+
+    if (expectSuccess) {
+        QVERIFY2(f.moveToTrash(), qPrintable(f.errorString()));
+        QCOMPARE_NE(f.fileName(), origFileName);        // must have changed!
+        QCOMPARE_NE(f.fileName(), QString());
+        QVERIFY(!QFile::exists(origFileName));
+        QVERIFY(QFile::exists(f.fileName()));
+        QCOMPARE(QFileInfo(f.fileName()).size(), contents.size());
+    } else {
+        QVERIFY(!f.moveToTrash());
+        QCOMPARE(f.fileName(), origFileName);           // mustn't have changed!
+        QCOMPARE_NE(f.error(), QFile::NoError);
+        QCOMPARE_NE(f.errorString(), "Unknown error");
+        QVERIFY(QFile::exists(origFileName));
+    }
+}
+
 void tst_QTemporaryFile::reOpenThroughQFile()
 {
     QByteArray data("abcdefghij");