QtConcurrent: Fix for leak in QFuture

To avoid leaking when converting a QFuture<T> to a QFuture<void> we need
to have a separate ref. counter for QFuture<T>. When the last QFuture<T>
goes out of scope, we need to clean out the result data.

backported from qt/qtbase commit 731ba8ed08f80644b403556638c7f6229e678ebe
Original commit by Christian Strømme

Task-number: QTBUG-27224
Change-Id: I0c6b525cf241b5c559a1bab4e0066cd4de556ea8
Reviewed-by: Christian Stromme <christian.stromme@digia.com>

5 files changed, 111 insertions, 13 deletions

diff --git a/src/corelib/concurrent/qfutureinterface.cpp b/src/corelib/concurrent/qfutureinterface.cpp
index 9668e5d..6853a1a 100644
--- a/src/corelib/concurrent/qfutureinterface.cpp
+++ b/src/corelib/concurrent/qfutureinterface.cpp
@@ -419,6 +419,16 @@ bool QFutureInterfaceBase::referenceCountIsOne() const
     return d->refCount == 1;
 }
 
+bool QFutureInterfaceBase::refT() const
+{
+    return d->refCount.refT();
+}
+
+bool QFutureInterfaceBase::derefT() const
+{
+    return d->refCount.derefT();
+}
+
 QFutureInterfaceBasePrivate::QFutureInterfaceBasePrivate(QFutureInterfaceBase::State initialState)
     : refCount(1), m_progressValue(0), m_progressMinimum(0), m_progressMaximum(0),
       state(initialState), pendingResults(0),
diff --git a/src/corelib/concurrent/qfutureinterface.h b/src/corelib/concurrent/qfutureinterface.h
index 30bfbdb..dfe7941 100644
--- a/src/corelib/concurrent/qfutureinterface.h
+++ b/src/corelib/concurrent/qfutureinterface.h
@@ -132,6 +132,8 @@ public:
 
 protected:
     bool referenceCountIsOne() const;
+    bool refT() const;
+    bool derefT() const;
 public:
 
 #ifndef QFUTURE_TEST
@@ -150,13 +152,17 @@ class QFutureInterface : public QFutureInterfaceBase
 public:
     QFutureInterface(State initialState = NoState)
         : QFutureInterfaceBase(initialState)
-    { }
+    {
+        refT();
+    }
     QFutureInterface(const QFutureInterface &other)
         : QFutureInterfaceBase(other)
-    { }
+    {
+        refT();
+    }
     ~QFutureInterface()
     {
-        if (referenceCountIsOne())
+        if (!derefT())
             resultStore().clear();
     }
 
@@ -165,7 +171,8 @@ public:
 
     QFutureInterface &operator=(const QFutureInterface &other)
     {
-        if (referenceCountIsOne())
+        other.refT();
+        if (!derefT())
             resultStore().clear();
         QFutureInterfaceBase::operator=(other);
         return *this;
diff --git a/src/corelib/concurrent/qfutureinterface_p.h b/src/corelib/concurrent/qfutureinterface_p.h
index 407c926..8431b1a 100644
--- a/src/corelib/concurrent/qfutureinterface_p.h
+++ b/src/corelib/concurrent/qfutureinterface_p.h
@@ -129,7 +129,31 @@ class QFutureInterfaceBasePrivate
 public:
     QFutureInterfaceBasePrivate(QFutureInterfaceBase::State initialState);
 
-    QAtomicInt refCount;
+    // When the last QFuture<T> reference is removed, we need to make
+    // sure that data stored in the ResultStore is cleaned out.
+    // Since QFutureInterfaceBasePrivate can be shared between QFuture<T>
+    // and QFuture<void> objects, we use a separate ref. counter
+    // to keep track of QFuture<T> objects.
+    class RefCount
+    {
+    public:
+        inline RefCount(int r = 0, int rt = 0)
+            : m_refCount(r), m_refCountT(rt) {}
+        // Default ref counter for QFIBP
+        inline bool ref() { return m_refCount.ref(); }
+        inline bool deref() { return m_refCount.deref(); }
+        // Ref counter for type T
+        inline bool refT() { return m_refCountT.ref(); }
+        inline bool derefT() { return m_refCountT.deref(); }
+        inline operator int() const { return int(m_refCount); }
+        inline bool operator==(int value) const { return m_refCount == value; }
+
+    private:
+        QAtomicInt m_refCount;
+        QAtomicInt m_refCountT;
+    };
+
+    RefCount refCount;
     mutable QMutex m_mutex;
     QWaitCondition waitCondition;
     QList<QFutureCallOutInterface *> outputConnections;
diff --git a/tests/auto/qfuture/tst_qfuture.cpp b/tests/auto/qfuture/tst_qfuture.cpp
index f6ed5a9..0f1836c 100644
--- a/tests/auto/qfuture/tst_qfuture.cpp
+++ b/tests/auto/qfuture/tst_qfuture.cpp
@@ -1282,18 +1282,32 @@ void tst_QFuture::throttling()
 
 void tst_QFuture::voidConversions()
 {
-    QFutureInterface<int> iface;
-    iface.reportStarted();
+    {
+        QFutureInterface<int> iface;
+        iface.reportStarted();
 
-    QFuture<int> intFuture(&iface);
+        QFuture<int> intFuture(&iface);
+        int value = 10;
+        iface.reportFinished(&value);
 
-    int value = 10;
-    iface.reportFinished(&value);
+        QFuture<void> voidFuture(intFuture);
+        voidFuture = intFuture;
+
+        QVERIFY(voidFuture == intFuture);
+    }
 
-    QFuture<void> voidFuture(intFuture);
-    voidFuture = intFuture;
+    {
+        QFuture<void> voidFuture;
+        {
+            QFutureInterface<QList<int> > iface;
+            iface.reportStarted();
 
-    QVERIFY(voidFuture == intFuture);
+            QFuture<QList<int> > listFuture(&iface);
+            iface.reportResult(QList<int>() << 1 << 2 << 3);
+            voidFuture = listFuture;
+        }
+        QCOMPARE(voidFuture.resultCount(), 0);
+    }
 }
 
 
diff --git a/tests/auto/qtconcurrentmap/tst_qtconcurrentmap.cpp b/tests/auto/qtconcurrentmap/tst_qtconcurrentmap.cpp
index 43075f1..f8c4a36 100644
--- a/tests/auto/qtconcurrentmap/tst_qtconcurrentmap.cpp
+++ b/tests/auto/qtconcurrentmap/tst_qtconcurrentmap.cpp
@@ -43,6 +43,7 @@
 
 #include <qdebug.h>
 #include <QThread>
+#include <QMutex>
 
 #include <QtTest/QtTest>
 
@@ -76,6 +77,7 @@ private slots:
     void stlContainers();
     void qFutureAssignmentLeak();
     void stressTest();
+    void persistentResultTest();
 public slots:
     void throttling();
 };
@@ -2416,6 +2418,47 @@ void tst_QtConcurrentMap::stressTest()
     }
 }
 
+struct LockedCounter
+{
+    LockedCounter(QMutex *mutex, QAtomicInt *ai)
+        : mtx(mutex),
+          ref(ai) {}
+
+    typedef int result_type;
+    int operator()(int x)
+    {
+        QMutexLocker locker(mtx);
+        ref->ref();
+        return ++x;
+    }
+
+    QMutex *mtx;
+    QAtomicInt *ref;
+};
+
+// The Thread engine holds the last reference
+// to the QFuture, so this should not leak
+// or fail.
+void tst_QtConcurrentMap::persistentResultTest()
+{
+    QFuture<void> voidFuture;
+    QMutex mtx;
+    QAtomicInt ref;
+    LockedCounter lc(&mtx, &ref);
+    QList<int> list;
+    {
+        list << 1 << 2 << 3;
+        mtx.lock();
+        QFuture<int> future = QtConcurrent::mapped(list
+                                                   ,lc);
+        voidFuture = future;
+    }
+    QCOMPARE(int(ref), 0);
+    mtx.unlock(); // Unblock
+    voidFuture.waitForFinished();
+    QCOMPARE(int(ref), 3);
+}
+
 QTEST_MAIN(tst_QtConcurrentMap)
 
 #else