Fix memory leak and crash with transform animators.

Every time initialize() was called, we would increment the ref on
an item. However, initialize is called every time the job is started,
so the ref would increase and only decrease once, leading to a leaked
helper. Change it to only increment the first time.

A different problem was that when an item was destroyed, we could run
the risk of the QQuickTransformAnimatorJob destructor being called
with the helper's item being null. This would lead to the helper not
being removed from the cache and a dangling helper would remain in the
transforms cache. Now change it so that when a target is destroyed, we
explicitly destroy the helper as well (as no animation can happen then
anyway) and reset all pointers in the job.

Change-Id: I1ce76db134bbc1871d32f1224ba5b68a4a4eeafa
Reviewed-by: Michael Brasser <michael.brasser@live.com>

4 files changed, 95 insertions, 7 deletions

diff --git a/src/quick/util/qquickanimatorcontroller.cpp b/src/quick/util/qquickanimatorcontroller.cpp
index 42b8840bf4..e009de205c 100644
--- a/src/quick/util/qquickanimatorcontroller.cpp
+++ b/src/quick/util/qquickanimatorcontroller.cpp
@@ -184,8 +184,6 @@ void QQuickAnimatorController::beforeNodeSync()
         m_nodesAreInvalid = false;
     }
 
-
-
     foreach (QQuickAnimatorJob *job, m_activeLeafAnimations) {
         if (!job->target())
             continue;
@@ -197,9 +195,9 @@ void QQuickAnimatorController::beforeNodeSync()
         }
     }
     foreach (QQuickItem *wiped, m_deletedSinceLastFrame) {
-        QQuickTransformAnimatorJob::Helper *helper = m_transforms.value(wiped);
-        if (helper)
-            helper->item = 0;
+        QQuickTransformAnimatorJob::Helper *helper = m_transforms.take(wiped);
+        // Helper will now already have been reset in all animators referencing it.
+        delete helper;
     }
 
     m_deletedSinceLastFrame.clear();
diff --git a/src/quick/util/qquickanimatorjob.cpp b/src/quick/util/qquickanimatorjob.cpp
index 3725e22397..fdbffd4709 100644
--- a/src/quick/util/qquickanimatorjob.cpp
+++ b/src/quick/util/qquickanimatorjob.cpp
@@ -256,6 +256,10 @@ QQuickTransformAnimatorJob::QQuickTransformAnimatorJob()
 QQuickTransformAnimatorJob::~QQuickTransformAnimatorJob()
 {
     if (m_helper && --m_helper->ref == 0) {
+        // The only condition for not having a controller is when target was
+        // destroyed, in which case we have neither m_helper nor m_contorller.
+        Q_ASSERT(m_controller);
+        Q_ASSERT(m_helper->item);
         m_controller->m_transforms.remove(m_helper->item);
         delete m_helper;
     }
@@ -266,6 +270,7 @@ void QQuickTransformAnimatorJob::initialize(QQuickAnimatorController *controller
     QQuickAnimatorJob::initialize(controller);
 
     if (m_controller) {
+        bool newHelper = m_helper == 0;
         m_helper = m_controller->m_transforms.value(m_target);
         if (!m_helper) {
             m_helper = new Helper();
@@ -273,7 +278,8 @@ void QQuickTransformAnimatorJob::initialize(QQuickAnimatorController *controller
             m_controller->m_transforms.insert(m_target, m_helper);
             QObject::connect(m_target, SIGNAL(destroyed(QObject*)), m_controller, SLOT(itemDestroyed(QObject*)), Qt::DirectConnection);
         } else {
-            ++m_helper->ref;
+            if (newHelper) // only add reference the first time around..
+                ++m_helper->ref;
             // Make sure leftovers from previous runs are being used...
             m_helper->wasSynced = false;
         }
@@ -287,6 +293,12 @@ void QQuickTransformAnimatorJob::nodeWasDestroyed()
         m_helper->node = 0;
 }
 
+void QQuickTransformAnimatorJob::targetWasDeleted()
+{
+    m_helper = 0;
+    QQuickAnimatorJob::targetWasDeleted();
+}
+
 void QQuickTransformAnimatorJob::Helper::sync()
 {
     const quint32 mask = QQuickItemPrivate::Position
diff --git a/src/quick/util/qquickanimatorjob_p.h b/src/quick/util/qquickanimatorjob_p.h
index f91410950c..4bdcd6917d 100644
--- a/src/quick/util/qquickanimatorjob_p.h
+++ b/src/quick/util/qquickanimatorjob_p.h
@@ -122,7 +122,7 @@ public:
     QEasingCurve easingCurve() const { return m_easing; }
     void setEasingCurve(const QEasingCurve &curve) { m_easing = curve; }
 
-    void targetWasDeleted();
+    virtual void targetWasDeleted();
     virtual void initialize(QQuickAnimatorController *controller);
     virtual void writeBack() = 0;
     virtual void nodeWasDestroyed() = 0;
@@ -204,6 +204,7 @@ protected:
     QQuickTransformAnimatorJob();
     void initialize(QQuickAnimatorController *controller);
     void nodeWasDestroyed();
+    void targetWasDeleted() Q_DECL_OVERRIDE;
 
     Helper *m_helper;
 };
diff --git a/tests/auto/qmltest/animators/tst_targetdestroyed.qml b/tests/auto/qmltest/animators/tst_targetdestroyed.qml
new file mode 100644
index 0000000000..92b14d0594
--- /dev/null
+++ b/tests/auto/qmltest/animators/tst_targetdestroyed.qml
@@ -0,0 +1,77 @@
+/****************************************************************************
+**
+** Copyright (C) 2014 Gunnar Sletta <gunnar@sletta.org>
+** Contact: http://www.qt-project.org/legal
+**
+** This file is part of the test suite of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL21$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and Digia. For licensing terms and
+** conditions see http://qt.digia.com/licensing. For further information
+** use the contact form at http://qt.digia.com/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 2.1 or version 3 as published by the Free
+** Software Foundation and appearing in the file LICENSE.LGPLv21 and
+** LICENSE.LGPLv3 included in the packaging of this file. Please review the
+** following information to ensure the GNU Lesser General Public License
+** requirements will be met: https://www.gnu.org/licenses/lgpl.html and
+** http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** In addition, as a special exception, Digia gives you certain additional
+** rights. These rights are described in the Digia Qt LGPL Exception
+** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+import QtQuick 2.2
+import QtTest 1.0
+
+Item {
+    id: root;
+    width: 200
+    height: 200
+
+    TestCase {
+        id: testcase
+        name: "animators-targetdestroyed"
+        when: false
+        function test_endresult() {
+            verify(true, "Got here :)");
+        }
+    }
+
+    Rectangle {
+        id: box
+        width: 10
+        height: 10
+        color: "steelblue"
+    }
+
+    YAnimator {
+        id: anim
+        target: box
+        from: 0;
+        to: 100
+        duration: 100
+        loops: Animation.Infinite
+        running: true
+    }
+
+    SequentialAnimation {
+        running: true
+        PauseAnimation { duration: 150 }
+        ScriptAction { script: box.destroy(); }
+        PauseAnimation { duration: 50 }
+        ScriptAction { script: anim.destroy(); }
+        PauseAnimation { duration: 50 }
+        ScriptAction { script: testcase.when = true }
+    }
+}