Refactor HighlightingResultReporter

In some unlikely circumstances it may happen that
HighlightingResultReporter might be destroyed before
returning from the HighlightingResultReporter::start()
function. This may lead to undefined behavior.

Refactor the HighlightingResultReporter so that instead
of using QRunnable subclass we define simple function
returning the QFuture object directly.

Change-Id: Ib833771a7e46e87c83d10b59ca056a0147fabe88
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp

@@ -37,6 +37,8 @@
 #include <diagnosticcontainer.h>
 #include <sourcelocationcontainer.h>
 
+#include <clangsupport/tokeninfocontainer.h>
+
 #include <cppeditor/builtincursorinfo.h>
 #include <cppeditor/clangdiagnosticconfigsmodel.h>
 #include <cppeditor/compileroptionsbuilder.h>
@@ -271,10 +273,7 @@ void ClangEditorDocumentProcessor::updateHighlighting(
         emit ifdefedOutBlocksUpdated(documentRevision, skippedPreprocessorBlocks);
 
         m_semanticHighlighter.setHighlightingRunner(
-            [tokenInfos]() {
+            [tokenInfos]() { return highlightResults(tokenInfos); });
-                auto *reporter = new HighlightingResultReporter(tokenInfos);
-                return reporter->start();
-            });
         m_semanticHighlighter.run();
     }
 }

src/plugins/clangcodemodel/clanghighlightingresultreporter.cpp

@@ -25,12 +25,13 @@
 
 #include "clanghighlightingresultreporter.h"
 
+#include <clangsupport/tokeninfocontainer.h>
 #include <cppeditor/semantichighlighter.h>
+#include <texteditor/semantichighlighter.h>
 #include <texteditor/textstyles.h>
+#include <utils/runextensions.h>
 #include <utils/qtcassert.h>
 
-#include <QFuture>
-
 namespace {
 
 TextEditor::TextStyle toTextStyle(ClangBackEnd::HighlightingType type)
@@ -161,78 +162,62 @@ TextEditor::HighlightingResult toHighlightingResult(
     return result;
 }
 
+void highlightResultsImpl(QFutureInterface<TextEditor::HighlightingResult> &fi,
+                          const QVector<ClangBackEnd::TokenInfoContainer> &tokenInfos,
+                          int chunkSize)
+{
+    if (fi.isCanceled())
+        return;
+
+    QVector<TextEditor::HighlightingResult> chunksToReport;
+    chunksToReport.reserve(chunkSize + 1);
+
+    using ClangBackEnd::HighlightingType;
+    bool flushRequested = false;
+    int flushLine = 0;
+
+    auto reportAndClearCurrentChunks = [&] {
+        flushRequested = false;
+        flushLine = 0;
+
+        if (!chunksToReport.isEmpty()) {
+            fi.reportResults(chunksToReport);
+            chunksToReport.erase(chunksToReport.begin(), chunksToReport.end());
+        }
+    };
+
+    auto reportChunkWise = [&](const TextEditor::HighlightingResult &highlightingResult) {
+        if (chunksToReport.size() >= chunkSize) {
+            if (flushRequested && highlightingResult.line != flushLine) {
+                reportAndClearCurrentChunks();
+            } else if (!flushRequested) {
+                flushRequested = true;
+                flushLine = highlightingResult.line;
+            }
+        }
+
+        chunksToReport.append(highlightingResult);
+    };
+
+    for (const auto &tokenInfo : tokenInfos)
+        reportChunkWise(toHighlightingResult(tokenInfo));
+
+    if (fi.isCanceled())
+        return;
+
+    reportAndClearCurrentChunks();
+}
+
 } // anonymous
 
 namespace ClangCodeModel {
 namespace Internal {
 
-HighlightingResultReporter::HighlightingResultReporter(
+QFuture<TextEditor::HighlightingResult> highlightResults(
-        const QVector<ClangBackEnd::TokenInfoContainer> &tokenInfos)
+                      const QVector<ClangBackEnd::TokenInfoContainer> &tokenInfos,
-    : m_tokenInfos(tokenInfos)
+                      int chunkSize)
 {
-    m_chunksToReport.reserve(m_chunkSize + 1);
+    return Utils::runAsync(highlightResultsImpl, tokenInfos, chunkSize);
-}
-
-void HighlightingResultReporter::reportChunkWise(
-        const TextEditor::HighlightingResult &highlightingResult)
-{
-    if (m_chunksToReport.size() >= m_chunkSize) {
-        if (m_flushRequested && highlightingResult.line != m_flushLine) {
-            reportAndClearCurrentChunks();
-        } else if (!m_flushRequested) {
-            m_flushRequested = true;
-            m_flushLine = highlightingResult.line;
-        }
-    }
-
-    m_chunksToReport.append(highlightingResult);
-}
-
-void HighlightingResultReporter::reportAndClearCurrentChunks()
-{
-    m_flushRequested = false;
-    m_flushLine = 0;
-
-    if (!m_chunksToReport.isEmpty()) {
-        reportResults(m_chunksToReport);
-        m_chunksToReport.erase(m_chunksToReport.begin(), m_chunksToReport.end());
-    }
-}
-
-void HighlightingResultReporter::setChunkSize(int chunkSize)
-{
-    m_chunkSize = chunkSize;
-}
-
-void HighlightingResultReporter::run()
-{
-    run_internal();
-    reportFinished();
-}
-
-void HighlightingResultReporter::run_internal()
-{
-    if (isCanceled())
-        return;
-
-    using ClangBackEnd::HighlightingType;
-
-    for (const auto &tokenInfo : qAsConst(m_tokenInfos))
-        reportChunkWise(toHighlightingResult(tokenInfo));
-
-    if (isCanceled())
-        return;
-
-    reportAndClearCurrentChunks();
-}
-
-QFuture<TextEditor::HighlightingResult> HighlightingResultReporter::start()
-{
-    this->setRunnable(this);
-    this->reportStarted();
-    QFuture<TextEditor::HighlightingResult> future = this->future();
-    QThreadPool::globalInstance()->start(this, QThread::LowestPriority);
-    return future;
 }
 
 } // namespace Internal

src/plugins/clangcodemodel/clanghighlightingresultreporter.h

@@ -25,48 +25,18 @@
 
 #pragma once
 
-#include <QFutureInterface>
+#include <QFuture>
-#include <QObject>
+#include <QVector>
-#include <QRunnable>
-#include <QThreadPool>
 
-#include <texteditor/semantichighlighter.h>
+namespace TextEditor { class HighlightingResult; }
-
+namespace ClangBackEnd { class TokenInfoContainer; }
-#include <clangsupport/tokeninfocontainer.h>
 
 namespace ClangCodeModel {
 namespace Internal {
 
-class HighlightingResultReporter:
+QFuture<TextEditor::HighlightingResult> highlightResults(
-        public QObject,
+                      const QVector<ClangBackEnd::TokenInfoContainer> &tokenInfos,
-        public QRunnable,
+                      int chunkSize = 100);
-        public QFutureInterface<TextEditor::HighlightingResult>
-{
-    Q_OBJECT
-
-public:
-    HighlightingResultReporter(const QVector<ClangBackEnd::TokenInfoContainer> &tokenInfos);
-
-    void setChunkSize(int chunkSize);
-
-    QFuture<TextEditor::HighlightingResult> start();
-
-private:
-    void run() override;
-    void run_internal();
-
-    void reportChunkWise(const TextEditor::HighlightingResult &highlightingResult);
-    void reportAndClearCurrentChunks();
-
-private:
-    QVector<ClangBackEnd::TokenInfoContainer> m_tokenInfos;
-    QVector<TextEditor::HighlightingResult> m_chunksToReport;
-
-    int m_chunkSize = 100;
-
-    bool m_flushRequested = false;
-    int m_flushLine = 0;
-};
 
 } // namespace Internal
 } // namespace ClangCodeModel

tests/unit/unittest/highlightingresultreporter-test.cpp

@@ -43,7 +43,6 @@ using ClangBackEnd::Document;
 using ClangBackEnd::Documents;
 using ClangBackEnd::UnsavedFiles;
 using ClangBackEnd::ChunksReportedMonitor;
-using ClangCodeModel::Internal::HighlightingResultReporter;
 
 namespace {
 
@@ -87,9 +86,7 @@ QVector<TokenInfoContainer> generateTokenInfos(uint count)
 
 TEST_F(HighlightingResultReporter, StartAndFinish)
 {
-    auto reporter = new ::HighlightingResultReporter(noTokenInfos());
+    auto future = ClangCodeModel::Internal::highlightResults(noTokenInfos());
-
-    auto future = reporter->start();
 
     future.waitForFinished();
     ASSERT_THAT(future.isFinished(), true);
@@ -97,9 +94,7 @@ TEST_F(HighlightingResultReporter, StartAndFinish)
 
 TEST_F(HighlightingResultReporter, ReportNothingIfNothingToReport)
 {
-    auto reporter = new ::HighlightingResultReporter(generateTokenInfos(0));
+    auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(0));
-
-    auto future = reporter->start();
 
     ChunksReportedMonitor monitor(future);
     ASSERT_THAT(monitor.resultsReadyCounter(), 0L);
@@ -107,10 +102,7 @@ TEST_F(HighlightingResultReporter, ReportNothingIfNothingToReport)
 
 TEST_F(HighlightingResultReporter, ReportSingleResultAsOneChunk)
 {
-    auto reporter = new ::HighlightingResultReporter(generateTokenInfos(1));
+    auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(1), 1);
-    reporter->setChunkSize(1);
-
-    auto future = reporter->start();
 
     ChunksReportedMonitor monitor(future);
     ASSERT_THAT(monitor.resultsReadyCounter(), 1L);
@@ -118,11 +110,7 @@ TEST_F(HighlightingResultReporter, ReportSingleResultAsOneChunk)
 
 TEST_F(HighlightingResultReporter, ReportRestIfChunkSizeNotReached)
 {
-    auto reporter = new ::HighlightingResultReporter(generateTokenInfos(1));
+    auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(1), 100);
-    const int notReachedChunkSize = 100;
-    reporter->setChunkSize(notReachedChunkSize);
-
-    auto future = reporter->start();
 
     ChunksReportedMonitor monitor(future);
     ASSERT_THAT(monitor.resultsReadyCounter(), 1L);
@@ -130,10 +118,7 @@ TEST_F(HighlightingResultReporter, ReportRestIfChunkSizeNotReached)
 
 TEST_F(HighlightingResultReporter, ReportChunksWithoutRest)
 {
-    auto reporter = new ::HighlightingResultReporter(generateTokenInfos(4));
+    auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(4), 1);
-    reporter->setChunkSize(1);
-
-    auto future = reporter->start();
 
     ChunksReportedMonitor monitor(future);
     ASSERT_THAT(monitor.resultsReadyCounter(), 2L);
@@ -141,10 +126,7 @@ TEST_F(HighlightingResultReporter, ReportChunksWithoutRest)
 
 TEST_F(HighlightingResultReporter, ReportSingleChunkAndRest)
 {
-    auto reporter = new ::HighlightingResultReporter(generateTokenInfos(5));
+    auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(5), 2);
-    reporter->setChunkSize(2);
-
-    auto future = reporter->start();
 
     ChunksReportedMonitor monitor(future);
     ASSERT_THAT(monitor.resultsReadyCounter(), 2L);
@@ -159,10 +141,8 @@ TEST_F(HighlightingResultReporter, ReportCompleteLines)
         TokenInfoContainer(1, 2, 1, types),
         TokenInfoContainer(2, 1, 1, types),
     };
-    auto reporter = new ::HighlightingResultReporter(tokenInfos);
-    reporter->setChunkSize(1);
 
-    auto future = reporter->start();
+    auto future = ClangCodeModel::Internal::highlightResults(tokenInfos, 1);
 
     ChunksReportedMonitor monitor(future);
     ASSERT_THAT(monitor.resultsReadyCounter(), 2L);