From 82b2735da40a8146100307afe628e775d96afcc2 Mon Sep 17 00:00:00 2001
From: Jarek Kobus <jaroslaw.kobus@qt.io>
Date: Tue, 8 Dec 2020 09:27:38 +0100
Subject: [PATCH] Don't leak memory when canceling Type Hierarchy

Add QFutureSynchronizer for collecting all running
futures (including those already canceled, but not finished yet).
It could happen, that we cancel the future, while
the associated task still needs some time in order
to catch the cancel request. When a d'tor of synchronizer
is run it cancels and waits for all pending futures
to be finished.

Add extra check for isCanceled() after a call to
updateDependencyTable(). In case the cancel was detected
inside updateDependencyTable(), we should return immediately.

Don't leak CppClass object inside handleLookupItemMatch()
when the task was canceled.

Amends: c400923308f5c4df4e8d96a04d12dc54322f7ade

Change-Id: I709e14a0c8b563d522a8e8c32b087e5f83310b24
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>
---
 src/plugins/cppeditor/cpptypehierarchy.cpp   | 22 ++++++++++++++------
 src/plugins/cppeditor/cpptypehierarchy.h     |  6 ++++--
 src/plugins/cpptools/cppelementevaluator.cpp |  6 ++++--
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/src/plugins/cppeditor/cpptypehierarchy.cpp b/src/plugins/cppeditor/cpptypehierarchy.cpp
index 34025a59bd8..0178ab60557 100644
--- a/src/plugins/cppeditor/cpptypehierarchy.cpp
+++ b/src/plugins/cppeditor/cpptypehierarchy.cpp
@@ -130,15 +130,21 @@ CppTypeHierarchyWidget::CppTypeHierarchyWidget()
 
     connect(CppEditorPlugin::instance(), &CppEditorPlugin::typeHierarchyRequested,
             this, &CppTypeHierarchyWidget::perform);
-    connect(&m_futureWatcher, &QFutureWatcher<QSharedPointer<CppElement>>::finished,
+    connect(&m_futureWatcher, &QFutureWatcher<void>::finished,
             this, &CppTypeHierarchyWidget::displayHierarchy);
+
+    m_synchronizer.setCancelOnWait(true);
 }
 
-CppTypeHierarchyWidget::~CppTypeHierarchyWidget()
+void CppTypeHierarchyWidget::updateSynchronizer()
 {
-    if (m_future.isRunning()) {
-        m_future.cancel();
-        m_future.waitForFinished();
+    const QList<QFuture<void>> futures = m_synchronizer.futures();
+
+    m_synchronizer.clearFutures();
+
+    for (const QFuture<void> &future : futures) {
+        if (!future.isFinished())
+            m_synchronizer.addFuture(future);
     }
 }
 
@@ -147,6 +153,8 @@ void CppTypeHierarchyWidget::perform()
     if (m_future.isRunning())
         m_future.cancel();
 
+    updateSynchronizer();
+
     auto editor = qobject_cast<CppEditor *>(Core::EditorManager::currentEditor());
     if (!editor) {
         showNoTypeHierarchyLabel();
@@ -165,13 +173,15 @@ void CppTypeHierarchyWidget::perform()
     evaluator.setLookupBaseClasses(true);
     evaluator.setLookupDerivedClasses(true);
     m_future = evaluator.asyncExecute();
-    m_futureWatcher.setFuture(m_future);
+    m_futureWatcher.setFuture(QFuture<void>(m_future));
+    m_synchronizer.addFuture(QFuture<void>(m_future));
 
     Core::ProgressManager::addTask(m_future, tr("Evaluating Type Hierarchy"), "TypeHierarchy");
 }
 
 void CppTypeHierarchyWidget::displayHierarchy()
 {
+    updateSynchronizer();
     hideProgress();
     clearTypeHierarchy();
 
diff --git a/src/plugins/cppeditor/cpptypehierarchy.h b/src/plugins/cppeditor/cpptypehierarchy.h
index 83b1a4d99f3..94414db895c 100644
--- a/src/plugins/cppeditor/cpptypehierarchy.h
+++ b/src/plugins/cppeditor/cpptypehierarchy.h
@@ -29,6 +29,7 @@
 
 #include <QFuture>
 #include <QFutureWatcher>
+#include <QFutureSynchronizer>
 #include <QList>
 #include <QSharedPointer>
 #include <QStackedWidget>
@@ -75,7 +76,6 @@ class CppTypeHierarchyWidget : public QWidget
     Q_OBJECT
 public:
     CppTypeHierarchyWidget();
-    ~CppTypeHierarchyWidget() override;
 
     void perform();
 
@@ -92,6 +92,7 @@ private:
     void hideProgress();
     void clearTypeHierarchy();
     void onItemActivated(const QModelIndex &index);
+    void updateSynchronizer();
 
     CppEditorWidget *m_cppEditor = nullptr;
     Utils::NavigationTreeView *m_treeView = nullptr;
@@ -102,7 +103,8 @@ private:
     TextEditor::TextEditorLinkLabel *m_inspectedClass = nullptr;
     QLabel *m_infoLabel = nullptr;
     QFuture<QSharedPointer<CppTools::CppElement>> m_future;
-    QFutureWatcher<QSharedPointer<CppTools::CppElement>> m_futureWatcher;
+    QFutureWatcher<void> m_futureWatcher;
+    QFutureSynchronizer<void> m_synchronizer;
     Utils::ProgressIndicator *m_progressIndicator = nullptr;
 };
 
diff --git a/src/plugins/cpptools/cppelementevaluator.cpp b/src/plugins/cpptools/cppelementevaluator.cpp
index 208f75433dc..0e739ddeab2 100644
--- a/src/plugins/cpptools/cppelementevaluator.cpp
+++ b/src/plugins/cpptools/cppelementevaluator.cpp
@@ -214,6 +214,8 @@ void CppClass::lookupDerived(QFutureInterfaceBase &futureInterface,
     using Data = QPair<CppClass*, CppTools::TypeHierarchy>;
 
     snapshot.updateDependencyTable(futureInterface);
+    if (futureInterface.isCanceled())
+        return;
     CppTools::TypeHierarchyBuilder builder(declaration, snapshot);
     const CppTools::TypeHierarchy &completeHierarchy = builder.buildDerivedTypeHierarchy(futureInterface);
 
@@ -531,7 +533,7 @@ static void handleLookupItemMatch(QFutureInterface<QSharedPointer<CppElement>> &
 
             if (futureInterface.isCanceled())
                 return;
-            auto cppClass = new CppClass(declaration);
+            QSharedPointer<CppClass> cppClass(new CppClass(declaration));
             if (lookupBaseClasses)
                 cppClass->lookupBases(futureInterface, declaration, contextToUse);
             if (futureInterface.isCanceled())
@@ -540,7 +542,7 @@ static void handleLookupItemMatch(QFutureInterface<QSharedPointer<CppElement>> &
                 cppClass->lookupDerived(futureInterface, declaration, snapshot);
             if (futureInterface.isCanceled())
                 return;
-            element = QSharedPointer<CppElement>(cppClass);
+            element = cppClass;
         } else if (Enum *enumDecl = declaration->asEnum()) {
             element = QSharedPointer<CppElement>(new CppEnum(enumDecl));
         } else if (auto enumerator = dynamic_cast<EnumeratorDeclaration *>(declaration)) {