Archive: Avoid calling blocking stopProcess

Make Archive constructor public.
Make the caller responsible for deleting the Archive object.

Don't automatially run the unarchive process when constructing
Archive object. Provide a start() method, to be called after
the caller has connected to Archive signals.

Add Archive::isValid() method.

Remove Archive::unarchive() gui overload, as it's unused.

Make sure we don't leak the Archive object in
AndroidSdkDownloader.

Change-Id: Idf67262554cdfef50aef4a2234b6a5089110f9a2
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Eike Ziller <eike.ziller@qt.io>

src/libs/utils/archive.cpp

@@ -26,16 +26,11 @@
 #include "archive.h"
 
 #include "algorithm.h"
-#include "checkablemessagebox.h"
-#include "environment.h"
 #include "mimeutils.h"
 #include "qtcassert.h"
 #include "qtcprocess.h"
 
-#include <QDir>
-#include <QPushButton>
 #include <QSettings>
-#include <QTimer>
 
 namespace Utils {
 
@@ -163,99 +158,48 @@ bool Archive::supportsFile(const FilePath &filePath, QString *reason)
     return true;
 }
 
-bool Archive::unarchive(const FilePath &src, const FilePath &dest, QWidget *parent)
-{
-    Archive *archive = unarchive(src, dest);
-    QTC_ASSERT(archive, return false);
-
-    CheckableMessageBox box(parent);
-    box.setIcon(QMessageBox::Information);
-    box.setWindowTitle(tr("Unarchiving File"));
-    box.setText(tr("Unzipping \"%1\" to \"%2\".").arg(src.toUserOutput(), dest.toUserOutput()));
-    box.setStandardButtons(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
-    box.button(QDialogButtonBox::Ok)->setEnabled(false);
-    box.setCheckBoxVisible(false);
-    QObject::connect(archive, &Archive::outputReceived, &box, [&box](const QString &output) {
-        box.setDetailedText(box.detailedText() + output);
-    });
-    bool success = false;
-    QObject::connect(archive, &Archive::finished, [&box, &success](bool ret) {
-        box.button(QDialogButtonBox::Ok)->setEnabled(true);
-        box.button(QDialogButtonBox::Cancel)->setEnabled(false);
-        success = ret;
-    });
-    QObject::connect(&box, &QMessageBox::rejected, archive, &Archive::cancel);
-    box.exec();
-    return success;
-}
-
-Archive *Archive::unarchive(const FilePath &src, const FilePath &dest)
+Archive::Archive(const FilePath &src, const FilePath &dest)
 {
     const Utils::optional<Tool> tool = unzipTool(src, dest);
-    QTC_ASSERT(tool, return nullptr);
-
-    auto archive = new Archive;
-
-    const FilePath workingDirectory = dest.absolutePath();
-    workingDirectory.ensureWritableDir();
-
-    archive->m_process = new QtcProcess;
-    archive->m_process->setProcessChannelMode(QProcess::MergedChannels);
-    QObject::connect(
-        archive->m_process,
-        &QtcProcess::readyReadStandardOutput,
-        archive,
-        [archive]() {
-            if (!archive->m_process)
-                return;
-            emit archive->outputReceived(QString::fromUtf8(
-                                             archive->m_process->readAllStandardOutput()));
-        },
-        Qt::QueuedConnection);
-    QObject::connect(
-        archive->m_process,
-        &QtcProcess::finished,
-        archive,
-        [archive] {
-            if (!archive->m_process)
-                return;
-            emit archive->finished(archive->m_process->result() == ProcessResult::FinishedWithSuccess);
-            archive->m_process->deleteLater();
-            archive->m_process = nullptr;
-            archive->deleteLater();
-        },
-        Qt::QueuedConnection);
-    QObject::connect(
-        archive->m_process,
-        &QtcProcess::errorOccurred,
-        archive,
-        [archive](QProcess::ProcessError) {
-            if (!archive->m_process)
-                return;
-            emit archive->outputReceived(tr("Command failed."));
-            emit archive->finished(false);
-            archive->m_process->deleteLater();
-            archive->m_process = nullptr;
-            archive->deleteLater();
-        },
-        Qt::QueuedConnection);
-
-    QTimer::singleShot(0, archive, [archive, tool, workingDirectory] {
-        emit archive->outputReceived(
-            tr("Running %1\nin \"%2\".\n\n", "Running <cmd> in <workingdirectory>")
-                .arg(tool->command.toUserOutput(), workingDirectory.toUserOutput()));
-    });
-
-    archive->m_process->setCommand(tool->command);
-    archive->m_process->setWorkingDirectory(workingDirectory);
-    archive->m_process->start();
-    return archive;
+    if (!tool)
+        return;
+    m_commandLine = tool->command;
+    m_workingDirectory = dest.absoluteFilePath();
 }
 
-void Archive::cancel()
+Archive::~Archive() = default;
+
+bool Archive::isValid() const
 {
-    if (m_process)
-        m_process->stopProcess();
+    return !m_commandLine.isEmpty();
+}
+
+void Archive::unarchive()
+{
+    QTC_ASSERT(isValid(), return);
+    QTC_ASSERT(!m_process, return);
+
+    m_workingDirectory.ensureWritableDir();
+
+    m_process.reset(new QtcProcess);
+    m_process->setProcessChannelMode(QProcess::MergedChannels);
+    QObject::connect(m_process.get(), &QtcProcess::readyReadStandardOutput, this, [this] {
+        emit outputReceived(QString::fromUtf8(m_process->readAllStandardOutput()));
+    });
+    QObject::connect(m_process.get(), &QtcProcess::finished, this, [this] {
+        emit finished(m_process->result() == ProcessResult::FinishedWithSuccess);
+    });
+    QObject::connect(m_process.get(), &QtcProcess::errorOccurred, this, [this] {
+        emit outputReceived(tr("Command failed."));
+        emit finished(false);
+    });
+
+    emit outputReceived(tr("Running %1\nin \"%2\".\n\n", "Running <cmd> in <workingdirectory>")
+                 .arg(m_commandLine.toUserOutput(), m_workingDirectory.toUserOutput()));
+
+    m_process->setCommand(m_commandLine);
+    m_process->setWorkingDirectory(m_workingDirectory);
+    m_process->start();
 }
 
 } // namespace Utils

src/libs/utils/archive.h

@@ -27,6 +27,7 @@
 
 #include "utils_global.h"
 
+#include "commandline.h"
 #include "fileutils.h"
 
 #include <QObject>
@@ -39,20 +40,22 @@ class QTCREATOR_UTILS_EXPORT Archive : public QObject
 {
     Q_OBJECT
 public:
-    static bool supportsFile(const FilePath &filePath, QString *reason = nullptr);
-    static bool unarchive(const FilePath &src, const FilePath &dest, QWidget *parent);
-    static Archive *unarchive(const FilePath &src, const FilePath &dest);
+    Archive(const FilePath &src, const FilePath &dest);
+    ~Archive();
 
-    void cancel();
+    bool isValid() const;
+    void unarchive();
+
+    static bool supportsFile(const FilePath &filePath, QString *reason = nullptr);
 
 signals:
     void outputReceived(const QString &output);
     void finished(bool success);
 
 private:
-    Archive() = default;
-
-    QtcProcess *m_process = nullptr;
+    Utils::CommandLine m_commandLine;
+    Utils::FilePath m_workingDirectory;
+    std::unique_ptr<QtcProcess> m_process;
 };
 
 } // namespace Utils

src/plugins/android/androidsdkdownloader.cpp

@@ -54,6 +54,8 @@ AndroidSdkDownloader::AndroidSdkDownloader()
     connect(&m_manager, &QNetworkAccessManager::finished, this, &AndroidSdkDownloader::downloadFinished);
 }
 
+AndroidSdkDownloader::~AndroidSdkDownloader() = default;
+
 #if QT_CONFIG(ssl)
 void AndroidSdkDownloader::sslErrors(const QList<QSslError> &sslErrors)
 {
@@ -91,9 +93,12 @@ void AndroidSdkDownloader::downloadAndExtractSdk()
     connect(m_progressDialog, &QProgressDialog::canceled, this, &AndroidSdkDownloader::cancel);
 
     connect(this, &AndroidSdkDownloader::sdkPackageWriteFinished, this, [this]() {
+        if (!Archive::supportsFile(m_sdkFilename))
+            return;
         const FilePath extractDir = m_sdkFilename.parentDir();
-        if (Archive *archive = Archive::unarchive(m_sdkFilename, extractDir)) {
-            connect(archive, &Archive::finished, [this, extractDir](bool success) {
+        m_archive.reset(new Archive(m_sdkFilename, extractDir));
+        if (m_archive->isValid()) {
+            connect(m_archive.get(), &Archive::finished, this, [this, extractDir](bool success) {
                 if (success) {
                     // Save the extraction path temporarily which can be used by sdkmanager
                     // to install essential packages at firt time setup.
@@ -101,7 +106,9 @@ void AndroidSdkDownloader::downloadAndExtractSdk()
                                 extractDir.pathAppended(Constants::cmdlineToolsName));
                     emit sdkExtracted();
                 }
+                m_archive.release()->deleteLater();
             });
+            m_archive->unarchive();
         }
     });
 }

src/plugins/android/androidsdkdownloader.h

@@ -32,7 +32,10 @@
 #include <QObject>
 #include <QProgressDialog>
 
-namespace Utils { class FilePath; }
+namespace Utils {
+class Archive;
+class FilePath;
+}
 
 namespace Android {
 namespace Internal {
@@ -43,6 +46,7 @@ class AndroidSdkDownloader : public QObject
 
 public:
     AndroidSdkDownloader();
+    ~AndroidSdkDownloader();
     void downloadAndExtractSdk();
     static QString dialogTitle();
 
@@ -72,6 +76,7 @@ private:
     Utils::FilePath m_sdkFilename;
     QProgressDialog *m_progressDialog = nullptr;
     AndroidConfig &m_androidConfig;
+    std::unique_ptr<Utils::Archive> m_archive;
 };
 
 } // Internal

src/plugins/coreplugin/plugininstallwizard.cpp

@@ -207,60 +207,66 @@ public:
         m_cancelButton->setVisible(true);
         m_output->clear();
 
-        m_archive = Archive::unarchive(m_data->sourcePath, m_tempDir->path());
-
-        if (!m_archive) {
+        m_archive.reset(new Archive(m_data->sourcePath, m_tempDir->path()));
+        if (!m_archive->isValid()) {
             m_label->setType(InfoLabel::Error);
             m_label->setText(PluginInstallWizard::tr("The file is not an archive."));
-
             return;
         }
-        QObject::connect(m_archive, &Archive::outputReceived, this, [this](const QString &output) {
+        QObject::connect(m_archive.get(), &Archive::outputReceived, this,
+                         [this](const QString &output) {
             m_output->append(output);
         });
-        QObject::connect(m_archive, &Archive::finished, this, [this](bool success) {
-            m_archive = nullptr; // we don't own it
-            m_cancelButton->disconnect();
-            if (!success) { // unarchiving failed
+        QObject::connect(m_archive.get(), &Archive::finished, this, [this](bool success) {
+            m_archive.release()->deleteLater();
+            handleFinished(success);
+        });
+        QObject::connect(m_cancelButton, &QPushButton::clicked, this, [this] {
+            m_canceled = true;
+            m_archive.reset();
+            handleFinished(false);
+        });
+        m_archive->unarchive();
+    }
+
+    void handleFinished(bool success)
+    {
+        m_cancelButton->disconnect();
+        if (!success) { // unarchiving failed
+            m_cancelButton->setVisible(false);
+            if (m_canceled) {
+                m_label->setType(InfoLabel::Information);
+                m_label->setText(PluginInstallWizard::tr("Canceled."));
+            } else {
+                m_label->setType(InfoLabel::Error);
+                m_label->setText(
+                    PluginInstallWizard::tr("There was an error while unarchiving."));
+            }
+        } else { // unarchiving was successful, run a check
+            m_archiveCheck = Utils::runAsync(
+                [this](QFutureInterface<ArchiveIssue> &fi) { return checkContents(fi); });
+            Utils::onFinished(m_archiveCheck, this, [this](const QFuture<ArchiveIssue> &f) {
                 m_cancelButton->setVisible(false);
-                if (m_canceled) {
+                m_cancelButton->disconnect();
+                const bool ok = f.resultCount() == 0 && !f.isCanceled();
+                if (f.isCanceled()) {
                     m_label->setType(InfoLabel::Information);
                     m_label->setText(PluginInstallWizard::tr("Canceled."));
+                } else if (ok) {
+                    m_label->setType(InfoLabel::Ok);
+                    m_label->setText(PluginInstallWizard::tr("Archive is OK."));
                 } else {
-                    m_label->setType(InfoLabel::Error);
-                    m_label->setText(
-                        PluginInstallWizard::tr("There was an error while unarchiving."));
+                    const ArchiveIssue issue = f.result();
+                    m_label->setType(issue.type);
+                    m_label->setText(issue.message);
                 }
-            } else { // unarchiving was successful, run a check
-                m_archiveCheck = Utils::runAsync(
-                    [this](QFutureInterface<ArchiveIssue> &fi) { return checkContents(fi); });
-                Utils::onFinished(m_archiveCheck, this, [this](const QFuture<ArchiveIssue> &f) {
-                    m_cancelButton->setVisible(false);
-                    m_cancelButton->disconnect();
-                    const bool ok = f.resultCount() == 0 && !f.isCanceled();
-                    if (f.isCanceled()) {
-                        m_label->setType(InfoLabel::Information);
-                        m_label->setText(PluginInstallWizard::tr("Canceled."));
-                    } else if (ok) {
-                        m_label->setType(InfoLabel::Ok);
-                        m_label->setText(PluginInstallWizard::tr("Archive is OK."));
-                    } else {
-                        const ArchiveIssue issue = f.result();
-                        m_label->setType(issue.type);
-                        m_label->setText(issue.message);
-                    }
-                    m_isComplete = ok;
-                    emit completeChanged();
-                });
-                QObject::connect(m_cancelButton, &QPushButton::clicked, this, [this] {
-                    m_archiveCheck.cancel();
-                });
-            }
-        });
-        QObject::connect(m_cancelButton, &QPushButton::clicked, m_archive, [this] {
-            m_canceled = true;
-            m_archive->cancel();
-        });
+                m_isComplete = ok;
+                emit completeChanged();
+            });
+            QObject::connect(m_cancelButton, &QPushButton::clicked, this, [this] {
+                m_archiveCheck.cancel();
+            });
+        }
     }
 
     // Async. Result is set if any issue was found.
@@ -311,11 +317,7 @@ public:
     {
         // back button pressed
         m_cancelButton->disconnect();
-        if (m_archive) {
-            m_archive->disconnect();
-            m_archive->cancel();
-            m_archive = nullptr; // we don't own it
-        }
+        m_archive.reset();
         if (m_archiveCheck.isRunning()) {
             m_archiveCheck.cancel();
             m_archiveCheck.waitForFinished();
@@ -326,7 +328,7 @@ public:
     bool isComplete() const final { return m_isComplete; }
 
     std::unique_ptr<TemporaryDirectory> m_tempDir;
-    Archive *m_archive = nullptr;
+    std::unique_ptr<Archive> m_archive;
     QFuture<ArchiveIssue> m_archiveCheck;
     InfoLabel *m_label = nullptr;
     QPushButton *m_cancelButton = nullptr;

src/plugins/studiowelcome/examplecheckout.cpp

@@ -378,9 +378,9 @@ void FileExtractor::extract()
     // Create a new directory to generate a proper creation date
     targetDir.mkdir(targetFolder);
 
-    Utils::Archive *archive = Utils::Archive::unarchive(m_sourceFile, m_targetPath);
+    Utils::Archive *archive = new Utils::Archive(m_sourceFile, m_targetPath);
+    QTC_ASSERT(archive->isValid(), delete archive; return);
     archive->setParent(this);
-    QTC_ASSERT(archive, return );
 
     m_timer.start();
     qint64 bytesBefore = QStorageInfo(m_targetPath.toFileInfo().dir()).bytesAvailable();
@@ -429,6 +429,7 @@ void FileExtractor::extract()
 
         emit targetFolderExistsChanged();
         emit finishedChanged();
-        QTC_ASSERT(ret, ;);
+        QTC_CHECK(ret);
     });
+    archive->unarchive();
 }