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 <eike.ziller@qt.io>

src/plugins/classview/classviewmanager.cpp

@@ -44,6 +44,7 @@
 #include <QTimer>
 
 using namespace Core;
+using namespace ProjectExplorer;
 using namespace Utils;
 
 namespace ClassView {
@@ -94,7 +95,7 @@ public:
     ParserTreeItem::ConstPtr m_root;
 
     QTimer m_timer;
-    QHash<QString, CPlusPlus::Document::Ptr> m_awaitingDocuments;
+    QSet<FilePath> 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<FilePath, QPair<QString, FilePaths>> 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<CPlusPlus::Document::Ptr> docsToBeUpdated = d->m_awaitingDocuments.values();
+        const QSet<FilePath> 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();
+    // TODO: this one may change into getter (when a new class view widget is being shown)
-}
-
-/*!
-    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;
-
     QMetaObject::invokeMethod(d->m_parser, &Parser::requestCurrentState, Qt::QueuedConnection);
 }
 

src/plugins/classview/classviewmanager.h

@@ -60,7 +60,6 @@ signals:
     void treeDataUpdate(QSharedPointer<QStandardItem> result);
 
 private:
-    void onProjectListChanged();
     void initialize();
 
     inline bool state() const;

src/plugins/classview/classviewparser.cpp

@@ -32,18 +32,13 @@
 
 // other
 #include <cpptools/cppmodelmanager.h>
-#include <projectexplorer/projectexplorer.h>
-#include <projectexplorer/session.h>
-#include <projectexplorer/project.h>
-
 #include <utils/algorithm.h>
 #include <utils/qtcassert.h>
 
-#include <QStandardItem>
+#include <QElapsedTimer>
 #include <QDebug>
 #include <QHash>
 #include <QSet>
-#include <QElapsedTimer>
 
 enum { debug = false };
 
@@ -67,17 +62,11 @@ namespace Internal {
    \brief The Parser class parses C++ information. Multithreading is supported.
 */
 
-/*!
-    \fn void Parser::treeDataUpdate(QSharedPointer<QStandardItem> 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<FilePath> fileNames;
     };
 
     // Project file path to its cached data
-    QHash<QString, DocumentCache> m_documentCache;
+    QHash<FilePath, DocumentCache> m_documentCache;
     // Project file path to its cached data
-    QHash<QString, ProjectCache> m_projectCache;
+    QHash<FilePath, ProjectCache> m_projectCache;
-
-    // other
-    //! List for files which has to be parsed
-    QSet<QString> fileList;
 
     //! 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<SymbolInformation, ParserTreeItem::ConstPtr> projectTrees;
 
-    // TODO: move a call to SessionManager::projects() out of this thread
+    for (auto it = d->m_projectCache.cbegin(); it != d->m_projectCache.cend(); ++it) {
-    for (const Project *prj : SessionManager::projects()) {
+        const ParserPrivate::ProjectCache &projectCache = it.value();
-        const QString prjName(prj->displayName());
+        const FilePath projectPath = it.key();
-        const QString prjType = prj->projectFilePath().toString();
+        const SymbolInformation projectInfo = { projectCache.projectName, projectPath.toString() };
-        const SymbolInformation inf(prjName, prjType);
+        ParserTreeItem::ConstPtr item = getCachedOrParseProjectTree(projectPath, projectCache.fileNames);
-
-        ParserTreeItem::ConstPtr item = addFlatTree(prj);
         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,
+ParserTreeItem::ConstPtr Parser::getParseProjectTree(const FilePath &projectPath,
-                                                const QString &projectId)
+                                                     const QSet<FilePath> &filesInProject)
 {
     //! \todo Way to optimize - for documentUpdate - use old cached project and subtract
     //! changed files only (old edition), and add curent editions
 
     QList<ParserTreeItem::ConstPtr> docTrees;
     unsigned revision = 0;
-    for (const QString &file : fileList) {
+    for (const FilePath &fileInProject : filesInProject) {
-        const CPlusPlus::Document::Ptr &doc = d->document(file);
+        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()) {
+    if (!projectPath.isEmpty()) {
-        ParserPrivate::ProjectCache &projectCache = d->m_projectCache[projectId];
+        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,
+ParserTreeItem::ConstPtr Parser::getCachedOrParseProjectTree(const FilePath &projectPath,
-                                                        const QString &projectId)
+                                                             const QSet<FilePath> &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) {
+        for (const FilePath &fileInProject : filesInProject) {
-            const CPlusPlus::Document::Ptr &doc = d->document(file);
+            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();
+    const FilePath fileName = FilePath::fromString(doc->fileName());
-    if (!d->fileList.contains(fileName))
-        return ParserTreeItem::ConstPtr();
 
     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<CPlusPlus::Document::Ptr> &docs)
+void Parser::updateDocuments(const QSet<FilePath> &documentPaths)
 {
-    for (const CPlusPlus::Document::Ptr &doc: docs) {
+    updateDocumentsFromSnapshot(documentPaths, CppTools::CppModelManager::instance()->snapshot());
-        const QString &name = doc->fileName();
+}
 
-        // if it is external file (not in any of our projects)
+void Parser::updateDocumentsFromSnapshot(const QSet<Utils::FilePath> &documentPaths,
-        if (!d->fileList.contains(name))
+                                 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<CPlusPlus::Document::Ptr> &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);
+        const FilePath filePath = FilePath::fromString(name);
-        d->m_documentCache.remove(name);
+        d->m_documentCache.remove(filePath);
-        d->m_projectCache.remove(name);
+        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<FilePath, QPair<QString, FilePaths>> &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
+    const CPlusPlus::Snapshot &snapshot = CppTools::CppModelManager::instance()->snapshot();
-    FilePaths fileList;
+    for (auto it = projects.cbegin(); it != projects.cend(); ++it) {
+        const auto projectData = it.value();
+        QSet<FilePath> 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
+    requestCurrentState();
-    for (const Project *prj : SessionManager::projects())
+}
-        fileList += prj->files(Project::SourceFiles);
+
-    setFileList(Utils::transform(fileList, &FilePath::toString));
+void Parser::addProject(const FilePath &projectPath, const QString &projectName,
+                        const FilePaths &filesInProject)
+{
+    const CPlusPlus::Snapshot &snapshot = CppTools::CppModelManager::instance()->snapshot();
+    QSet<FilePath> 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<FilePath> &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

src/plugins/classview/classviewparser.h

@@ -54,28 +54,29 @@ public:
 
     void requestCurrentState();
     void removeFiles(const QStringList &fileList);
-    void resetDataToCurrentState();
+    void resetData(const QHash<Utils::FilePath, QPair<QString, Utils::FilePaths>> &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<CPlusPlus::Document::Ptr> &docs);
+    void updateDocuments(const QSet<Utils::FilePath> &documentPaths);
 
 signals:
     void treeRegenerated(const ParserTreeItem::ConstPtr &root);
 
 private:
-    void setFileList(const QStringList &fileList);
+    void updateDocumentsFromSnapshot(const QSet<Utils::FilePath> &documentPaths,
-    void resetData(const CPlusPlus::Snapshot &snapshot);
+                                     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 getParseProjectTree(const Utils::FilePath &projectPath,
-    ParserTreeItem::ConstPtr getCachedOrParseProjectTree(const QStringList &fileList,
+                                                 const QSet<Utils::FilePath> &filesInProject);
-                                                    const QString &projectId);
+    ParserTreeItem::ConstPtr getCachedOrParseProjectTree(const Utils::FilePath &projectPath,
+                                                         const QSet<Utils::FilePath> &filesInProject);
     ParserTreeItem::ConstPtr parse();
 
-    QStringList getAllFiles(const ProjectExplorer::Project *project);
-    ParserTreeItem::ConstPtr addFlatTree(const ProjectExplorer::Project *project);
-
     //! Private class data pointer
     ParserPrivate *d;
 };