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, 4 insertions, 3 deletions

diff --git a/src/quick/util/qquicktransitionmanager.cpp b/src/quick/util/qquicktransitionmanager.cpp
index e51de1a02a..0ee7e57997 100644
--- a/src/quick/util/qquicktransitionmanager.cpp
+++ b/src/quick/util/qquicktransitionmanager.cpp
@@ -47,6 +47,7 @@
 #include <private/qqmlproperty_p.h>
 
 #include <QtCore/qdebug.h>
+#include <private/qanimationjobutil_p.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -79,6 +80,7 @@ void QQuickTransitionManager::setState(QQuickState *s)
 QQuickTransitionManager::~QQuickTransitionManager()
 {
     delete d->transitionInstance;
+    d->transitionInstance = nullptr;
     delete d; d = nullptr;
 }
 
@@ -129,7 +131,7 @@ void QQuickTransitionManager::transition(const QList<QQuickStateAction> &list,
                                       QQuickTransition *transition,
                                       QObject *defaultTarget)
 {
-    cancel();
+    RETURN_IF_DELETED(cancel());
 
     // The copy below is ON PURPOSE, because firing actions might involve scripts that modify the list.
     QQuickStateOperation::ActionList applyList = list;
@@ -154,7 +156,6 @@ void QQuickTransitionManager::transition(const QList<QQuickStateAction> &list,
     //
     // This doesn't catch everything, and it might be a little fragile in
     // some cases - but whatcha going to do?
-
     if (transition && !d->bindingsList.isEmpty()) {
 
         // Apply all the property and binding changes
@@ -258,7 +259,7 @@ void QQuickTransitionManager::transition(const QList<QQuickStateAction> &list,
 void QQuickTransitionManager::cancel()
 {
     if (d->transitionInstance && d->transitionInstance->isRunning())
-        d->transitionInstance->stop();
+        RETURN_IF_DELETED(d->transitionInstance->stop());
 
     for (const QQuickStateAction &action : qAsConst(d->bindingsList)) {
         if (action.toBinding && action.deletableToBinding) {