PropertyAnimation: Handle targets being deleted

QQuickPropertyAnimation does not own the targets in its targets list.
Thus, we need to be careful to not access deleted objects. Calling
QQmlData::wasDeleted is not enoguh, as the object might be fully
deleted, but we'd still have a dangling pointer to it.

Fix the issue by using QPointer. A similar issue might exists for
target, but there we don't normally end up with dangling references -
the engine will correctly null the target. And the QPointer isNull
check will avoid potentiall nullptr derefences there, too.

Ideally, we would use QQmlGuard instead of QPointer, as it would have a
lower overhead - however, we run into linker issues on MSVC. Fixing them
is deferred to a future commit, to not block this crash fix.

Fixes: QTBUG-100392
Pick-to: 6.5 6.6 6.2
Change-Id: If084e2e0f22d50dbee8bf5aedfa2e749e79fc105
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>

src/quick/util/qquickanimation.cpp

@@ -2649,7 +2649,38 @@ void QQuickPropertyAnimation::setProperties(const QString &prop)
 QQmlListProperty<QObject> QQuickPropertyAnimation::targets()
 {
     Q_D(QQuickPropertyAnimation);
-    return QQmlListProperty<QObject>(this, &(d->targets));
+    using ListPtr = QList<QPointer<QObject>> *;
+    using LP = QQmlListProperty<QObject>;
+    LP::AppendFunction appendFn = [](LP *prop, QObject *value)
+    {
+        static_cast<ListPtr>(prop->data)->append(value);
+    };
+    LP::CountFunction countFn = [](LP *prop)
+    {
+        return static_cast<ListPtr>(prop->data)->size();
+    };
+
+    LP::AtFunction atFn = [](LP *prop, qsizetype index) -> QObject *
+    {
+        return static_cast<ListPtr>(prop->data)->at(index);
+    };
+
+    LP::ClearFunction clearFN = [](LP *prop)
+    {
+        return static_cast<ListPtr>(prop->data)->clear();
+    };
+
+    LP::ReplaceFunction replaceFn = [](LP *prop, qsizetype index, QObject *value)
+    {
+        static_cast<ListPtr>(prop->data)->replace(index, value);
+    };
+
+    LP::RemoveLastFunction removeLastFn = [](LP *prop)
+    {
+        static_cast<ListPtr>(prop->data)->removeLast();
+    };
+
+    return QQmlListProperty<QObject>(this, &(d->targets), appendFn, countFn, atFn, clearFN, replaceFn, removeLastFn);
 }
 
 /*!
@@ -2721,7 +2752,7 @@ QQuickStateActions QQuickPropertyAnimation::createTransitionActions(QQuickStateA
     if (!d->propertyName.isEmpty())
         props << d->propertyName;
 
-    QList<QObject*> targets = d->targets;
+    QList<QPointer<QObject>> targets = d->targets;
     if (d->target)
         targets.append(d->target);
 
@@ -2750,10 +2781,14 @@ QQuickStateActions QQuickPropertyAnimation::createTransitionActions(QQuickStateA
 
         for (int i = 0; i < props.size(); ++i) {
             for (int j = 0; j < targets.size(); ++j) {
+                const auto& guarded = targets.at(j);
+                if (guarded.isNull())
+                    continue;
+                QObject *target = guarded.get();
                 QQuickStateAction myAction;
                 QString errorMessage;
                 const QString &propertyName = props.at(i);
-                myAction.property = d->createProperty(targets.at(j), propertyName, this, &errorMessage);
+                myAction.property = d->createProperty(target, propertyName, this, &errorMessage);
                 if (myAction.property.isValid()) {
                     if (usingDefaultProperties)
                         successfullyCreatedDefaultProperty = true;

src/quick/util/qquickanimation_p_p.h

@@ -250,7 +250,7 @@ class Q_QUICK_PRIVATE_EXPORT QQuickPropertyAnimationPrivate : public QQuickAbstr
     QObject *target;
     QString propertyName;
     QString properties;
-    QList<QObject *> targets;
+    QList<QPointer<QObject>> targets;
     QList<QObject *> exclude;
     QString defaultProperties;
 

tests/auto/quick/qquickanimations/data/targetsDeletedWithoutRemoval.qml

@@ -0,0 +1,29 @@
+import QtQuick
+
+Item {
+    id: root
+
+    property list<Translate> targets
+    property alias animTargets: animation.targets
+
+    Component {
+        id: trComponent
+        Translate {}
+    }
+
+    Component.onCompleted: {
+        const target = trComponent.createObject(this);
+        targets.push(target);
+        target.destroy();
+        // give event loop some time to actually stop the animation and destroy the target
+        Qt.callLater(animation.start);
+    }
+
+    NumberAnimation {
+        id: animation
+        targets: root.targets
+        property: "x"
+        running: false
+        to: 100
+    }
+}

tests/auto/quick/qquickanimations/tst_qquickanimations.cpp

@@ -100,6 +100,7 @@ private slots:
     void frameAnimation2();
     void restartAnimationGroupWhenDirty();
     void restartNestedAnimationGroupWhenDirty();
+    void targetsDeletedNotRemoved();
 };
 
 #define QTIMED_COMPARE(lhs, rhs) do { \
@@ -2273,6 +2274,27 @@ void tst_qquickanimations::restartNestedAnimationGroupWhenDirty()
     QTRY_COMPARE(target0->x(), 140);
     QTRY_COMPARE(target1->x(), 140);
 }
+
+void tst_qquickanimations::targetsDeletedNotRemoved()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl("targetsDeletedWithoutRemoval.qml"));
+    QScopedPointer<QObject> obj(component.create());
+    QVERIFY2(obj.get(), qPrintable(component.errorString()));
+    {
+        QQmlListReference ref(obj.get(), "targets");
+        QVERIFY(ref.isValid());
+        QCOMPARE(ref.size(), 1);
+        QTRY_COMPARE(ref.at(0), nullptr);
+    }
+    {
+        QQmlListReference ref(obj.get(), "animTargets");
+        QVERIFY(ref.isValid());
+        QCOMPARE(ref.size(), 1);
+        QCOMPARE(ref.at(0), nullptr);
+    }
+}
+
 QTEST_MAIN(tst_qquickanimations)
 
 #include "tst_qquickanimations.moc"