Fix extracted files list formation in ExtractArchive operation

In ExtractArchive, the ConnectionType used to connect signal
'currentFileChanged' to slot 'fileFinished' was changed to
Qt::AutoConnection in c8de51cadbc5855ca1e77d038d7f09bf60d059ee,
rather than using Qt::DirectConnection explicitly.

Now the connection type defaults to queued as the sender and
receiver live on a different thread. This seems to cause an issue
where the slot function is not invoked for every emitted
'currentFileChanged' signal before returning from
ExtractArchiveOperation::performOperation(). This can happen
randomly on some installer runs, leaving entries out from
ExtractArchive's "files" list for some components.

The list formation works fine with Qt::DirectConnection, however
reverting to this behavior might be a bad practice as it leaves
'fileFinished' vulnerable to accidental calls from another thread
if we change the behavior in the future. Rather move the extracted
files list formation to ExtractArchiveOperation::Callback like we
already do for backup files.

Task-number: QTIFW-1239
Change-Id: I7dbe73abefe00814e8652432d8abdbcaa83e0bac
Reviewed-by: Iikka Eklund <iikka.eklund@qt.io>
Reviewed-by: Katja Marttila <katja.marttila@qt.io>

3 files changed, 11 insertions, 21 deletions

diff --git a/src/libs/installer/extractarchiveoperation.cpp b/src/libs/installer/extractarchiveoperation.cpp
index 12067a7c6..1c3e1b78a 100644
--- a/src/libs/installer/extractarchiveoperation.cpp
+++ b/src/libs/installer/extractarchiveoperation.cpp
@@ -59,7 +59,6 @@ bool ExtractArchiveOperation::performOperation()
     Receiver receiver;
     Callback callback;
 
-    connect(&callback, &Callback::currentFileChanged, this, &ExtractArchiveOperation::fileFinished);
     connect(&callback, &Callback::progressChanged, this, &ExtractArchiveOperation::progressChanged);
 
     if (PackageManagerCore *core = packageManager()) {
@@ -70,8 +69,6 @@ bool ExtractArchiveOperation::performOperation()
     connect(runnable, &Runnable::finished, &receiver, &Receiver::runnableFinished,
         Qt::QueuedConnection);
 
-    m_files.clear();
-
     QFileInfo fileInfo(archivePath);
     emit outputTextChanged(tr("Extracting \"%1\"").arg(fileInfo.fileName()));
 
@@ -96,6 +93,8 @@ bool ExtractArchiveOperation::performOperation()
     //   -<component_name> (dir)
     //    -<filename>.txt (file)
 
+    QStringList files = callback.extractedFiles();
+
     QString fileDirectory = targetDir + QLatin1String("/installerResources/") +
             archivePath.section(QLatin1Char('/'), 1, 1, QString::SectionSkipEmpty) + QLatin1Char('/');
     QString archiveFileName = archivePath.section(QLatin1Char('/'), 2, 2, QString::SectionSkipEmpty);
@@ -114,10 +113,10 @@ bool ExtractArchiveOperation::performOperation()
     if (file.open(QIODevice::WriteOnly)) {
         setDefaultFilePermissions(file.fileName(), DefaultFilePermissions::NonExecutable);
         QDataStream out (&file);
-        for (int i = 0; i < m_files.count(); ++i) {
-            m_files[i] = replacePath(m_files.at(i), targetDir, QLatin1String(scRelocatable));
+        for (int i = 0; i < files.count(); ++i) {
+            files[i] = replacePath(files.at(i), targetDir, QLatin1String(scRelocatable));
         }
-        out << m_files;
+        out << files;
         setValue(QLatin1String("files"), file.fileName());
         file.close();
     } else {
@@ -199,13 +198,4 @@ bool ExtractArchiveOperation::testOperation()
     return true;
 }
 
-/*!
-    This slot is direct connected to the caller so please don't call it from another thread in the
-    same time.
-*/
-void ExtractArchiveOperation::fileFinished(const QString &filename)
-{
-    m_files.prepend(filename);
-}
-
 } // namespace QInstaller
diff --git a/src/libs/installer/extractarchiveoperation.h b/src/libs/installer/extractarchiveoperation.h
index 0f584dc0a..e706159ed 100644
--- a/src/libs/installer/extractarchiveoperation.h
+++ b/src/libs/installer/extractarchiveoperation.h
@@ -55,11 +55,7 @@ Q_SIGNALS:
 private:
     void startUndoProcess(const QStringList &files);
 
-private Q_SLOTS:
-    void fileFinished(const QString &progress);
-
 private:
-    QStringList m_files;
     class Callback;
     class Runnable;
     class Receiver;
diff --git a/src/libs/installer/extractarchiveoperation_p.h b/src/libs/installer/extractarchiveoperation_p.h
index f333da366..9cc07246f 100644
--- a/src/libs/installer/extractarchiveoperation_p.h
+++ b/src/libs/installer/extractarchiveoperation_p.h
@@ -97,6 +97,10 @@ public:
         return m_backupFiles;
     }
 
+    QStringList extractedFiles() const {
+        return m_extractedFiles;
+    }
+
 public slots:
     void statusChanged(QInstaller::PackageManagerCore::Status status)
     {
@@ -113,13 +117,12 @@ public slots:
     }
 
 signals:
-    void currentFileChanged(const QString &filename);
     void progressChanged(double progress);
 
 private:
     void setCurrentFile(const QString &filename) Q_DECL_OVERRIDE
     {
-        emit currentFileChanged(QDir::toNativeSeparators(filename));
+        m_extractedFiles.prepend(QDir::toNativeSeparators(filename));
     }
 
     static QString generateBackupName(const QString &fn)
@@ -157,6 +160,7 @@ private:
 private:
     HRESULT m_state = S_OK;
     BackupFiles m_backupFiles;
+    QStringList m_extractedFiles;
 };
 
 class ExtractArchiveOperation::Runnable : public QObject, public QRunnable