C++: Fix crash after triggering completion and closing editor

Fix use-after-free for the following case:
  1. Open an editor
  2. Trigger a long processing completion
     (e.g. simulate with QThread::msleep in
      CppCompletionAssistInterface::getCppSpecifics)
  3. ...and immediately close the editor (e.g. with Ctrl+W)
  4. Wait until it crashes.

The completion thread relied on the BuiltinEditorDocumentParser object,
which is deleted once the editor is closed. Fixed by sharing the
ownership of that object between the *EditorDocumentProcessor and the
completion assist interface.

This case came up when doing tests for the bug report below.

Task-number: QTCREATORBUG-14991
Change-Id: I0b009229e68fc6b7838740858cdc41a32403fe6f
Reviewed-by: David Schulz <david.schulz@theqtcompany.com>

src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp

@@ -94,7 +94,7 @@ ClangEditorDocumentProcessor::ClangEditorDocumentProcessor(
         TextEditor::TextDocument *document)
     : BaseEditorDocumentProcessor(document)
     , m_modelManagerSupport(modelManagerSupport)
-    , m_parser(document->filePath().toString())
+    , m_parser(new ClangEditorDocumentParser(document->filePath().toString()))
     , m_parserRevision(0)
     , m_semanticHighlighter(document)
     , m_builtinProcessor(document, /*enableSemanticHighlighter=*/ false)
@@ -114,7 +114,7 @@ ClangEditorDocumentProcessor::ClangEditorDocumentProcessor(
             const int firstLine = 1;
             const int lastLine = baseTextDocument()->document()->blockCount();
 
-            CreateMarkers *createMarkers = CreateMarkers::create(m_parser.semanticMarker(),
+            CreateMarkers *createMarkers = CreateMarkers::create(m_parser->semanticMarker(),
                                                                  baseTextDocument()->filePath().toString(),
                                                                  firstLine, lastLine);
             return createMarkers->start();
@@ -169,9 +169,9 @@ CppTools::SemanticInfo ClangEditorDocumentProcessor::recalculateSemanticInfo()
     return m_builtinProcessor.recalculateSemanticInfo();
 }
 
-CppTools::BaseEditorDocumentParser *ClangEditorDocumentProcessor::parser()
+CppTools::BaseEditorDocumentParser::Ptr ClangEditorDocumentProcessor::parser()
 {
-    return &m_parser;
+    return m_parser;
 }
 
 CPlusPlus::Snapshot ClangEditorDocumentProcessor::snapshot()
@@ -196,7 +196,7 @@ ClangEditorDocumentProcessor *ClangEditorDocumentProcessor::get(const QString &f
 
 void ClangEditorDocumentProcessor::updateProjectPartAndTranslationUnitForCompletion()
 {
-    const CppTools::ProjectPart::Ptr projectPart = m_parser.projectPart();
+    const CppTools::ProjectPart::Ptr projectPart = m_parser->projectPart();
     QTC_ASSERT(projectPart, return);
 
     updateTranslationUnitForCompletion(*projectPart.data());
@@ -209,11 +209,11 @@ void ClangEditorDocumentProcessor::onParserFinished()
         return;
 
     // Emit ifdefed out blocks
-    const auto ifdefoutBlocks = toTextEditorBlocks(m_parser.ifdefedOutBlocks());
+    const auto ifdefoutBlocks = toTextEditorBlocks(m_parser->ifdefedOutBlocks());
     emit ifdefedOutBlocksUpdated(revision(), ifdefoutBlocks);
 
     // Emit code warnings
-    const auto diagnostics = toCppToolsDiagnostics(filePath(), m_parser.diagnostics());
+    const auto diagnostics = toCppToolsDiagnostics(filePath(), m_parser->diagnostics());
     const auto codeWarnings = toTextEditorSelections(diagnostics, textDocument());
     emit codeWarningsUpdated(revision(), codeWarnings);
 

src/plugins/clangcodemodel/clangeditordocumentprocessor.h

@@ -59,7 +59,7 @@ public:
     void semanticRehighlight() override;
     void recalculateSemanticInfoDetached(bool force) override;
     CppTools::SemanticInfo recalculateSemanticInfo() override;
-    CppTools::BaseEditorDocumentParser *parser() override;
+    CppTools::BaseEditorDocumentParser::Ptr parser() override;
     CPlusPlus::Snapshot snapshot() override;
     bool isParserRunning() const override;
 
@@ -78,7 +78,7 @@ private:
 
     QPointer<ModelManagerSupportClang> m_modelManagerSupport;
 
-    ClangEditorDocumentParser m_parser;
+    QSharedPointer<ClangEditorDocumentParser> m_parser;
     CppTools::ProjectPart::Ptr m_projectPart;
     QFutureWatcher<void> m_parserWatcher;
     unsigned m_parserRevision;

src/plugins/clangcodemodel/clangutils.cpp

@@ -234,7 +234,7 @@ QStringList createPCHInclusionOptions(const QString &pchFile)
 
 ProjectPart::Ptr projectPartForFile(const QString &filePath)
 {
-    if (CppTools::BaseEditorDocumentParser *parser = CppTools::BaseEditorDocumentParser::get(filePath))
+    if (const auto parser = CppTools::BaseEditorDocumentParser::get(filePath))
         return parser->projectPart();
     return ProjectPart::Ptr();
 }

src/plugins/cppeditor/cppeditordocument.cpp

@@ -266,7 +266,7 @@ void CppEditorDocument::updatePreprocessorSettings()
 void CppEditorDocument::setPreprocessorSettings(const CppTools::ProjectPart::Ptr &projectPart,
                                                 const QByteArray &defines)
 {
-    CppTools::BaseEditorDocumentParser *parser = processor()->parser();
+    const auto parser = processor()->parser();
     QTC_ASSERT(parser, return);
     if (parser->projectPart() != projectPart || parser->configuration().editorDefines != defines) {
         CppTools::BaseEditorDocumentParser::Configuration config = parser->configuration();

src/plugins/cpptools/baseeditordocumentparser.cpp

@@ -104,14 +104,14 @@ ProjectPart::Ptr BaseEditorDocumentParser::projectPart() const
     return state().projectPart;
 }
 
-BaseEditorDocumentParser *BaseEditorDocumentParser::get(const QString &filePath)
+BaseEditorDocumentParser::Ptr BaseEditorDocumentParser::get(const QString &filePath)
 {
     CppModelManager *cmmi = CppModelManager::instance();
     if (CppEditorDocumentHandle *cppEditorDocument = cmmi->cppEditorDocument(filePath)) {
         if (BaseEditorDocumentProcessor *processor = cppEditorDocument->processor())
             return processor->parser();
     }
-    return 0;
+    return BaseEditorDocumentParser::Ptr();
 }
 
 ProjectPart::Ptr BaseEditorDocumentParser::determineProjectPart(const QString &filePath,

src/plugins/cpptools/baseeditordocumentparser.h

@@ -44,7 +44,8 @@ class CPPTOOLS_EXPORT BaseEditorDocumentParser : public QObject
     Q_OBJECT
 
 public:
-    static BaseEditorDocumentParser *get(const QString &filePath);
+    using Ptr = QSharedPointer<BaseEditorDocumentParser>;;
+    static Ptr get(const QString &filePath);
 
     struct Configuration {
         bool stickToPreviousProjectPart = true;

src/plugins/cpptools/baseeditordocumentprocessor.cpp

@@ -118,7 +118,7 @@ QList<QTextEdit::ExtraSelection> BaseEditorDocumentProcessor::toTextEditorSelect
 }
 
 void BaseEditorDocumentProcessor::runParser(QFutureInterface<void> &future,
-                                            BaseEditorDocumentParser *parser,
+                                            BaseEditorDocumentParser::Ptr parser,
                                             BaseEditorDocumentParser::InMemoryInfo info)
 {
     future.setProgressRange(0, 1);

src/plugins/cpptools/baseeditordocumentprocessor.h

@@ -62,7 +62,7 @@ public:
     virtual void recalculateSemanticInfoDetached(bool force) = 0;
     virtual CppTools::SemanticInfo recalculateSemanticInfo() = 0;
     virtual CPlusPlus::Snapshot snapshot() = 0;
-    virtual BaseEditorDocumentParser *parser() = 0;
+    virtual BaseEditorDocumentParser::Ptr parser() = 0;
     virtual bool isParserRunning() const = 0;
 
 public:
@@ -85,7 +85,7 @@ protected:
             QTextDocument *textDocument);
 
     static void runParser(QFutureInterface<void> &future,
-                          CppTools::BaseEditorDocumentParser *parser,
+                          BaseEditorDocumentParser::Ptr parser,
                           BaseEditorDocumentParser::InMemoryInfo info);
 
     // Convenience

src/plugins/cpptools/builtineditordocumentparser.cpp

@@ -226,11 +226,11 @@ ProjectPart::HeaderPaths BuiltinEditorDocumentParser::headerPaths() const
     return extraState().headerPaths;
 }
 
-BuiltinEditorDocumentParser *BuiltinEditorDocumentParser::get(const QString &filePath)
+BuiltinEditorDocumentParser::Ptr BuiltinEditorDocumentParser::get(const QString &filePath)
 {
-    if (BaseEditorDocumentParser *b = BaseEditorDocumentParser::get(filePath))
+    if (BaseEditorDocumentParser::Ptr b = BaseEditorDocumentParser::get(filePath))
-        return qobject_cast<BuiltinEditorDocumentParser *>(b);
+        return b.objectCast<BuiltinEditorDocumentParser>();
-    return 0;
+    return BuiltinEditorDocumentParser::Ptr();
 }
 
 void BuiltinEditorDocumentParser::addFileAndDependencies(Snapshot *snapshot,

src/plugins/cpptools/builtineditordocumentparser.h

@@ -61,7 +61,8 @@ signals:
     void finished(CPlusPlus::Document::Ptr document, CPlusPlus::Snapshot snapshot);
 
 public:
-    static BuiltinEditorDocumentParser *get(const QString &filePath);
+    using Ptr = QSharedPointer<BuiltinEditorDocumentParser>;
+    static Ptr get(const QString &filePath);
 
 private:
     void updateHelper(const InMemoryInfo &info) override;

src/plugins/cpptools/builtineditordocumentprocessor.cpp

@@ -125,7 +125,7 @@ BuiltinEditorDocumentProcessor::BuiltinEditorDocumentProcessor(
         TextEditor::TextDocument *document,
         bool enableSemanticHighlighter)
     : BaseEditorDocumentProcessor(document)
-    , m_parser(document->filePath().toString())
+    , m_parser(new BuiltinEditorDocumentParser(document->filePath().toString()))
     , m_codeWarningsUpdated(false)
     , m_semanticHighlighter(enableSemanticHighlighter
                             ? new CppTools::SemanticHighlighter(document)
@@ -135,9 +135,9 @@ BuiltinEditorDocumentProcessor::BuiltinEditorDocumentProcessor(
 
     QSharedPointer<CppCodeModelSettings> cms = CppToolsPlugin::instance()->codeModelSettings();
 
-    BaseEditorDocumentParser::Configuration config = m_parser.configuration();
+    BaseEditorDocumentParser::Configuration config = m_parser->configuration();
     config.usePrecompiledHeaders = cms->pchUsage() != CppCodeModelSettings::PchUse_None;
-    m_parser.setConfiguration(config);
+    m_parser->setConfiguration(config);
 
     if (m_semanticHighlighter) {
         m_semanticHighlighter->setHighlightingRunner(
@@ -152,7 +152,7 @@ BuiltinEditorDocumentProcessor::BuiltinEditorDocumentProcessor(
             });
     }
 
-    connect(&m_parser, &BuiltinEditorDocumentParser::finished,
+    connect(m_parser.data(), &BuiltinEditorDocumentParser::finished,
             this, &BuiltinEditorDocumentProcessor::onParserFinished);
     connect(&m_semanticInfoUpdater, &SemanticInfoUpdater::updated,
             this, &BuiltinEditorDocumentProcessor::onSemanticInfoUpdated);
@@ -171,14 +171,14 @@ void BuiltinEditorDocumentProcessor::run()
                                        BuiltinEditorDocumentParser::InMemoryInfo(false));
 }
 
-BaseEditorDocumentParser *BuiltinEditorDocumentProcessor::parser()
+BaseEditorDocumentParser::Ptr BuiltinEditorDocumentProcessor::parser()
 {
-    return &m_parser;
+    return m_parser;
 }
 
 CPlusPlus::Snapshot BuiltinEditorDocumentProcessor::snapshot()
 {
-    return m_parser.snapshot();
+    return m_parser->snapshot();
 }
 
 void BuiltinEditorDocumentProcessor::recalculateSemanticInfoDetached(bool force)

src/plugins/cpptools/builtineditordocumentprocessor.h

@@ -53,7 +53,7 @@ public:
     void recalculateSemanticInfoDetached(bool force) override;
     void semanticRehighlight() override;
     CppTools::SemanticInfo recalculateSemanticInfo() override;
-    BaseEditorDocumentParser *parser() override;
+    BaseEditorDocumentParser::Ptr parser() override;
     CPlusPlus::Snapshot snapshot() override;
     bool isParserRunning() const override;
 
@@ -66,7 +66,7 @@ private:
     SemanticInfo::Source createSemanticInfoSource(bool force) const;
 
 private:
-    BuiltinEditorDocumentParser m_parser;
+    BuiltinEditorDocumentParser::Ptr m_parser;
     QFuture<void> m_parserFuture;
 
     CPlusPlus::Snapshot m_documentSnapshot;

src/plugins/cpptools/cppcompletionassist.cpp

@@ -426,13 +426,13 @@ AssistInterface *InternalCompletionAssistProvider::createAssistInterface(
 {
     QTC_ASSERT(textEditorWidget, return 0);
 
-    CppModelManager *modelManager = CppModelManager::instance();
     return new CppCompletionAssistInterface(filePath,
                                             textEditorWidget,
+                                            BuiltinEditorDocumentParser::get(filePath),
                                             languageFeatures,
                                             position,
                                             reason,
-                                            modelManager->workingCopy());
+                                            CppModelManager::instance()->workingCopy());
 }
 
 // -----------------
@@ -2187,11 +2187,11 @@ void CppCompletionAssistInterface::getCppSpecifics() const
         return;
     m_gotCppSpecifics = true;
 
-    if (BuiltinEditorDocumentParser *parser = BuiltinEditorDocumentParser::get(fileName())) {
+    if (m_parser) {
-        parser->update(BuiltinEditorDocumentParser::InMemoryInfo(false));
+        m_parser->update(BuiltinEditorDocumentParser::InMemoryInfo(false));
-        m_snapshot = parser->snapshot();
+        m_snapshot = m_parser->snapshot();
-        m_headerPaths = parser->headerPaths();
+        m_headerPaths = m_parser->headerPaths();
-        if (Document::Ptr document = parser->document())
+        if (Document::Ptr document = m_parser->document())
             m_languageFeatures = document->languageFeatures();
         else
             m_languageFeatures = LanguageFeatures::defaultFeatures();

src/plugins/cpptools/cppcompletionassist.h

@@ -31,6 +31,7 @@
 #ifndef CPPCOMPLETIONASSIST_H
 #define CPPCOMPLETIONASSIST_H
 
+#include "builtineditordocumentparser.h"
 #include "cppcompletionassistprocessor.h"
 #include "cppcompletionassistprovider.h"
 #include "cppmodelmanager.h"
@@ -171,11 +172,13 @@ class CppCompletionAssistInterface : public TextEditor::AssistInterface
 public:
     CppCompletionAssistInterface(const QString &filePath,
                                  const TextEditor::TextEditorWidget *textEditorWidget,
+                                 BuiltinEditorDocumentParser::Ptr parser,
                                  const CPlusPlus::LanguageFeatures &languageFeatures,
                                  int position,
                                  TextEditor::AssistReason reason,
                                  const WorkingCopy &workingCopy)
         : TextEditor::AssistInterface(textEditorWidget->document(), position, filePath, reason)
+        , m_parser(parser)
         , m_gotCppSpecifics(false)
         , m_workingCopy(workingCopy)
         , m_languageFeatures(languageFeatures)
@@ -204,6 +207,7 @@ public:
 private:
     void getCppSpecifics() const;
 
+    BuiltinEditorDocumentParser::Ptr m_parser;
     mutable bool m_gotCppSpecifics;
     WorkingCopy m_workingCopy;
     mutable CPlusPlus::Snapshot m_snapshot;

src/plugins/cpptools/cppmodelmanager_test.cpp

@@ -910,7 +910,7 @@ void CppToolsPlugin::test_modelmanager_precompiled_headers()
         QCOMPARE(Core::DocumentModel::openedDocuments().size(), 1);
         QVERIFY(mm->isCppEditor(editor));
 
-        auto *parser = BuiltinEditorDocumentParser::get(fileName);
+        auto parser = BuiltinEditorDocumentParser::get(fileName);
         QVERIFY(parser);
         BaseEditorDocumentParser::Configuration config = parser->configuration();
         config.usePrecompiledHeaders = true;
@@ -994,7 +994,7 @@ void CppToolsPlugin::test_modelmanager_defines_per_editor()
         QVERIFY(mm->isCppEditor(editor));
 
         const QString filePath = editor->document()->filePath().toString();
-        BaseEditorDocumentParser *parser = BaseEditorDocumentParser::get(filePath);
+        const auto parser = BaseEditorDocumentParser::get(filePath);
         BaseEditorDocumentParser::Configuration config = parser->configuration();
         config.editorDefines = editorDefines.toUtf8();
         parser->setConfiguration(config);

src/plugins/designer/gotoslot_test.cpp

@@ -196,12 +196,12 @@ public:
         }
 
         // Compare
-        BuiltinEditorDocumentParser *cppDocumentParser = BuiltinEditorDocumentParser::get(cppFile);
+        const auto cppDocumentParser = BuiltinEditorDocumentParser::get(cppFile);
         QVERIFY(cppDocumentParser);
         const Document::Ptr cppDocument = cppDocumentParser->document();
         QVERIFY(checkDiagsnosticMessages(cppDocument));
 
-        BuiltinEditorDocumentParser *hDocumentParser = BuiltinEditorDocumentParser::get(hFile);
+        const auto hDocumentParser = BuiltinEditorDocumentParser::get(hFile);
         QVERIFY(hDocumentParser);
         const Document::Ptr hDocument = hDocumentParser->document();
         QVERIFY(checkDiagsnosticMessages(hDocument));

src/plugins/texteditor/codeassist/codeassistant.cpp

@@ -89,7 +89,6 @@ public:
     virtual bool eventFilter(QObject *o, QEvent *e);
 
 private:
-    void finalizeRequest();
     void proposalComputed();
     void processProposalItem(AssistProposalItem *proposalItem);
     void handlePrefixExpansion(const QString &newPrefix);
@@ -251,7 +250,7 @@ void CodeAssistantPrivate::requestProposal(AssistReason reason,
         connect(m_requestRunner, &ProcessorRunner::finished,
                 this, &CodeAssistantPrivate::proposalComputed);
         connect(m_requestRunner, &ProcessorRunner::finished,
-                this, &CodeAssistantPrivate::finalizeRequest);
+                m_requestRunner, &QObject::deleteLater);
         connect(m_requestRunner, &ProcessorRunner::finished,
                 q, &CodeAssistant::finished);
         assistInterface->prepareForAsyncUse();
@@ -383,12 +382,6 @@ void CodeAssistantPrivate::handlePrefixExpansion(const QString &newPrefix)
     notifyChange();
 }
 
-void CodeAssistantPrivate::finalizeRequest()
-{
-    if (ProcessorRunner *runner = qobject_cast<ProcessorRunner *>(sender()))
-        delete runner;
-}
-
 void CodeAssistantPrivate::finalizeProposal()
 {
     stopAutomaticProposalTimer();