Fix crash caused by objects self-destructions during displacement animations

The root cause was that the QAbstractAnimationJob::finished() might delegate its
destruction to change.listener->animationFinished(this), and the original author
was aware of that and provided a RETURN_IF_DELETE macro to return early if itself
got deleted. In the bug's case, change.listener->animationFinished(this)
dispatched to QQuickItemViewPrivate::animationFinished() which called
QQuickItemViewPrivate::release() and deleted the QAbstractAnimationJob object
itself in the end.

However, any objects derived from QAbstractAnimationJob, or holding a pointer
to a QAbstractAnimationJob, may potentially fall into the code path calling
QAbstractAnimationJob::finished(). Any QAnimationJobChangeListener that directly
or indirectly deletes QAbstractAnimationJob should be very suspicious to this
kind of "heap-use-after-free" bug. Should ensure that the QAbstractAnimationJob
won't be referenced after deletion.

In the bug's case, within the code path triggered by ListView displacement
animation, the other affected classes by QAbstractAnimationJob are:
QQuickItemViewFxItem, QQuickItemViewTransitionableItem, QQuickTransitionManager.

To fix this, a new SelfDeletable class is factored out to simplify the self-deletion
test logic. Any affected classes are made to have a public member m_selfDeletable.
Any code paths that finally reach QAbstractAnimationJob::finished() are
wrapped with related util macro.

Change-Id: Idd33fc3f2d529fd7d8bb088c329101b1e70dd6c0
Task-number: QTBUG-44308
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
 

1 files changed, 5 insertions, 15 deletions

diff --git a/src/quick/items/qquickitemviewtransition.cpp b/src/quick/items/qquickitemviewtransition.cpp
index 0fde0beb75..109851608b 100644
--- a/src/quick/items/qquickitemviewtransition.cpp
+++ b/src/quick/items/qquickitemviewtransition.cpp
@@ -61,7 +61,6 @@ public:
     QPointF m_toPos;
     QQuickItemViewTransitioner::TransitionType m_type;
     bool m_isTarget;
-    bool *m_wasDeleted;
 
 protected:
     void finished() override;
@@ -73,14 +72,11 @@ QQuickItemViewTransitionJob::QQuickItemViewTransitionJob()
     , m_item(nullptr)
     , m_type(QQuickItemViewTransitioner::NoTransition)
     , m_isTarget(false)
-    , m_wasDeleted(nullptr)
 {
 }
 
 QQuickItemViewTransitionJob::~QQuickItemViewTransitionJob()
 {
-    if (m_wasDeleted)
-        *m_wasDeleted = true;
     if (m_transitioner)
         m_transitioner->runningJobs.remove(this);
 }
@@ -138,13 +134,7 @@ void QQuickItemViewTransitionJob::finished()
     QQuickTransitionManager::finished();
 
     if (m_transitioner) {
-        bool deleted = false;
-        m_wasDeleted = &deleted;
-        m_transitioner->finishedTransition(this, m_item);
-        if (deleted)
-            return;
-        m_wasDeleted = nullptr;
-
+        RETURN_IF_DELETED(m_transitioner->finishedTransition(this, m_item));
         m_transitioner = nullptr;
     }
 
@@ -482,7 +472,7 @@ bool QQuickItemViewTransitionableItem::prepareTransition(QQuickItemViewTransitio
         // if transition type is not valid, the previous transition still has to be
         // canceled so that the item can move immediately to the right position
         item->setPosition(nextTransitionTo);
-        stopTransition();
+        ACTION_IF_DELETED(this, stopTransition(), return false);
     }
 
     prepared = true;
@@ -501,12 +491,12 @@ void QQuickItemViewTransitionableItem::startTransition(QQuickItemViewTransitione
 
     if (!transition || transition->m_type != nextTransitionType || transition->m_isTarget != isTransitionTarget) {
         if (transition)
-            transition->cancel();
+            RETURN_IF_DELETED(transition->cancel());
         delete transition;
         transition = new QQuickItemViewTransitionJob;
     }
 
-    transition->startTransition(this, index, transitioner, nextTransitionType, nextTransitionTo, isTransitionTarget);
+    RETURN_IF_DELETED(transition->startTransition(this, index, transitioner, nextTransitionType, nextTransitionTo, isTransitionTarget));
     clearCurrentScheduledTransition();
 }
 
@@ -558,7 +548,7 @@ void QQuickItemViewTransitionableItem::clearCurrentScheduledTransition()
 void QQuickItemViewTransitionableItem::stopTransition()
 {
     if (transition)
-        transition->cancel();
+        RETURN_IF_DELETED(transition->cancel());
     clearCurrentScheduledTransition();
     resetNextTransitionPos();
 }