fix: prevent potential QFutureWatcher race conditions

Mahlet-Inc · May 17, 2021 · d5a72d8 · d5a72d8
1 parent bb90767
commit d5a72d8
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 11 deletions.
diff --git a/src/hobbits-core/abstractpluginrunner.h b/src/hobbits-core/abstractpluginrunner.h
@@ -80,13 +80,16 @@ class Q_DECL_EXPORT AbstractPluginRunner : public AbstractPluginRunnerQObject
         m_actionWatcher = QSharedPointer<PluginActionWatcher<QSharedPointer<T>>>(
                 new PluginActionWatcher<QSharedPointer<T>>(
                         future,
-                        progress));
+                        progress,
+                        true));
 
         connect(m_actionWatcher->watcher(), SIGNAL(finished()), this, SLOT(postProcess()));
         connect(m_actionWatcher->progress().data(), &PluginActionProgress::progressPercentChanged, [this](int prog) {
             this->progress(m_id, prog);
         });
 
+        m_actionWatcher->setFutureInWatcher();
+
         return m_actionWatcher;
     }
 

diff --git a/src/hobbits-core/pluginactionwatcher.h b/src/hobbits-core/pluginactionwatcher.h
@@ -15,11 +15,15 @@ template<class T>
 class Q_DECL_EXPORT PluginActionWatcher
 {
 public:
-    explicit PluginActionWatcher(QFuture<T> future, QSharedPointer<PluginActionProgress> progress) :
+    explicit PluginActionWatcher(QFuture<T> future,
+                                    QSharedPointer<PluginActionProgress> progress,
+                                    bool delayWatcherSet = false) :
         m_progress(progress)
     {
         m_future = future;
-        m_futureWatcher.setFuture(future);
+        if (!delayWatcherSet) {
+            m_futureWatcher.setFuture(future);
+        }
     }
 
     T result()
@@ -37,6 +41,12 @@ class Q_DECL_EXPORT PluginActionWatcher
         return &m_futureWatcher;
     }
 
+    /// If the watcher set was delayed to allow for race-safe connections, it can be set with this method
+    void setFutureInWatcher()
+    {
+        m_futureWatcher.setFuture(m_future);
+    }
+
 private:
     QFuture<T> m_future;
     QFutureWatcher<T> m_futureWatcher;

diff --git a/src/hobbits-widgets/displaywidget.cpp b/src/hobbits-widgets/displaywidget.cpp
@@ -161,13 +161,16 @@ void DisplayWidget::performDisplayRender()
                                         this->size(),
                                         m_displayParameters,
                                         m_displayRenderProgress);
-        connect(&m_displayRenderWatcher, &QFutureWatcher<QImage>::finished, this, [this]() {
-            if (this->m_displayRenderWatcher.isFinished()) {
-                this->setDisplayResult(this->m_displayRenderWatcher.result());
+
+        m_displayRenderWatcher = QSharedPointer<QFutureWatcher<QSharedPointer<DisplayResult>>>(new QFutureWatcher<QSharedPointer<DisplayResult>>());
+
+        connect(m_displayRenderWatcher.data(), &QFutureWatcher<QImage>::finished, this, [this]() {
+            if (this->m_displayRenderWatcher->isFinished()) {
+                this->setDisplayResult(this->m_displayRenderWatcher->result());
             }
         });
 
-        m_displayRenderWatcher.setFuture(future);
+        m_displayRenderWatcher->setFuture(future);
     }
     else {
         m_displayResult = m_display->renderDisplay(this->size(), m_displayParameters, QSharedPointer<PluginActionProgress>());
@@ -196,8 +199,8 @@ void DisplayWidget::resetRendering()
         m_displayRenderProgress->setCancelled(true);
         disconnect(m_displayRenderProgress.data(), SIGNAL(progressUpdate(QString, QVariant)), this, SLOT(handleDisplayRenderPreview(QString, QVariant)));
     }
-    if (m_displayRenderWatcher.isRunning()) {
-        m_displayRenderWatcher.cancel();
+    if (!m_displayRenderWatcher.isNull() && m_displayRenderWatcher->isRunning()) {
+        m_displayRenderWatcher->cancel();
     }
 }
 

diff --git a/src/hobbits-widgets/displaywidget.h b/src/hobbits-widgets/displaywidget.h
@@ -65,7 +65,7 @@ private slots:
     bool m_repaintScheduled;
     QMutex m_mutex;
 
-    QFutureWatcher<QSharedPointer<DisplayResult>> m_displayRenderWatcher;
+    QSharedPointer<QFutureWatcher<QSharedPointer<DisplayResult>>> m_displayRenderWatcher;
     QSharedPointer<PluginActionProgress> m_displayRenderProgress;
 };
 

diff --git a/src/hobbits-widgets/previewscrollbar.cpp b/src/hobbits-widgets/previewscrollbar.cpp
@@ -51,7 +51,7 @@ void PreviewScrollBar::paintEvent(QPaintEvent *event)
                                                        container, progress);
 
             auto actionWatcher = QSharedPointer<PluginActionWatcher<QImage>>(
-                    new PluginActionWatcher<QImage>(future, progress));
+                    new PluginActionWatcher<QImage>(future, progress, true));
             m_renderWatchers.insert(containerPtr, actionWatcher);
 
             auto watcherRef = QWeakPointer<PluginActionWatcher<QImage>>(actionWatcher);
@@ -93,6 +93,9 @@ void PreviewScrollBar::paintEvent(QPaintEvent *event)
                 this->update();
             });
 
+            // set the future in the watcher now that the connections are done
+            actionWatcher->setFutureInWatcher();
+
             // clean out any deleted containers
             m_weakRefMap.insert(containerPtr, QWeakPointer<BitContainer>(container));
             for (quint64 key : m_weakRefMap.keys()) {