Fix segfault when using qfuture continuations with move only types

When using move-only types, continuations args are set using takeResult
function, which has the side effect of invalidating the QFutureInterface
associated with the promise/futures by:

1. setting isValid to false
2. setting the state to NoState

And when the promise is destroyed, it tries to run the continuations if
`finished()` is not called, which is done by checking the Finished bit
in the state. But since the continuation has been run before, and the
state has been set to NoState it tries to run the continuation again
causing a segfault. Multiple solutions come in mind:

1. don't run the continuation if the state is NoState, but this would
   break the case when an empty promise is destroyed
2. check inside the continuation if it has been run before, and if so
   don't run it again, but this seems hacky since we don't want the
   continuation to be run twice, and it should break if it did.
3. when invalidating the promise leave the state as is, and change
   isValid only to false, which changes the current behavior, but is
   still compatible with the documentation which states only that
   isValid will return false if takeResult is called

I chose option 3

I also extended some tests to test for move only types, and added a test
that continuations run when a promise is finished. This simple case
would segfault before with move only types.

Fixes: QTBUG-112513
Pick-to: 6.5 6.6
Change-Id: Ie225ac4fdf618e4edfb0efd663d6c7fd6b916dbd
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>

1 files changed, 77 insertions, 11 deletions

diff --git a/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp b/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp
index 0b0e6c321f..fc1ace10cb 100644
--- a/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp
+++ b/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp
@@ -44,6 +44,7 @@ private slots:
     void cancelWhenReassigned();
     void cancelWhenDestroyedWithoutStarting();
     void cancelWhenDestroyedRunsContinuations();
+    void continuationsRunWhenFinished();
     void finishWhenSwapped();
     void cancelWhenMoved();
     void waitUntilResumed();
@@ -290,6 +291,10 @@ void tst_QPromise::setException()
                        std::make_exception_ptr(TestException()));
     RUN_TEST_FUNC(testExceptionCaught, QPromise<int>(),
                        std::make_exception_ptr(TestException()));
+    RUN_TEST_FUNC(testExceptionCaught, QPromise<CopyOnlyType>(),
+                       std::make_exception_ptr(TestException()));
+    RUN_TEST_FUNC(testExceptionCaught, QPromise<MoveOnlyType>(),
+                       std::make_exception_ptr(TestException()));
 }
 #endif
 
@@ -303,6 +308,8 @@ void tst_QPromise::cancel()
 
     testCancel(QPromise<void>());
     testCancel(QPromise<int>());
+    testCancel(QPromise<CopyOnlyType>());
+    testCancel(QPromise<MoveOnlyType>());
 }
 
 void tst_QPromise::progress()
@@ -330,6 +337,8 @@ void tst_QPromise::progress()
 
     RUN_TEST_FUNC(testProgress, QPromise<void>());
     RUN_TEST_FUNC(testProgress, QPromise<int>());
+    RUN_TEST_FUNC(testProgress, QPromise<CopyOnlyType>());
+    RUN_TEST_FUNC(testProgress, QPromise<MoveOnlyType>());
 }
 
 void tst_QPromise::addInThread()
@@ -455,6 +464,8 @@ void tst_QPromise::doNotCancelWhenFinished()
     RUN_TEST_FUNC(testFinishedPromise, QPromise<void>());
     RUN_TEST_FUNC(testFinishedPromise, QPromise<int>());
     RUN_TEST_FUNC(testFinishedPromise, QPromise<QString>());
+    RUN_TEST_FUNC(testFinishedPromise, QPromise<CopyOnlyType>());
+    RUN_TEST_FUNC(testFinishedPromise, QPromise<MoveOnlyType>());
 #endif
 }
 
@@ -516,11 +527,12 @@ void tst_QPromise::cancelWhenReassigned()
 #endif
 }
 
-void tst_QPromise::cancelWhenDestroyedWithoutStarting()
+template <typename T>
+static inline void testCancelWhenDestroyedWithoutStarting()
 {
-    QFuture<void> future;
+    QFuture<T> future;
     {
-        QPromise<void> promise;
+        QPromise<T> promise;
         future = promise.future();
     }
     future.waitForFinished();
@@ -529,17 +541,26 @@ void tst_QPromise::cancelWhenDestroyedWithoutStarting()
     QVERIFY(future.isFinished());
 }
 
-void tst_QPromise::cancelWhenDestroyedRunsContinuations()
+void tst_QPromise::cancelWhenDestroyedWithoutStarting()
 {
-    QFuture<void> future;
+    testCancelWhenDestroyedWithoutStarting<void>();
+    testCancelWhenDestroyedWithoutStarting<int>();
+    testCancelWhenDestroyedWithoutStarting<CopyOnlyType>();
+    testCancelWhenDestroyedWithoutStarting<MoveOnlyType>();
+}
+
+template <typename T>
+static inline void testCancelWhenDestroyedRunsContinuations()
+{
+    QFuture<T> future;
     bool onCanceledCalled = false;
     bool thenCalled = false;
     {
-        QPromise<void> promise;
+        QPromise<T> promise;
         future = promise.future();
-        future.then([&] {
+        future.then([&] (auto&&) {
             thenCalled = true;
-        }).onCanceled([&] {
+        }).onCanceled([&] () {
             onCanceledCalled = true;
         });
     }
@@ -548,6 +569,42 @@ void tst_QPromise::cancelWhenDestroyedRunsContinuations()
     QVERIFY(onCanceledCalled);
 }
 
+void tst_QPromise::cancelWhenDestroyedRunsContinuations()
+{
+    testCancelWhenDestroyedRunsContinuations<void>();
+    testCancelWhenDestroyedRunsContinuations<int>();
+    testCancelWhenDestroyedRunsContinuations<CopyOnlyType>();
+    testCancelWhenDestroyedRunsContinuations<MoveOnlyType>();
+}
+
+template <typename T>
+static inline void testContinuationsRunWhenFinished()
+{
+    QPromise<T> promise;
+    QFuture<T> future = promise.future();
+
+    bool thenCalled = false;
+    future.then([&] (auto&&) {
+        thenCalled = true;
+    });
+
+    promise.start();
+    if constexpr (!std::is_void_v<T>) {
+        promise.addResult(T{});
+    }
+    promise.finish();
+
+    QVERIFY(thenCalled);
+}
+
+void tst_QPromise::continuationsRunWhenFinished()
+{
+    testContinuationsRunWhenFinished<void>();
+    testContinuationsRunWhenFinished<int>();
+    testContinuationsRunWhenFinished<CopyOnlyType>();
+    testContinuationsRunWhenFinished<MoveOnlyType>();
+}
+
 void tst_QPromise::finishWhenSwapped()
 {
 #if !QT_CONFIG(cxx11_future)
@@ -590,16 +647,17 @@ void tst_QPromise::finishWhenSwapped()
 #endif
 }
 
-void tst_QPromise::cancelWhenMoved()
+template <typename T>
+void testCancelWhenMoved()
 {
 #if !QT_CONFIG(cxx11_future)
     QSKIP("This test requires QThread::create");
 #else
-    QPromise<int> promise1;
+    QPromise<T> promise1;
     auto f1 = promise1.future();
     promise1.start();
 
-    QPromise<int> promise2;
+    QPromise<T> promise2;
     auto f2 = promise2.future();
     promise2.start();
 
@@ -623,6 +681,14 @@ void tst_QPromise::cancelWhenMoved()
 #endif
 }
 
+void tst_QPromise::cancelWhenMoved()
+{
+    testCancelWhenMoved<void>();
+    testCancelWhenMoved<int>();
+    testCancelWhenMoved<CopyOnlyType>();
+    testCancelWhenMoved<MoveOnlyType>();
+}
+
 void tst_QPromise::waitUntilResumed()
 {
 #if !QT_CONFIG(cxx11_future)