QFutureSynchronizer: fix aliasing problem in setFuture()

When setFuture() was handed an element of m_futures, it would hold the
reference to past the clear(), which invalidates said reference.

Fix by taking the future by value instead of by cref.

While append() is not affected, as QList::append() already guards
against aliasing, do the same change there, both for consistency as
well as to optimize the common case of passing rvalues. It also means
we can use the rvalue overload of QList::append(), skipping the alias
analysis in the lvalue QList::append().

[ChangeLog][QtConcurrent][QFutureSynchronizer] Fixed a crash in
setFuture() if the argument was already a member of
QFutureSynchronizer::futures().

Pick-to: 6.6 6.5 6.2
Change-Id: Ic0b212b9f265a746df9a6beb6272a5415d131442
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

src/corelib/thread/qfuturesynchronizer.h

@@ -18,21 +18,21 @@ class QFutureSynchronizer
 
 public:
     QFutureSynchronizer() : m_cancelOnWait(false) { }
-    explicit QFutureSynchronizer(const QFuture<T> &future)
+    explicit QFutureSynchronizer(QFuture<T> future)
         : m_cancelOnWait(false)
-    { addFuture(future); }
+    { addFuture(std::move(future)); }
     ~QFutureSynchronizer()  { waitForFinished(); }
 
-    void setFuture(const QFuture<T> &future)
+    void setFuture(QFuture<T> future)
     {
         waitForFinished();
         m_futures.clear();
-        addFuture(future);
+        addFuture(std::move(future));
     }
 
-    void addFuture(const QFuture<T> &future)
+    void addFuture(QFuture<T> future)
     {
-        m_futures.append(future);
+        m_futures.append(std::move(future));
     }
 
     void waitForFinished()

src/corelib/thread/qfuturesynchronizer.qdoc

@@ -38,7 +38,7 @@
 */
 
 /*!
-    \fn template <typename T> QFutureSynchronizer<T>::QFutureSynchronizer(const QFuture<T> &future)
+    \fn template <typename T> QFutureSynchronizer<T>::QFutureSynchronizer(QFuture<T> future)
 
     Constructs a QFutureSynchronizer and begins watching \a future by calling
     addFuture().
@@ -56,7 +56,7 @@
 */
 
 /*!
-    \fn template <typename T> void QFutureSynchronizer<T>::setFuture(const QFuture<T> &future)
+    \fn template <typename T> void QFutureSynchronizer<T>::setFuture(QFuture<T> future)
 
     Sets \a future to be the only future managed by this QFutureSynchronizer.
     This is a convenience function that calls waitForFinished(),
@@ -66,7 +66,7 @@
 */
 
 /*!
-    \fn template <typename T> void QFutureSynchronizer<T>::addFuture(const QFuture<T> &future)
+    \fn template <typename T> void QFutureSynchronizer<T>::addFuture(QFuture<T> future)
 
     Adds \a future to the list of managed futures.
 

tests/auto/corelib/thread/qfuturesynchronizer/tst_qfuturesynchronizer.cpp

@@ -13,6 +13,7 @@ class tst_QFutureSynchronizer : public QObject
 
 private Q_SLOTS:
     void construction();
+    void setFutureAliasingExistingMember();
     void addFuture();
     void cancelOnWait();
     void clearFutures();
@@ -33,6 +34,28 @@ void tst_QFutureSynchronizer::construction()
     QCOMPARE(synchronizerWithFuture.futures().size(), 1);
 }
 
+void tst_QFutureSynchronizer::setFutureAliasingExistingMember()
+{
+    //
+    // GIVEN: a QFutureSynchronizer with one QFuture:
+    //
+    QFutureSynchronizer synchronizer(QtFuture::makeReadyValueFuture(42));
+
+    //
+    // WHEN: calling setFuture() with an alias of the QFuture already in `synchronizer`:
+    //
+    for (int i = 0; i < 2; ++i) {
+        const auto &f = synchronizer.futures().constFirst();
+        synchronizer.setFuture(f);
+    }
+
+    //
+    // THEN: it didn't crash
+    //
+    QCOMPARE(synchronizer.futures().size(), 1);
+    QCOMPARE(synchronizer.futures().constFirst().result(), 42);
+}
+
 void tst_QFutureSynchronizer::addFuture()
 {
     QFutureSynchronizer<void> synchronizer;