From 8fad758f6038875f5219c79fd35e937956e4c890 Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Mon, 1 Mar 2021 15:01:35 +0100 Subject: [PATCH] ClassView: Fix a possible crash on session switch Don't call SessionManager methods in non-main thread. It's not safe to call SessionManger::projects() or any method of Project class in a non-main thread, as in meantime the Project object may get deleted or the Project object may change in main thread in a not thread-safe way. Instead, prepare the data needed for the parser's thread before, when scheduling a call in a main thread, and pass this data in a safe way. This fixes possible crash in class view, e.g. on session switch. Task-number: QTCREATORBUG-25317 Fixes: QTCREATORBUG-25312 Change-Id: I114aae788aec649d1de3b3d3afdd049ed1e9b2c6 Reviewed-by: Eike Ziller --- src/plugins/classview/classviewmanager.cpp | 49 ++--- src/plugins/classview/classviewmanager.h | 1 - src/plugins/classview/classviewparser.cpp | 203 +++++++++------------ src/plugins/classview/classviewparser.h | 21 ++- 4 files changed, 124 insertions(+), 150 deletions(-) diff --git a/src/plugins/classview/classviewmanager.cpp b/src/plugins/classview/classviewmanager.cpp index 7724fbc4938..b067ad1cc54 100644 --- a/src/plugins/classview/classviewmanager.cpp +++ b/src/plugins/classview/classviewmanager.cpp @@ -44,6 +44,7 @@ #include using namespace Core; +using namespace ProjectExplorer; using namespace Utils; namespace ClassView { @@ -94,7 +95,7 @@ public: ParserTreeItem::ConstPtr m_root; QTimer m_timer; - QHash m_awaitingDocuments; + QSet m_awaitingDocuments; //! Internal manager state. \sa Manager::state bool state = false; @@ -116,7 +117,15 @@ void ManagerPrivate::cancelScheduledUpdate() void ManagerPrivate::resetParser() { cancelScheduledUpdate(); - QMetaObject::invokeMethod(m_parser, &Parser::resetDataToCurrentState, Qt::QueuedConnection); + + QHash> projectData; + for (const Project *project : SessionManager::projects()) { + projectData.insert(project->projectFilePath(), + qMakePair(project->displayName(), project->files(Project::SourceFiles))); + } + QMetaObject::invokeMethod(m_parser, [this, projectData]() { + m_parser->resetData(projectData); + }, Qt::QueuedConnection); } /*! @@ -218,15 +227,26 @@ bool Manager::hasChildren(QStandardItem *item) const void Manager::initialize() { - using ProjectExplorer::SessionManager; d->m_timer.setSingleShot(true); // connections to enable/disable navi widget factory SessionManager *sessionManager = SessionManager::instance(); connect(sessionManager, &SessionManager::projectAdded, - this, &Manager::onProjectListChanged); + this, [this](Project *project) { + const FilePath projectPath = project->projectFilePath(); + const QString projectName = project->displayName(); + const FilePaths projectFiles = project->files(Project::SourceFiles); + QMetaObject::invokeMethod(d->m_parser, [this, projectPath, projectName, projectFiles]() { + d->m_parser->addProject(projectPath, projectName, projectFiles); + }, Qt::QueuedConnection); + }); connect(sessionManager, &SessionManager::projectRemoved, - this, &Manager::onProjectListChanged); + this, [this](Project *project) { + const FilePath projectPath = project->projectFilePath(); + QMetaObject::invokeMethod(d->m_parser, [this, projectPath]() { + d->m_parser->removeProject(projectPath); + }, Qt::QueuedConnection); + }); // connect to the progress manager for signals about Parsing tasks connect(ProgressManager::instance(), &ProgressManager::taskStarted, @@ -283,12 +303,12 @@ void Manager::initialize() if (doc.data() == nullptr) return; - d->m_awaitingDocuments.insert(doc->fileName(), doc); + d->m_awaitingDocuments.insert(FilePath::fromString(doc->fileName())); d->m_timer.start(400); // Accumulate multiple requests into one, restarts the timer }); connect(&d->m_timer, &QTimer::timeout, this, [this]() { - const QList docsToBeUpdated = d->m_awaitingDocuments.values(); + const QSet docsToBeUpdated = d->m_awaitingDocuments; d->cancelScheduledUpdate(); if (!state() || d->disableCodeParser) // enabling any of them will trigger the total update return; @@ -346,20 +366,7 @@ void Manager::onWidgetVisibilityIsChanged(bool visibility) if (!visibility) return; setState(true); - onProjectListChanged(); -} - -/*! - Reacts to the project list being changed by updating the navigation pane - visibility if necessary. -*/ - -void Manager::onProjectListChanged() -{ - // do nothing if Manager is disabled - if (!state()) - return; - + // TODO: this one may change into getter (when a new class view widget is being shown) QMetaObject::invokeMethod(d->m_parser, &Parser::requestCurrentState, Qt::QueuedConnection); } diff --git a/src/plugins/classview/classviewmanager.h b/src/plugins/classview/classviewmanager.h index 30e3fc9a487..538eb9a72f2 100644 --- a/src/plugins/classview/classviewmanager.h +++ b/src/plugins/classview/classviewmanager.h @@ -60,7 +60,6 @@ signals: void treeDataUpdate(QSharedPointer result); private: - void onProjectListChanged(); void initialize(); inline bool state() const; diff --git a/src/plugins/classview/classviewparser.cpp b/src/plugins/classview/classviewparser.cpp index cfeac097452..ba331096c2a 100644 --- a/src/plugins/classview/classviewparser.cpp +++ b/src/plugins/classview/classviewparser.cpp @@ -32,18 +32,13 @@ // other #include -#include -#include -#include - #include #include -#include +#include #include #include #include -#include enum { debug = false }; @@ -67,17 +62,11 @@ namespace Internal { \brief The Parser class parses C++ information. Multithreading is supported. */ -/*! - \fn void Parser::treeDataUpdate(QSharedPointer result) - - Emits a signal about a tree data update. -*/ - class ParserPrivate { public: //! Get document from documentList - CPlusPlus::Document::Ptr document(const QString &fileName) const; + CPlusPlus::Document::Ptr document(const Utils::FilePath &fileName) const; struct DocumentCache { unsigned treeRevision = 0; @@ -87,23 +76,20 @@ public: struct ProjectCache { unsigned treeRevision = 0; ParserTreeItem::ConstPtr tree; - QStringList fileList; + QString projectName; + QSet fileNames; }; // Project file path to its cached data - QHash m_documentCache; + QHash m_documentCache; // Project file path to its cached data - QHash m_projectCache; - - // other - //! List for files which has to be parsed - QSet fileList; + QHash m_projectCache; //! Flat mode bool flatMode = false; }; -CPlusPlus::Document::Ptr ParserPrivate::document(const QString &fileName) const +CPlusPlus::Document::Ptr ParserPrivate::document(const FilePath &fileName) const { return m_documentCache.value(fileName).document; } @@ -161,16 +147,14 @@ ParserTreeItem::ConstPtr Parser::parse() QHash projectTrees; - // TODO: move a call to SessionManager::projects() out of this thread - for (const Project *prj : SessionManager::projects()) { - const QString prjName(prj->displayName()); - const QString prjType = prj->projectFilePath().toString(); - const SymbolInformation inf(prjName, prjType); - - ParserTreeItem::ConstPtr item = addFlatTree(prj); + for (auto it = d->m_projectCache.cbegin(); it != d->m_projectCache.cend(); ++it) { + const ParserPrivate::ProjectCache &projectCache = it.value(); + const FilePath projectPath = it.key(); + const SymbolInformation projectInfo = { projectCache.projectName, projectPath.toString() }; + ParserTreeItem::ConstPtr item = getCachedOrParseProjectTree(projectPath, projectCache.fileNames); if (item.isNull()) continue; - projectTrees.insert(inf, item); + projectTrees.insert(projectInfo, item); } ParserTreeItem::ConstPtr rootItem(new ParserTreeItem(projectTrees)); @@ -189,16 +173,16 @@ ParserTreeItem::ConstPtr Parser::parse() project. */ -ParserTreeItem::ConstPtr Parser::getParseProjectTree(const QStringList &fileList, - const QString &projectId) +ParserTreeItem::ConstPtr Parser::getParseProjectTree(const FilePath &projectPath, + const QSet &filesInProject) { //! \todo Way to optimize - for documentUpdate - use old cached project and subtract //! changed files only (old edition), and add curent editions QList docTrees; unsigned revision = 0; - for (const QString &file : fileList) { - const CPlusPlus::Document::Ptr &doc = d->document(file); + for (const FilePath &fileInProject : filesInProject) { + const CPlusPlus::Document::Ptr &doc = d->document(fileInProject); if (doc.isNull()) continue; @@ -210,11 +194,11 @@ ParserTreeItem::ConstPtr Parser::getParseProjectTree(const QStringList &fileList docTrees.append(docTree); } - ParserTreeItem::ConstPtr item = ParserTreeItem::mergeTrees(Utils::FilePath::fromString(projectId), docTrees); + ParserTreeItem::ConstPtr item = ParserTreeItem::mergeTrees(projectPath, docTrees); // update the cache - if (!projectId.isEmpty()) { - ParserPrivate::ProjectCache &projectCache = d->m_projectCache[projectId]; + if (!projectPath.isEmpty()) { + ParserPrivate::ProjectCache &projectCache = d->m_projectCache[projectPath]; projectCache.tree = item; projectCache.treeRevision = revision; } @@ -227,15 +211,15 @@ ParserTreeItem::ConstPtr Parser::getParseProjectTree(const QStringList &fileList Updates the internal cached tree for this project. */ -ParserTreeItem::ConstPtr Parser::getCachedOrParseProjectTree(const QStringList &fileList, - const QString &projectId) +ParserTreeItem::ConstPtr Parser::getCachedOrParseProjectTree(const FilePath &projectPath, + const QSet &filesInProject) { - const auto it = d->m_projectCache.constFind(projectId); + const auto it = d->m_projectCache.constFind(projectPath); if (it != d->m_projectCache.constEnd() && !it.value().tree.isNull()) { // calculate project's revision unsigned revision = 0; - for (const QString &file : fileList) { - const CPlusPlus::Document::Ptr &doc = d->document(file); + for (const FilePath &fileInProject : filesInProject) { + const CPlusPlus::Document::Ptr &doc = d->document(fileInProject); if (doc.isNull()) continue; revision += doc->revision(); @@ -246,7 +230,7 @@ ParserTreeItem::ConstPtr Parser::getCachedOrParseProjectTree(const QStringList & return it.value().tree; } - return getParseProjectTree(fileList, projectId); + return getParseProjectTree(projectPath, filesInProject); } /*! @@ -261,9 +245,7 @@ ParserTreeItem::ConstPtr Parser::getParseDocumentTree(const CPlusPlus::Document: if (doc.isNull()) return ParserTreeItem::ConstPtr(); - const QString &fileName = doc->fileName(); - if (!d->fileList.contains(fileName)) - return ParserTreeItem::ConstPtr(); + const FilePath fileName = FilePath::fromString(doc->fileName()); ParserTreeItem::ConstPtr itemPtr = ParserTreeItem::parseDocument(doc); @@ -284,7 +266,7 @@ ParserTreeItem::ConstPtr Parser::getCachedOrParseDocumentTree(const CPlusPlus::D return ParserTreeItem::ConstPtr(); const QString &fileName = doc->fileName(); - const auto it = d->m_documentCache.constFind(fileName); + const auto it = d->m_documentCache.constFind(FilePath::fromString(fileName)); if (it != d->m_documentCache.constEnd() && !it.value().tree.isNull() && it.value().treeRevision == doc->revision()) { return it.value().tree; @@ -297,13 +279,17 @@ ParserTreeItem::ConstPtr Parser::getCachedOrParseDocumentTree(const CPlusPlus::D the internal storage. */ -void Parser::updateDocuments(const QList &docs) +void Parser::updateDocuments(const QSet &documentPaths) { - for (const CPlusPlus::Document::Ptr &doc: docs) { - const QString &name = doc->fileName(); + updateDocumentsFromSnapshot(documentPaths, CppTools::CppModelManager::instance()->snapshot()); +} - // if it is external file (not in any of our projects) - if (!d->fileList.contains(name)) +void Parser::updateDocumentsFromSnapshot(const QSet &documentPaths, + const CPlusPlus::Snapshot &snapshot) +{ + for (const FilePath &documentPath : documentPaths) { + CPlusPlus::Document::Ptr doc = snapshot.document(documentPath); + if (doc.isNull()) continue; getParseDocumentTree(doc); @@ -311,16 +297,6 @@ void Parser::updateDocuments(const QList &docs) requestCurrentState(); } -/*! - Specifies the files that must be allowed for the parsing as a \a fileList. - Files outside of this list will not be in any tree. -*/ - -void Parser::setFileList(const QStringList &fileList) -{ - d->fileList = Utils::toSet(fileList); -} - /*! Removes the files defined in the \a fileList from the parsing. */ @@ -331,11 +307,11 @@ void Parser::removeFiles(const QStringList &fileList) return; for (const QString &name : fileList) { - d->fileList.remove(name); - d->m_documentCache.remove(name); - d->m_projectCache.remove(name); + const FilePath filePath = FilePath::fromString(name); + d->m_documentCache.remove(filePath); + d->m_projectCache.remove(filePath); for (auto it = d->m_projectCache.begin(); it != d->m_projectCache.end(); ++it) - it.value().fileList.removeOne(name); + it.value().fileNames.remove(filePath); } requestCurrentState(); } @@ -343,75 +319,66 @@ void Parser::removeFiles(const QStringList &fileList) /*! Fully resets the internal state of the code parser to \a snapshot. */ -void Parser::resetData(const CPlusPlus::Snapshot &snapshot) +void Parser::resetData(const QHash> &projects) { d->m_projectCache.clear(); d->m_documentCache.clear(); - for (auto it = snapshot.begin(); it != snapshot.end(); ++it) - d->m_documentCache[it.key().toString()].document = it.value(); - // recalculate file list - FilePaths fileList; + const CPlusPlus::Snapshot &snapshot = CppTools::CppModelManager::instance()->snapshot(); + for (auto it = projects.cbegin(); it != projects.cend(); ++it) { + const auto projectData = it.value(); + QSet commonFiles; + for (const auto &fileInProject : projectData.second) { + CPlusPlus::Document::Ptr doc = snapshot.document(fileInProject); + if (doc.isNull()) + continue; + commonFiles.insert(fileInProject); + d->m_documentCache[fileInProject].document = doc; + } + d->m_projectCache.insert(it.key(), { 0, nullptr, projectData.first, commonFiles }); + } - // TODO: move a call to SessionManager::projects() out of this thread - for (const Project *prj : SessionManager::projects()) - fileList += prj->files(Project::SourceFiles); - setFileList(Utils::transform(fileList, &FilePath::toString)); + requestCurrentState(); +} + +void Parser::addProject(const FilePath &projectPath, const QString &projectName, + const FilePaths &filesInProject) +{ + const CPlusPlus::Snapshot &snapshot = CppTools::CppModelManager::instance()->snapshot(); + QSet commonFiles; + for (const auto &fileInProject : filesInProject) { + CPlusPlus::Document::Ptr doc = snapshot.document(fileInProject); + if (doc.isNull()) + continue; + commonFiles.insert(fileInProject); + d->m_documentCache[fileInProject].document = doc; + } + d->m_projectCache.insert(projectPath, { 0, nullptr, projectName, commonFiles }); + updateDocumentsFromSnapshot(commonFiles, snapshot); +} + +void Parser::removeProject(const FilePath &projectPath) +{ + auto it = d->m_projectCache.find(projectPath); + if (it == d->m_projectCache.end()) + return; + + const QSet &filesInProject = it.value().fileNames; + for (const FilePath &fileInProject : filesInProject) + d->m_documentCache.remove(fileInProject); + + d->m_projectCache.erase(it); requestCurrentState(); } -/*! - Fully resets the internal state of the code parser to the current state. - - \sa resetData -*/ - -void Parser::resetDataToCurrentState() -{ - // get latest data - resetData(CppTools::CppModelManager::instance()->snapshot()); -} - /*! Requests to emit a signal with the current tree state. */ - void Parser::requestCurrentState() { - // TODO: we need to have a fresh SessionManager data here, which we could pass to parse() emit treeRegenerated(parse()); } -// TODO: don't use Project class in this thread -QStringList Parser::getAllFiles(const Project *project) -{ - if (!project) - return {}; - - const QString projectPath = project->projectFilePath().toString(); - const auto it = d->m_projectCache.constFind(projectPath); - if (it != d->m_projectCache.constEnd()) - return it.value().fileList; - - const QStringList fileList = Utils::transform(project->files(Project::SourceFiles), - &FilePath::toString); - d->m_projectCache[projectPath].fileList = fileList; - return fileList; -} - -// TODO: don't use Project class in this thread -ParserTreeItem::ConstPtr Parser::addFlatTree(const Project *project) -{ - if (!project) - return {}; - - const QStringList fileList = getAllFiles(project); - if (fileList.isEmpty()) - return {}; - - return getCachedOrParseProjectTree(fileList, project->projectFilePath().toString()); -} - } // namespace Internal } // namespace ClassView diff --git a/src/plugins/classview/classviewparser.h b/src/plugins/classview/classviewparser.h index 23cb007251e..88f653d4b4a 100644 --- a/src/plugins/classview/classviewparser.h +++ b/src/plugins/classview/classviewparser.h @@ -54,28 +54,29 @@ public: void requestCurrentState(); void removeFiles(const QStringList &fileList); - void resetDataToCurrentState(); + void resetData(const QHash> &projects); + void addProject(const Utils::FilePath &projectPath, const QString &projectName, + const Utils::FilePaths &filesInProject); + void removeProject(const Utils::FilePath &projectPath); void setFlatMode(bool flat); - void updateDocuments(const QList &docs); + void updateDocuments(const QSet &documentPaths); signals: void treeRegenerated(const ParserTreeItem::ConstPtr &root); private: - void setFileList(const QStringList &fileList); - void resetData(const CPlusPlus::Snapshot &snapshot); + void updateDocumentsFromSnapshot(const QSet &documentPaths, + const CPlusPlus::Snapshot &snapshot); ParserTreeItem::ConstPtr getParseDocumentTree(const CPlusPlus::Document::Ptr &doc); ParserTreeItem::ConstPtr getCachedOrParseDocumentTree(const CPlusPlus::Document::Ptr &doc); - ParserTreeItem::ConstPtr getParseProjectTree(const QStringList &fileList, const QString &projectId); - ParserTreeItem::ConstPtr getCachedOrParseProjectTree(const QStringList &fileList, - const QString &projectId); + ParserTreeItem::ConstPtr getParseProjectTree(const Utils::FilePath &projectPath, + const QSet &filesInProject); + ParserTreeItem::ConstPtr getCachedOrParseProjectTree(const Utils::FilePath &projectPath, + const QSet &filesInProject); ParserTreeItem::ConstPtr parse(); - QStringList getAllFiles(const ProjectExplorer::Project *project); - ParserTreeItem::ConstPtr addFlatTree(const ProjectExplorer::Project *project); - //! Private class data pointer ParserPrivate *d; };