From ed94e0c0662aa01f97af327b8e269613806e853d Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Thu, 24 Mar 2022 10:29:02 +0100 Subject: [PATCH] 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: Reviewed-by: Eike Ziller --- src/libs/utils/archive.cpp | 130 +++++------------- src/libs/utils/archive.h | 17 ++- src/plugins/android/androidsdkdownloader.cpp | 11 +- src/plugins/android/androidsdkdownloader.h | 7 +- .../coreplugin/plugininstallwizard.cpp | 100 +++++++------- src/plugins/studiowelcome/examplecheckout.cpp | 7 +- 6 files changed, 117 insertions(+), 155 deletions(-) diff --git a/src/libs/utils/archive.cpp b/src/libs/utils/archive.cpp index 3883fa3f192..fcef2c22b04 100644 --- a/src/libs/utils/archive.cpp +++ b/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 -#include #include -#include 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 = 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 in ") - .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 in ") + .arg(m_commandLine.toUserOutput(), m_workingDirectory.toUserOutput())); + + m_process->setCommand(m_commandLine); + m_process->setWorkingDirectory(m_workingDirectory); + m_process->start(); } } // namespace Utils diff --git a/src/libs/utils/archive.h b/src/libs/utils/archive.h index 217191073a8..a392cf04e3e 100644 --- a/src/libs/utils/archive.h +++ b/src/libs/utils/archive.h @@ -27,6 +27,7 @@ #include "utils_global.h" +#include "commandline.h" #include "fileutils.h" #include @@ -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 m_process; }; } // namespace Utils diff --git a/src/plugins/android/androidsdkdownloader.cpp b/src/plugins/android/androidsdkdownloader.cpp index 91319cb8a96..6e0b66c555d 100644 --- a/src/plugins/android/androidsdkdownloader.cpp +++ b/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 &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(); } }); } diff --git a/src/plugins/android/androidsdkdownloader.h b/src/plugins/android/androidsdkdownloader.h index 1cfe5b950dd..c18f4c51000 100644 --- a/src/plugins/android/androidsdkdownloader.h +++ b/src/plugins/android/androidsdkdownloader.h @@ -32,7 +32,10 @@ #include #include -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 m_archive; }; } // Internal diff --git a/src/plugins/coreplugin/plugininstallwizard.cpp b/src/plugins/coreplugin/plugininstallwizard.cpp index af091b29ec2..c6cd1bbba43 100644 --- a/src/plugins/coreplugin/plugininstallwizard.cpp +++ b/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 &fi) { return checkContents(fi); }); + Utils::onFinished(m_archiveCheck, this, [this](const QFuture &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 &fi) { return checkContents(fi); }); - Utils::onFinished(m_archiveCheck, this, [this](const QFuture &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 m_tempDir; - Archive *m_archive = nullptr; + std::unique_ptr m_archive; QFuture m_archiveCheck; InfoLabel *m_label = nullptr; QPushButton *m_cancelButton = nullptr; diff --git a/src/plugins/studiowelcome/examplecheckout.cpp b/src/plugins/studiowelcome/examplecheckout.cpp index 267295b1713..d0f1d1ce996 100644 --- a/src/plugins/studiowelcome/examplecheckout.cpp +++ b/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(); }