From f6b37328c75649b106057a962d09dcb7fb2d3aa2 Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Thu, 21 Jul 2022 17:48:49 +0200 Subject: [PATCH] AutotoolsBuildSystem: Avoid using sender() According to the old explanation inside makefileParsingFinished() the solution of using sender() can't work reliably in this case. In scenarion where the old, deleted m_makefileParserThread had the same address as newly allocated one we might not detect correctly posted finished() signal of the deleted object. According to QObject::sender() documentation: "The pointer returned by this function becomes invalid if the sender is destroyed", so we can't rely on its value. Instead, add a new done() signal to the MakefileParserThread, which is being emitted from MakefileParserThread thread (i.e. from the thread where MakefileParserThread object lives in). In this way the finished() signal is still posted to the caller's thread, but this time if the MakefileParserThread object is already deleted, the finished() signal will be gone together with the MakefileParserThread object itself, so we also won't receive done() signal anymore. Simplify the internals by using std::unique_ptr. Change-Id: I43be209ecb71539ddefd72e50e9d60bfb43c49cb Reviewed-by: Reviewed-by: Eike Ziller --- .../autotoolsbuildsystem.cpp | 45 +++++-------------- .../autotoolsbuildsystem.h | 4 +- .../makefileparserthread.cpp | 4 +- .../makefileparserthread.h | 7 +++ 4 files changed, 23 insertions(+), 37 deletions(-) diff --git a/src/plugins/autotoolsprojectmanager/autotoolsbuildsystem.cpp b/src/plugins/autotoolsprojectmanager/autotoolsbuildsystem.cpp index f7e27b8b60d..c4ab94f5858 100644 --- a/src/plugins/autotoolsprojectmanager/autotoolsbuildsystem.cpp +++ b/src/plugins/autotoolsprojectmanager/autotoolsbuildsystem.cpp @@ -55,53 +55,31 @@ AutotoolsBuildSystem::~AutotoolsBuildSystem() { delete m_cppCodeModelUpdater; - if (m_makefileParserThread) { + if (m_makefileParserThread) m_makefileParserThread->wait(); - delete m_makefileParserThread; - m_makefileParserThread = nullptr; - } } void AutotoolsBuildSystem::triggerParsing() { - if (m_makefileParserThread) { - // The thread is still busy parsing a previous configuration. - // Wait until the thread has been finished and delete it. - // TODO: Discuss whether blocking is acceptable. - disconnect(m_makefileParserThread, - &QThread::finished, - this, - &AutotoolsBuildSystem::makefileParsingFinished); + // The thread is still busy parsing a previous configuration. + // Wait until the thread has been finished and delete it. + // TODO: Discuss whether blocking is acceptable. + if (m_makefileParserThread) m_makefileParserThread->wait(); - delete m_makefileParserThread; - m_makefileParserThread = nullptr; - } // Parse the makefile asynchronously in a thread - m_makefileParserThread = new MakefileParserThread(this); + m_makefileParserThread.reset(new MakefileParserThread(this)); - connect(m_makefileParserThread, - &MakefileParserThread::finished, - this, - &AutotoolsBuildSystem::makefileParsingFinished); + connect(m_makefileParserThread.get(), &MakefileParserThread::done, + this, &AutotoolsBuildSystem::makefileParsingFinished); m_makefileParserThread->start(); } void AutotoolsBuildSystem::makefileParsingFinished() { - // The finished() signal is from a previous makefile-parser-thread - // and can be skipped. This can happen, if the thread has emitted the - // finished() signal during the execution of AutotoolsBuildSystem::loadProjectTree(). - // In this case the signal is in the message queue already and deleting - // the thread of course does not remove the signal again. - if (sender() != m_makefileParserThread) - return; - + // The parsing has been cancelled by the user. Don't show any project data at all. if (m_makefileParserThread->isCanceled()) { - // The parsing has been cancelled by the user. Don't show any - // project data at all. - m_makefileParserThread->deleteLater(); - m_makefileParserThread = nullptr; + m_makefileParserThread.release()->deleteLater(); return; } @@ -151,8 +129,7 @@ void AutotoolsBuildSystem::makefileParsingFinished() updateCppCodeModel(); - m_makefileParserThread->deleteLater(); - m_makefileParserThread = nullptr; + m_makefileParserThread.release()->deleteLater(); emitBuildSystemUpdated(); } diff --git a/src/plugins/autotoolsprojectmanager/autotoolsbuildsystem.h b/src/plugins/autotoolsprojectmanager/autotoolsbuildsystem.h index 09cd656b772..078e01e7ecd 100644 --- a/src/plugins/autotoolsprojectmanager/autotoolsbuildsystem.h +++ b/src/plugins/autotoolsprojectmanager/autotoolsbuildsystem.h @@ -29,6 +29,8 @@ #include +#include + namespace CppEditor { class CppProjectUpdater; } namespace AutotoolsProjectManager { @@ -63,7 +65,7 @@ private: QStringList m_files; /// Responsible for parsing the makefiles asynchronously in a thread - MakefileParserThread *m_makefileParserThread = nullptr; + std::unique_ptr m_makefileParserThread; CppEditor::CppProjectUpdater *m_cppCodeModelUpdater = nullptr; }; diff --git a/src/plugins/autotoolsprojectmanager/makefileparserthread.cpp b/src/plugins/autotoolsprojectmanager/makefileparserthread.cpp index 1a88febbf84..183e396aa1a 100644 --- a/src/plugins/autotoolsprojectmanager/makefileparserthread.cpp +++ b/src/plugins/autotoolsprojectmanager/makefileparserthread.cpp @@ -35,8 +35,8 @@ MakefileParserThread::MakefileParserThread(ProjectExplorer::BuildSystem *bs) : m_parser(bs->projectFilePath().toString()), m_guard(bs->guardParsingRun()) { - connect(&m_parser, &MakefileParser::status, - this, &MakefileParserThread::status); + connect(&m_parser, &MakefileParser::status, this, &MakefileParserThread::status); + connect(this, &QThread::finished, this, &MakefileParserThread::done, Qt::QueuedConnection); } QStringList MakefileParserThread::sources() const diff --git a/src/plugins/autotoolsprojectmanager/makefileparserthread.h b/src/plugins/autotoolsprojectmanager/makefileparserthread.h index 75547af1dc5..cf2b448eeb9 100644 --- a/src/plugins/autotoolsprojectmanager/makefileparserthread.h +++ b/src/plugins/autotoolsprojectmanager/makefileparserthread.h @@ -131,6 +131,13 @@ signals: */ void status(const QString &status); + /** + * Similar to finished, but emitted from MakefileParserThread thread, i.e. from the + * thread where the MakefileParserThread lives in, not the tread that it creates. + * This helps to avoid race condition when connecting to finished() signal. + */ + void done(); + private: MakefileParser m_parser; ///< Is not accessible outside the thread