Fix a crash in animation groups when deleting uncontrolled animations

The problem was that we were not removing their references from the
private object hash and at some point we could access it.

Task-number: QTBUG-8910
Reviewed-by: gabi

7 files changed, 51 insertions, 17 deletions

diff --git a/src/corelib/animation/qanimationgroup.cpp b/src/corelib/animation/qanimationgroup.cpp
index a89f949..2decda3 100644
--- a/src/corelib/animation/qanimationgroup.cpp
+++ b/src/corelib/animation/qanimationgroup.cpp
@@ -244,7 +244,7 @@ QAbstractAnimation *QAnimationGroup::takeAnimation(int index)
     // in ChildRemoved event
     d->animations.removeAt(index);
     animation->setParent(0);
-    d->animationRemovedAt(index);
+    d->animationRemoved(index, animation);
     return animation;
 }
 
@@ -285,7 +285,7 @@ bool QAnimationGroup::event(QEvent *event)
 }
 
 
-void QAnimationGroupPrivate::animationRemovedAt(int index)
+void QAnimationGroupPrivate::animationRemoved(int index, QAbstractAnimation *)
 {
     Q_Q(QAnimationGroup);
     Q_UNUSED(index);
diff --git a/src/corelib/animation/qanimationgroup_p.h b/src/corelib/animation/qanimationgroup_p.h
index 394773b..45603b3 100644
--- a/src/corelib/animation/qanimationgroup_p.h
+++ b/src/corelib/animation/qanimationgroup_p.h
@@ -72,8 +72,19 @@ public:
         isGroup = true;
     }
 
-    virtual void animationInsertedAt(int index) { Q_UNUSED(index) };
-    virtual void animationRemovedAt(int index);
+    virtual void animationInsertedAt(int) { }
+    virtual void animationRemoved(int, QAbstractAnimation *);
+
+    void disconnectUncontrolledAnimation(QAbstractAnimation *anim)
+    {
+        //0 for the signal here because we might be called from the animation destructor
+        QObject::disconnect(anim, 0, q_func(), SLOT(_q_uncontrolledAnimationFinished()));
+    }
+
+    void connectUncontrolledAnimation(QAbstractAnimation *anim)
+    {
+        QObject::connect(anim, SIGNAL(finished()), q_func(), SLOT(_q_uncontrolledAnimationFinished()));
+    }
 
     QList<QAbstractAnimation *> animations;
 };
diff --git a/src/corelib/animation/qparallelanimationgroup.cpp b/src/corelib/animation/qparallelanimationgroup.cpp
index eaa0364..280afed 100644
--- a/src/corelib/animation/qparallelanimationgroup.cpp
+++ b/src/corelib/animation/qparallelanimationgroup.cpp
@@ -246,11 +246,9 @@ void QParallelAnimationGroupPrivate::_q_uncontrolledAnimationFinished()
 
 void QParallelAnimationGroupPrivate::disconnectUncontrolledAnimations()
 {
-    Q_Q(QParallelAnimationGroup);
-
     QHash<QAbstractAnimation *, int>::iterator it = uncontrolledFinishTime.begin();
     while (it != uncontrolledFinishTime.end()) {
-        QObject::disconnect(it.key(), SIGNAL(finished()), q, SLOT(_q_uncontrolledAnimationFinished()));
+        disconnectUncontrolledAnimation(it.key());
         ++it;
     }
 
@@ -259,13 +257,11 @@ void QParallelAnimationGroupPrivate::disconnectUncontrolledAnimations()
 
 void QParallelAnimationGroupPrivate::connectUncontrolledAnimations()
 {
-    Q_Q(QParallelAnimationGroup);
-
     for (int i = 0; i < animations.size(); ++i) {
         QAbstractAnimation *animation = animations.at(i);
         if (animation->duration() == -1 || animation->loopCount() < 0) {
             uncontrolledFinishTime[animation] = -1;
-            QObject::connect(animation, SIGNAL(finished()), q, SLOT(_q_uncontrolledAnimationFinished()));
+            connectUncontrolledAnimation(animation);
         }
     }
 }
@@ -305,6 +301,13 @@ bool QParallelAnimationGroupPrivate::isUncontrolledAnimationFinished(QAbstractAn
     return uncontrolledFinishTime.value(anim, -1) >= 0;
 }
 
+void QParallelAnimationGroupPrivate::animationRemoved(int index, QAbstractAnimation *anim)
+{
+    QAnimationGroupPrivate::animationRemoved(index, anim);
+    disconnectUncontrolledAnimation(anim);
+    uncontrolledFinishTime.remove(anim);
+}
+
 /*!
     \reimp
 */
diff --git a/src/corelib/animation/qparallelanimationgroup_p.h b/src/corelib/animation/qparallelanimationgroup_p.h
index a74d496..cab4fa9 100644
--- a/src/corelib/animation/qparallelanimationgroup_p.h
+++ b/src/corelib/animation/qparallelanimationgroup_p.h
@@ -80,6 +80,8 @@ public:
     void connectUncontrolledAnimations();
     void disconnectUncontrolledAnimations();
 
+    void animationRemoved(int index, QAbstractAnimation *);
+
     // private slot
     void _q_uncontrolledAnimationFinished();
 };
diff --git a/src/corelib/animation/qsequentialanimationgroup.cpp b/src/corelib/animation/qsequentialanimationgroup.cpp
index 8ab084a..7617c1f 100644
--- a/src/corelib/animation/qsequentialanimationgroup.cpp
+++ b/src/corelib/animation/qsequentialanimationgroup.cpp
@@ -479,7 +479,7 @@ void QSequentialAnimationGroupPrivate::activateCurrentAnimation(bool intermediat
 
     // connects to the finish signal of uncontrolled animations
     if (currentAnimation->totalDuration() == -1)
-        QObject::connect(currentAnimation, SIGNAL(finished()), q, SLOT(_q_uncontrolledAnimationFinished()));
+        connectUncontrolledAnimation(currentAnimation);
 
     currentAnimation->start();
     if (!intermediate && state == QSequentialAnimationGroup::Paused)
@@ -496,7 +496,7 @@ void QSequentialAnimationGroupPrivate::_q_uncontrolledAnimationFinished()
         actualDuration.append(-1);
     actualDuration[currentAnimationIndex] = currentAnimation->currentTime();
 
-    QObject::disconnect(currentAnimation, SIGNAL(finished()), q, SLOT(_q_uncontrolledAnimationFinished()));
+    disconnectUncontrolledAnimation(currentAnimation);
 
     if ((direction == QAbstractAnimation::Forward && currentAnimation == animations.last())
         || (direction == QAbstractAnimation::Backward && currentAnimationIndex == 0)) {
@@ -543,10 +543,10 @@ void QSequentialAnimationGroupPrivate::animationInsertedAt(int index)
     the group at index \a index. The animation is no more listed when this
     method is called.
 */
-void QSequentialAnimationGroupPrivate::animationRemovedAt(int index)
+void QSequentialAnimationGroupPrivate::animationRemoved(int index, QAbstractAnimation *anim)
 {
     Q_Q(QSequentialAnimationGroup);
-    QAnimationGroupPrivate::animationRemovedAt(index);
+    QAnimationGroupPrivate::animationRemoved(index, anim);
 
     Q_ASSERT(currentAnimation); // currentAnimation should always be set
 
@@ -555,7 +555,10 @@ void QSequentialAnimationGroupPrivate::animationRemovedAt(int index)
 
     const int currentIndex = animations.indexOf(currentAnimation);
     if (currentIndex == -1) {
-        //we're removing the current animation, let's update it to another one
+        //we're removing the current animation
+
+        disconnectUncontrolledAnimation(currentAnimation);
+
         if (index < animations.count())
             setCurrentAnimation(index); //let's try to take the next one
         else if (index > 0)
diff --git a/src/corelib/animation/qsequentialanimationgroup_p.h b/src/corelib/animation/qsequentialanimationgroup_p.h
index a55e136..de69916 100644
--- a/src/corelib/animation/qsequentialanimationgroup_p.h
+++ b/src/corelib/animation/qsequentialanimationgroup_p.h
@@ -85,7 +85,7 @@ public:
     void activateCurrentAnimation(bool intermediate = false);
 
     void animationInsertedAt(int index);
-    void animationRemovedAt(int index);
+    void animationRemoved(int index, QAbstractAnimation *anim);
 
     bool atEnd() const;
 
diff --git a/tests/auto/qparallelanimationgroup/tst_qparallelanimationgroup.cpp b/tests/auto/qparallelanimationgroup/tst_qparallelanimationgroup.cpp
index fb0f3e0..d2d86fb 100644
--- a/tests/auto/qparallelanimationgroup/tst_qparallelanimationgroup.cpp
+++ b/tests/auto/qparallelanimationgroup/tst_qparallelanimationgroup.cpp
@@ -75,6 +75,8 @@ private slots:
     void loopCount();
     void autoAdd();
     void pauseResume();
+
+    void QTBUG8910_crashWhenRemovingUncontrolledAnimation();
 };
 
 tst_QParallelAnimationGroup::tst_QParallelAnimationGroup()
@@ -999,9 +1001,22 @@ void tst_QParallelAnimationGroup::pauseResume()
     QCOMPARE(spy.count(), 2); //this shouldn't have changed
     group.resume();
     QCOMPARE(spy.count(), 2); //this shouldn't have changed
+}
 
 
-
+void tst_QParallelAnimationGroup::QTBUG8910_crashWhenRemovingUncontrolledAnimation()
+{
+    QParallelAnimationGroup group;
+    TestAnimation *anim = new TestAnimation;
+    anim->setLoopCount(-1);
+    TestAnimation *anim2 = new TestAnimation;
+    anim2->setLoopCount(-1);
+    group.addAnimation(anim);
+    group.addAnimation(anim2);
+    group.start();
+    delete anim;
+    // it would crash here because the internals of the group would still have a reference to anim
+    delete anim2;
 }