Merge pull request #4586 from daschuer/lp1955331

Fix writing of track metadata on Windows

src/sources/metadatasourcetaglib.cpp

@@ -1,18 +1,22 @@
 #include "sources/metadatasourcetaglib.h"
 
-#include "track/taglib/trackmetadata.h"
-
-#include "util/logger.h"
-#include "util/memory.h"
+#include <taglib/vorbisfile.h>
 
 #include <QFile>
 #include <QFileInfo>
+#include <QThread>
 
-#include <taglib/vorbisfile.h>
+#include "track/taglib/trackmetadata.h"
+#include "util/logger.h"
+#include "util/memory.h"
 #if (TAGLIB_HAS_OPUSFILE)
 #include <taglib/opusfile.h>
 #endif
 
+#if defined(__WINDOWS__)
+#include <Windows.h>
+#endif
+
 namespace mixxx {
 
 namespace {
@@ -30,6 +34,11 @@ const QString kSafelyWritableTempFileSuffix = QStringLiteral("_temp");
 // potential failures caused by exceeded path length.
 const QString kSafelyWritableOrigFileSuffix = QStringLiteral("_orig");
 
+#if defined(__WINDOWS__)
+const int kWindowsSharingViolationMaxRetries = 5;
+const int kWindowsSharingViolationSleepBeforeNextRetryMillis = 100;
+#endif
+
 // Workaround for missing functionality in TagLib 1.11.x that
 // doesn't support to read text chunks from AIFF files.
 // See also:
@@ -702,6 +711,102 @@ class SafelyWritableFile final {
         if (m_tempFileName.isNull()) {
             return true; // nothing to do
         }
+        QString backupFileName = m_origFileName + kSafelyWritableOrigFileSuffix;
+#ifdef __WINDOWS__
+        // After Mixxx has closed the track file, the indexer or virus scanner
+        // might kick in and fail ReplaceFileW() with a sharing violation when
+        // replacing the original file with the one with the updated metadata.
+        int i = 0;
+        for (; i < kWindowsSharingViolationMaxRetries; ++i) {
+            if (ReplaceFileW(
+                        reinterpret_cast<LPCWSTR>(m_origFileName.utf16()),
+                        reinterpret_cast<LPCWSTR>(m_tempFileName.utf16()),
+                        reinterpret_cast<LPCWSTR>(backupFileName.utf16()),
+                        REPLACEFILE_IGNORE_MERGE_ERRORS | REPLACEFILE_IGNORE_ACL_ERRORS,
+                        nullptr,
+                        nullptr)) {
+                // Success, break retry loop
+                break;
+            } else {
+                DWORD error = GetLastError();
+                switch (error) {
+                case ERROR_UNABLE_TO_MOVE_REPLACEMENT:
+                    // The m_tempFileName file could not be renamed. m_origFileName
+                    // file and m_tempFileName file retain their original file names.
+                    kLogger.critical()
+                            << "Unable to rename replacement file"
+                            << m_tempFileName
+                            << "->"
+                            << m_origFileName;
+                    return false;
+                case ERROR_UNABLE_TO_MOVE_REPLACEMENT_2:
+                    // The m_tempFileName file could not be moved. The m_tempFileName file still exists
+                    // under its original name; however, it has inherited the file streams and
+                    // attributes from the file it is replacing. The m_origFileName file still exists.
+                    kLogger.critical()
+                            << "Unable to move replacement file"
+                            << m_tempFileName
+                            << "->"
+                            << m_origFileName;
+                    return false;
+                case ERROR_UNABLE_TO_REMOVE_REPLACED:
+                    // The replaced file could not be deleted. The replaced and replacement files
+                    // retain their original file names.
+                    kLogger.critical()
+                            << "Unable to remove"
+                            << m_origFileName
+                            << "before replacing by"
+                            << m_tempFileName;
+                    return false;
+                case ERROR_SHARING_VIOLATION:
+                    // The process cannot access the file because it is being used by another process.
+                    kLogger.warning()
+                            << "Unable to replace"
+                            << m_origFileName
+                            << "by"
+                            << m_tempFileName
+                            << "because it is used by another process";
+                    QThread::msleep(kWindowsSharingViolationSleepBeforeNextRetryMillis);
+                    continue; // Retry
+                case ERROR_ACCESS_DENIED:
+                    kLogger.critical()
+                            << "Unable to replace"
+                            << m_origFileName
+                            << "by"
+                            << m_tempFileName
+                            << "Access is denied";
+                    return false;
+                default:
+                    // If any other error is returned, such as ERROR_INVALID_PARAMETER, the replaced
+                    // and replacement files will retain their original file names. In this scenario,
+                    // a backup file does not exist and it is not guaranteed that the replacement file
+                    // will have inherited all of the attributes and streams of the replaced file.
+                    kLogger.critical()
+                            << "Error"
+                            << error
+                            << "during replacing"
+                            << m_origFileName
+                            << "by"
+                            << m_tempFileName;
+                    return false;
+                }
+            }
+        }
+        QFile backupFile(backupFileName);
+        if (backupFile.exists()) {
+            if (!backupFile.remove()) {
+                kLogger.warning()
+                        << backupFile.errorString()
+                        << "- Failed to remove backup file after writing:"
+                        << backupFile.fileName();
+                return false;
+            }
+        }
+        if (i >= kWindowsSharingViolationMaxRetries) {
+            // We have given up after the maximum retries in the loop above.
+            return false;
+        }
+#else
         QFile newFile(m_tempFileName);
         if (!newFile.exists()) {
             kLogger.warning()
@@ -711,7 +816,6 @@ class SafelyWritableFile final {
         }
         QFile oldFile(m_origFileName);
         if (oldFile.exists()) {
-            QString backupFileName = m_origFileName + kSafelyWritableOrigFileSuffix;
             DEBUG_ASSERT(!QFile::exists(backupFileName)); // very unlikely, otherwise renaming fails
             if (!oldFile.rename(backupFileName)) {
                 kLogger.critical()
@@ -753,6 +857,7 @@ class SafelyWritableFile final {
                 return false;
             }
         }
+#endif
         // Prevent any further interaction and file access
         m_origFileName = QString();
         m_tempFileName = QString();

src/sources/soundsourceproxy.cpp

@@ -361,6 +361,13 @@ SoundSourceProxy::exportTrackMetadataBeforeSaving(Track* pTrack, UserSettingsPoi
                 pTrack->updateStreamInfoFromSource(
                         pAudioSource->getStreamInfo());
                 DEBUG_ASSERT(pTrack->hasStreamInfoFromSource());
+                // Make sure that the track file is closed after reading.
+                // Otherwise writing of track metadata into file tags
+                // might fail.
+                proxy.closeAudioSource();
+                // The SoundSource must still be available after closing
+                // the AudioSource.
+                DEBUG_ASSERT(proxy.m_pSoundSource);
             } else {
                 kLogger.warning()
                         << "Failed to update stream info from audio "
@@ -731,7 +738,11 @@ mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(
                 // before exporting metadata. In this case the caller (this class)
                 // is responsible for updating the stream info if needed.
                 if (!m_pTrack) {
-                    return m_pSoundSource;
+                    // Initialize the internal AudioSource without a valid TrackPointer
+                    // This is required to ensure that the corresponding invocation of
+                    // closeAudioSource() works as expected.
+                    m_pAudioSource = m_pSoundSource;
+                    return m_pAudioSource;
                 }
                 m_pAudioSource = mixxx::AudioSourceTrackProxy::create(m_pTrack, m_pSoundSource);
                 DEBUG_ASSERT(m_pAudioSource);
@@ -781,7 +792,7 @@ mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(
         openMode = nextProviderWithOpenModePair.second;
         m_pProvider.reset();
         m_pSoundSource.reset();
-        initSoundSource(std::move(std::move(pNextProvider)));
+        initSoundSource(std::move(pNextProvider));
         // try again
     }
     // All available providers have returned OpenResult::Aborted when