QAbstractAnimation: fixes segfault when deleting animation inside updateCurrentTime

That deletion removed the respective animation from the list of running
animations being processed, but not from the copy of the list introduced in
48489988521b818bda8d76e60cb272508e91b490, thus we had a dangling pointer.

Reviewed-by: thierry

3 files changed, 68 insertions, 13 deletions

diff --git a/src/corelib/animation/qabstractanimation.cpp b/src/corelib/animation/qabstractanimation.cpp
index 617f4db..dfd6779 100644
--- a/src/corelib/animation/qabstractanimation.cpp
+++ b/src/corelib/animation/qabstractanimation.cpp
@@ -161,7 +161,9 @@ QT_BEGIN_NAMESPACE
 
 Q_GLOBAL_STATIC(QThreadStorage<QUnifiedTimer *>, unifiedTimer)
 
-QUnifiedTimer::QUnifiedTimer() : QObject(), lastTick(0), timingInterval(DEFAULT_TIMER_INTERVAL), consistentTiming(false)
+QUnifiedTimer::QUnifiedTimer() :
+    QObject(), lastTick(0), timingInterval(DEFAULT_TIMER_INTERVAL),
+    currentAnimationIdx(0), consistentTiming(false)
 {
 }
 
@@ -200,21 +202,19 @@ void QUnifiedTimer::timerEvent(QTimerEvent *event)
         }
     } else if (event->timerId() == animationTimer.timerId()) {
         const int delta = lastTick - oldLastTick;
-        //we copy the list so that if it is changed we still get to
-        //call setCurrentTime on all animations.
-        const QList<QAbstractAnimation*> currentAnimations = animations;
-        for (int i = 0; i < currentAnimations.count(); ++i) {
-            QAbstractAnimation *animation = currentAnimations.at(i);
+        for (currentAnimationIdx = 0; currentAnimationIdx < animations.count(); ++currentAnimationIdx) {
+            QAbstractAnimation *animation = animations.at(currentAnimationIdx);
             int elapsed = QAbstractAnimationPrivate::get(animation)->totalCurrentTime
                 + (animation->direction() == QAbstractAnimation::Forward ? delta : -delta);
             animation->setCurrentTime(elapsed);
         }
+        currentAnimationIdx = 0;
     }
 }
 
 void QUnifiedTimer::registerAnimation(QAbstractAnimation *animation)
 {
-    if (animations.contains(animation) ||animationsToStart.contains(animation))
+    if (animations.contains(animation) || animationsToStart.contains(animation))
         return;
     animationsToStart << animation;
     startStopAnimationTimer.start(0, this); // we delay the check if we should start/stop the global timer
@@ -222,8 +222,17 @@ void QUnifiedTimer::registerAnimation(QAbstractAnimation *animation)
 
 void QUnifiedTimer::unregisterAnimation(QAbstractAnimation *animation)
 {
-    animations.removeAll(animation);
-    animationsToStart.removeAll(animation);
+    Q_ASSERT(animations.count(animation) + animationsToStart.count(animation) <= 1);
+
+    int idx = animations.indexOf(animation);
+    if (idx != -1) {
+        animations.removeAt(idx);
+        // this is needed if we unregister an animation while its running
+        if (idx <= currentAnimationIdx)
+            --currentAnimationIdx;
+    } else {
+        animationsToStart.removeOne(animation);
+    }
     startStopAnimationTimer.start(0, this); // we delay the check if we should start/stop the global timer
 }
 
diff --git a/src/corelib/animation/qabstractanimation_p.h b/src/corelib/animation/qabstractanimation_p.h
index 32189f5..bcd7354 100644
--- a/src/corelib/animation/qabstractanimation_p.h
+++ b/src/corelib/animation/qabstractanimation_p.h
@@ -141,6 +141,7 @@ private:
     QTime time;
     int lastTick;
     int timingInterval;
+    int currentAnimationIdx;
     bool consistentTiming;
     QList<QAbstractAnimation*> animations, animationsToStart;
 };
diff --git a/tests/auto/qpropertyanimation/tst_qpropertyanimation.cpp b/tests/auto/qpropertyanimation/tst_qpropertyanimation.cpp
index 04cfe1a..57fca9d 100644
--- a/tests/auto/qpropertyanimation/tst_qpropertyanimation.cpp
+++ b/tests/auto/qpropertyanimation/tst_qpropertyanimation.cpp
@@ -66,7 +66,7 @@ protected:
 class MyObject : public QObject
 {
     Q_OBJECT
-    Q_PROPERTY(qreal x READ x WRITE setX) 
+    Q_PROPERTY(qreal x READ x WRITE setX)
 public:
     MyObject() : m_x(0) { }
     qreal x() const { return m_x; }
@@ -115,6 +115,7 @@ private slots:
     void restart();
     void valueChanged();
     void twoAnimations();
+    void deletedInUpdateCurrentTime();
 };
 
 tst_QPropertyAnimation::tst_QPropertyAnimation()
@@ -142,7 +143,7 @@ class AnimationObject : public QObject
     Q_PROPERTY(qreal realValue READ realValue WRITE setRealValue)
 public:
     AnimationObject(int startValue = 0)
-        : v(startValue)
+        : v(startValue), rv(startValue)
     { }
 
     int value() const { return v; }
@@ -402,7 +403,7 @@ void tst_QPropertyAnimation::duration0()
     animation.setStartValue(42);
     QVERIFY(animation.currentValue().isValid());
     QCOMPARE(animation.currentValue().toInt(), 42);
-    
+
     QCOMPARE(o.property("ole").toInt(), 42);
     animation.setDuration(0);
     QCOMPARE(animation.currentValue().toInt(), 43); //it is at the end
@@ -1125,11 +1126,55 @@ void tst_QPropertyAnimation::twoAnimations()
 
     QCOMPARE(o1.ole(), 1000);
     QCOMPARE(o2.ole(), 1000);
-
 }
 
+class MyComposedAnimation : public QPropertyAnimation
+{
+    Q_OBJECT
+public:
+    MyComposedAnimation(QObject *target, const QByteArray &propertyName, const QByteArray &innerPropertyName)
+        : QPropertyAnimation(target, propertyName)
+    {
+        innerAnim = new QPropertyAnimation(target, innerPropertyName);
+        this->setEndValue(1000);
+        innerAnim->setEndValue(1000);
+        innerAnim->setDuration(duration() + 100);
+    }
 
+    void start()
+    {
+        QPropertyAnimation::start();
+        innerAnim->start();
+    }
 
+    void updateState(QAbstractAnimation::State oldState, QAbstractAnimation::State newState)
+    {
+        QPropertyAnimation::updateState(oldState, newState);
+        if (newState == QAbstractAnimation::Stopped)
+            delete innerAnim;
+    }
+
+public:
+    QPropertyAnimation *innerAnim;
+};
+
+void tst_QPropertyAnimation::deletedInUpdateCurrentTime()
+{
+    // this test case reproduces an animation being deleted in the updateCurrentTime of
+    // another animation(was causing segfault).
+    // the deleted animation must have been started after the animation that is deleting.
+    AnimationObject o;
+    o.setValue(0);
+    o.setRealValue(0.0);
+
+    MyComposedAnimation composedAnimation(&o, "value", "realValue");
+    composedAnimation.start();
+    QCOMPARE(composedAnimation.state(), QAbstractAnimation::Running);
+    QTest::qWait(composedAnimation.duration() + 50);
+
+    QCOMPARE(composedAnimation.state(), QAbstractAnimation::Stopped);
+    QCOMPARE(o.value(), 1000);
+}
 
 QTEST_MAIN(tst_QPropertyAnimation)
 #include "tst_qpropertyanimation.moc"