QFuture: fix handling of cancelled continuation chain

To support cancellation of continuations attached via the parent future,
for each future returned by a continuation we store a pointer to its
parent (i.e. future the continuation is attached to). Later, before
executing a continuation, we go through chain of parents and check if
any of them is cancelled. However, if one of the parents is destroyed
while the chain is executing, the next continuations' parent pointers
will become invalid. So storing the parent pointers isn't safe.

This commit changes the logic of handling the cancelled continuation
chain in the following way:

- Instead of storing a parent pointer in the continuation future's data,
  we do the opposite: we store a pointer to continuation's future in the
  parent.
- When a future is cancelled, we mark all continuation futures in the
  chain with a flag indicating that the chain is cancelled.
- To guarantee that the pointers to continuation future's data don't
  become invalid, we clean the continuation (that stores a copy of its
  future's data and keeps it alive) only when the associated promise
  is destructed, instead of cleaning it after the continuation is run.

Fixes: QTBUG-105182
Fixes: QTBUG-106083
Change-Id: I48afa98152672c0fc737112be4ca3b1b42f6ed30
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
(cherry picked from commit 24dedaeaa1a94bfe9ade2da2a2c9aa112241b07a)

4 files changed, 103 insertions, 20 deletions

diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp
index 58b1bf17c8..7738865d6d 100644
--- a/src/corelib/thread/qfutureinterface.cpp
+++ b/src/corelib/thread/qfutureinterface.cpp
@@ -139,6 +139,13 @@ void QFutureInterfaceBase::cancel(QFutureInterfaceBase::CancelMode mode)
         break;
     }
 
+    // Cancel the continuations chain
+    QFutureInterfaceBasePrivate *next = d->continuationData;
+    while (next) {
+        next->continuationState = QFutureInterfaceBasePrivate::Canceled;
+        next = next->continuationData;
+    }
+
     d->waitCondition.wakeAll();
     d->pausedWaitCondition.wakeAll();
 
@@ -850,16 +857,19 @@ void QFutureInterfaceBase::setContinuation(std::function<void(const QFutureInter
 {
     QMutexLocker lock(&d->continuationMutex);
 
-    if (continuationFutureData)
-        continuationFutureData->parentData = d;
-
     // If the state is ready, run continuation immediately,
     // otherwise save it for later.
     if (isFinished()) {
         lock.unlock();
         func(*this);
-    } else {
+        lock.relock();
+    }
+    // Unless the continuation has been cleaned earlier, we have to
+    // store the move-only continuation, to guarantee that the associated
+    // future's data stays alive.
+    if (d->continuationState != QFutureInterfaceBasePrivate::Cleaned) {
         d->continuation = std::move(func);
+        d->continuationData = continuationFutureData;
     }
 }
 
@@ -873,34 +883,32 @@ void QFutureInterfaceBase::cleanContinuation()
     // copies of this, so that the allocated memory can be freed.
     QMutexLocker lock(&d->continuationMutex);
     d->continuation = nullptr;
+    d->continuationState = QFutureInterfaceBasePrivate::Cleaned;
 }
 
 void QFutureInterfaceBase::runContinuation() const
 {
     QMutexLocker lock(&d->continuationMutex);
     if (d->continuation) {
-        auto fn = std::exchange(d->continuation, nullptr);
+        // Save the continuation in a local function, to avoid calling
+        // a null std::function below, in case cleanContinuation() is
+        // called from some other thread right after unlock() below.
+        auto fn = std::move(d->continuation);
         lock.unlock();
         fn(*this);
+
+        lock.relock();
+        // Unless the continuation has been cleaned earlier, we have to
+        // store the move-only continuation, to guarantee that the associated
+        // future's data stays alive.
+        if (d->continuationState != QFutureInterfaceBasePrivate::Cleaned)
+            d->continuation = std::move(fn);
     }
 }
 
 bool QFutureInterfaceBase::isChainCanceled() const
 {
-    if (isCanceled())
-        return true;
-
-    auto parent = d->parentData;
-    while (parent) {
-        // If the future is in Canceled state because it had an exception, we want to
-        // continue checking the chain of parents for cancellation, otherwise if the exception
-        // is handeled inside the chain, it won't be interrupted even though cancellation has
-        // been requested.
-        if ((parent->state.loadRelaxed() & Canceled) && !parent->hasException)
-            return true;
-        parent = parent->parentData;
-    }
-    return false;
+    return isCanceled() || d->continuationState == QFutureInterfaceBasePrivate::Canceled;
 }
 
 void QFutureInterfaceBase::setLaunchAsync(bool value)
diff --git a/src/corelib/thread/qfutureinterface_p.h b/src/corelib/thread/qfutureinterface_p.h
index 24902cbc9a..4e5ca0ae77 100644
--- a/src/corelib/thread/qfutureinterface_p.h
+++ b/src/corelib/thread/qfutureinterface_p.h
@@ -190,7 +190,10 @@ public:
     QThreadPool *m_pool = nullptr;
     // Wrapper for continuation
     std::function<void(const QFutureInterfaceBase &)> continuation;
-    QFutureInterfaceBasePrivate *parentData = nullptr;
+    QFutureInterfaceBasePrivate *continuationData = nullptr;
+
+    enum ContinuationState : quint8 { Default, Canceled, Cleaned };
+    std::atomic<ContinuationState> continuationState { Default };
 
     RefCount refCount = 1;
     QAtomicInt state; // reads and writes can happen unprotected, both must be atomic
diff --git a/src/corelib/thread/qpromise.h b/src/corelib/thread/qpromise.h
index a9d5f417a3..8041a5cc42 100644
--- a/src/corelib/thread/qpromise.h
+++ b/src/corelib/thread/qpromise.h
@@ -77,6 +77,7 @@ public:
             d.cancelAndFinish(); // cancel and finalize the state
             d.runContinuation();
         }
+        d.cleanContinuation();
     }
 
     // Core QPromise APIs
diff --git a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp
index a84b11ee68..e0afe7d38e 100644
--- a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp
+++ b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp
@@ -2258,6 +2258,26 @@ void tst_QFuture::then()
         QVERIFY(threadId1 != QThread::currentThreadId());
         QVERIFY(threadId2 != QThread::currentThreadId());
     }
+
+    // QTBUG-106083 & QTBUG-105182
+    {
+        QThread thread;
+        thread.start();
+
+        QObject context;
+        context.moveToThread(&thread);
+
+        auto future = QtConcurrent::run([] {
+            return 42;
+        }).then([] (int result) {
+            return result + 1;
+        }).then(&context, [] (int result) {
+            return result + 1;
+        });
+        QCOMPARE(future.result(), 44);
+        thread.quit();
+        thread.wait();
+    }
 }
 
 template<class Type, class Callable>
@@ -3134,6 +3154,57 @@ void tst_QFuture::cancelContinuations()
         QCOMPARE(checkpoint, 3);
     }
 #endif // QT_NO_EXCEPTIONS
+
+    // Check notifications from QFutureWatcher
+    {
+        QPromise<void> p;
+        auto f = p.future();
+
+        auto f1 = f.then([] {});
+        auto f2 = f1.then([] {});
+
+        QFutureWatcher<void> watcher1, watcher2;
+        int state = 0;
+        QObject::connect(&watcher1, &QFutureWatcher<void>::started, [&] {
+            QCOMPARE(state, 0);
+            ++state;
+        });
+        QObject::connect(&watcher1, &QFutureWatcher<void>::canceled, [&] {
+            QCOMPARE(state, 1);
+            ++state;
+        });
+        QObject::connect(&watcher1, &QFutureWatcher<void>::finished, [&] {
+            QCOMPARE(state, 2);
+            ++state;
+        });
+        QObject::connect(&watcher2, &QFutureWatcher<void>::started, [&] {
+            QCOMPARE(state, 3);
+            ++state;
+        });
+        QObject::connect(&watcher2, &QFutureWatcher<void>::canceled, [&] {
+            QCOMPARE(state, 4);
+            ++state;
+        });
+        QObject::connect(&watcher2, &QFutureWatcher<int>::finished, [&] {
+            QCOMPARE(state, 5);
+            ++state;
+        });
+
+        watcher1.setFuture(f1);
+        watcher2.setFuture(f2);
+
+        p.start();
+        f.cancel();
+        p.finish();
+
+        qApp->processEvents();
+
+        QCOMPARE(state, 6);
+        QVERIFY(watcher1.isFinished());
+        QVERIFY(watcher1.isCanceled());
+        QVERIFY(watcher2.isFinished());
+        QVERIFY(watcher2.isCanceled());
+    }
 }
 
 void tst_QFuture::continuationsWithContext()