From 320a1b81c958a6f465103bb603767f2edb1c3f6c Mon Sep 17 00:00:00 2001
From: Jonas Kvinge <jonas@jkvinge.net>
Date: Wed, 14 Jul 2021 00:13:42 +0200
Subject: [PATCH] Fix incorrect use of QFutureWatcher

To avoid a race condition, it is important to call setFuture() after doing the connections.

See: https://doc.qt.io/qt-6/qfuturewatcher.html
---
 ext/libclementine-common/core/closure.h |  4 ++--
 src/musicbrainz/tagfetcher.cpp          |  2 +-
 tests/concurrentrun_test.cpp            | 16 ++++++++--------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/ext/libclementine-common/core/closure.h b/ext/libclementine-common/core/closure.h
index fafc69abc..b8efba093 100644
--- a/ext/libclementine-common/core/closure.h
+++ b/ext/libclementine-common/core/closure.h
@@ -202,8 +202,8 @@ template <typename T, typename... Args>
 _detail::ClosureBase* NewClosure(QFuture<T> future, QObject* receiver,
                                  const char* slot, const Args&... args) {
   QFutureWatcher<T>* watcher = new QFutureWatcher<T>;
-  watcher->setFuture(future);
   QObject::connect(watcher, SIGNAL(finished()), watcher, SLOT(deleteLater()));
+  watcher->setFuture(future);
   return NewClosure(watcher, SIGNAL(finished()), receiver, slot, args...);
 }
 
@@ -211,8 +211,8 @@ template <typename T, typename F, typename... Args>
 _detail::ClosureBase* NewClosure(QFuture<T> future, const F& callback,
                                  const Args&... args) {
   QFutureWatcher<T>* watcher = new QFutureWatcher<T>;
-  watcher->setFuture(future);
   QObject::connect(watcher, SIGNAL(finished()), watcher, SLOT(deleteLater()));
+  watcher->setFuture(future);
   return NewClosure(watcher, SIGNAL(finished()), callback, args...);
 }
 
diff --git a/src/musicbrainz/tagfetcher.cpp b/src/musicbrainz/tagfetcher.cpp
index b7ddf9a7b..160e4421c 100644
--- a/src/musicbrainz/tagfetcher.cpp
+++ b/src/musicbrainz/tagfetcher.cpp
@@ -50,9 +50,9 @@ void TagFetcher::StartFetch(const SongList& songs) {
 
   QFuture<QString> future = QtConcurrent::mapped(songs_, GetFingerprint);
   fingerprint_watcher_ = new QFutureWatcher<QString>(this);
-  fingerprint_watcher_->setFuture(future);
   connect(fingerprint_watcher_, SIGNAL(resultReadyAt(int)),
           SLOT(FingerprintFound(int)));
+  fingerprint_watcher_->setFuture(future);
 
   for (const Song& song : songs) {
     emit Progress(song, tr("Fingerprinting song"));
diff --git a/tests/concurrentrun_test.cpp b/tests/concurrentrun_test.cpp
index 09eb3f21f..44ad530f4 100644
--- a/tests/concurrentrun_test.cpp
+++ b/tests/concurrentrun_test.cpp
@@ -17,9 +17,9 @@ TEST(ConcurrentRunTest, ConcurrentRun0StartAndWait) {
   QThreadPool threadpool;
   QFuture<int> future = ConcurrentRun::Run<int>(&threadpool, &f);
   QFutureWatcher<int> watcher;
-  watcher.setFuture(future);
   QEventLoop loop;
   QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
+  watcher.setFuture(future);
   loop.exec();
   EXPECT_EQ(1337, watcher.result());
 }
@@ -33,9 +33,9 @@ TEST(ConcurrentRunTest, ConcurrentRun1StartAndWait) {
   int i = 1336;
   QFuture<int> future = ConcurrentRun::Run<int, int>(&threadpool, &g, i);
   QFutureWatcher<int> watcher;
-  watcher.setFuture(future);
   QEventLoop loop;
   QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
+  watcher.setFuture(future);
   loop.exec();
   EXPECT_EQ(1337, watcher.result());
 }
@@ -50,9 +50,9 @@ TEST(ConcurrentRunTest, ConcurrentRun2StartAndWait) {
   QThreadPool threadpool;
   QFuture<int> future = ConcurrentRun::Run<int, int, int>(&threadpool, &max, i, j);
   QFutureWatcher<int> watcher;
-  watcher.setFuture(future);
   QEventLoop loop;
   QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
+  watcher.setFuture(future);
   loop.exec();
   EXPECT_EQ(42, watcher.result());
 }
@@ -68,9 +68,9 @@ TEST(ConcurrentRunTest, ConcurrentRun3StartAndWait) {
   QThreadPool threadpool;
   QFuture<int> future = ConcurrentRun::Run<int, int, int, int>(&threadpool, &sum, i, j, k);
   QFutureWatcher<int> watcher;
-  watcher.setFuture(future);
   QEventLoop loop;
   QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
+  watcher.setFuture(future);
   loop.exec();
   EXPECT_EQ(102, watcher.result());
 }
@@ -95,9 +95,9 @@ TEST(ConcurrentRunTest, ConcurrentRunVoidFunction1Start) {
   int n = 10;
   QFuture<void> future = ConcurrentRun::Run<void, int*>(&threadpool, &aFunction, &n);
   QFutureWatcher<void> watcher;
-  watcher.setFuture(future);
   QEventLoop loop;
   QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
+  watcher.setFuture(future);
   loop.exec();
   EXPECT_EQ(1337, n);
 }
@@ -108,9 +108,9 @@ TEST(ConcurrentRunTest, ConcurrentRunVoidFunction2Start) {
   int n = 10, m = 11;
   QFuture<void> future = ConcurrentRun::Run<void, int*, int*>(&threadpool, &bFunction, &n, &m);
   QFutureWatcher<void> watcher;
-  watcher.setFuture(future);
   QEventLoop loop;
   QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
+  watcher.setFuture(future);
   loop.exec();
   EXPECT_EQ(1337, n);
   EXPECT_EQ(1338, m);
@@ -122,9 +122,9 @@ TEST(ConcurrentRunTest, ConcurrentRunVoidFunction3Start) {
   int n = 10, m = 11, o = 12;
   QFuture<void> future = ConcurrentRun::Run<void, int*, int*, int*>(&threadpool, &cFunction, &n, &m, &o);
   QFutureWatcher<void> watcher;
-  watcher.setFuture(future);
   QEventLoop loop;
   QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
+  watcher.setFuture(future);
   loop.exec();
   EXPECT_EQ(1337, n);
   EXPECT_EQ(1338, m);
@@ -145,9 +145,9 @@ TEST(ConcurrentRunTest, ConcurrentRunVoidBindFunctionStart) {
   int nb = 10;
   QFuture<void> future = ConcurrentRun::Run<void>(&threadpool, std::bind(&A::f, &a, &nb));
   QFutureWatcher<void> watcher;
-  watcher.setFuture(future);
   QEventLoop loop;
   QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
+  watcher.setFuture(future);
   loop.exec();
   EXPECT_EQ(11, nb);
 }