Fix JS ownership of model and delegate properties in QtQuick item views

When assigning a JS owned model or delegate to an item view, we must
ensure that they stay alive as long as the item view. This happens
easily for example when doing something like

    delegate: Qt.createComponent(...)

This patch takes the minimally invasive approach by changing the QObject
parent of such objects.

Task-number: QTBUG-50319
Task-number: QTBUG-51620
Change-Id: Ie6384b8dd93dcdc62d49f64b38173b3fc4ffd3b3
Reviewed-by: J-P Nurmi <jpnurmi@qt.io>

7 files changed, 105 insertions, 9 deletions

diff --git a/src/qml/qml/qqmlguard_p.h b/src/qml/qml/qqmlguard_p.h
index 808bf4c709..3ac63926a0 100644
--- a/src/qml/qml/qqmlguard_p.h
+++ b/src/qml/qml/qqmlguard_p.h
@@ -106,6 +106,34 @@ protected:
     virtual void objectDestroyed(T *) {}
 };
 
+template <typename T>
+class QQmlStrongJSQObjectReference : public QQmlGuard<T>
+{
+public:
+    void setObject(T *o, QObject *parent) {
+        T *old = this->object();
+        if (o == old)
+            return;
+
+        if (m_jsOwnership && old && old->parent() == parent)
+            QQml_setParent_noEvent(old, nullptr);
+
+        this->QQmlGuard<T>::operator=(o);
+
+        if (o && !o->parent() && !QQmlData::keepAliveDuringGarbageCollection(o)) {
+            m_jsOwnership = true;
+            QQml_setParent_noEvent(o, parent);
+        } else {
+            m_jsOwnership = false;
+        }
+    }
+
+private:
+    using QQmlGuard<T>::setObject;
+    using QQmlGuard<T>::operator=;
+    bool m_jsOwnership = false;
+};
+
 QT_END_NAMESPACE
 
 Q_DECLARE_METATYPE(QQmlGuard<QObject>)
diff --git a/src/qml/types/qqmldelegatemodel.cpp b/src/qml/types/qqmldelegatemodel.cpp
index 1c0da2875c..61cbb8700a 100644
--- a/src/qml/types/qqmldelegatemodel.cpp
+++ b/src/qml/types/qqmldelegatemodel.cpp
@@ -200,8 +200,7 @@ QQmlDelegateModelParts::QQmlDelegateModelParts(QQmlDelegateModel *parent)
 */
 
 QQmlDelegateModelPrivate::QQmlDelegateModelPrivate(QQmlContext *ctxt)
-    : m_delegate(nullptr)
-    , m_cacheMetaType(nullptr)
+    : m_cacheMetaType(nullptr)
     , m_context(ctxt)
     , m_parts(nullptr)
     , m_filterGroup(QStringLiteral("items"))
@@ -263,6 +262,7 @@ QQmlDelegateModel::QQmlDelegateModel(QQmlContext *ctxt, QObject *parent)
 QQmlDelegateModel::~QQmlDelegateModel()
 {
     Q_D(QQmlDelegateModel);
+    d->m_adaptorModel.setObject(nullptr, this);
 
     for (QQmlDelegateModelItem *cacheItem : qAsConst(d->m_cache)) {
         if (cacheItem->object) {
@@ -409,7 +409,7 @@ void QQmlDelegateModel::setDelegate(QQmlComponent *delegate)
         return;
     }
     bool wasValid = d->m_delegate != nullptr;
-    d->m_delegate = delegate;
+    d->m_delegate.setObject(delegate, this);
     d->m_delegateValidated = false;
     if (wasValid && d->m_complete) {
         for (int i = 1; i < d->m_groupCount; ++i) {
diff --git a/src/qml/types/qqmldelegatemodel_p_p.h b/src/qml/types/qqmldelegatemodel_p_p.h
index fc46986e23..3f9dd12243 100644
--- a/src/qml/types/qqmldelegatemodel_p_p.h
+++ b/src/qml/types/qqmldelegatemodel_p_p.h
@@ -305,7 +305,7 @@ public:
 
     QQmlAdaptorModel m_adaptorModel;
     QQmlListCompositor m_compositor;
-    QQmlComponent *m_delegate;
+    QQmlStrongJSQObjectReference<QQmlComponent> m_delegate;
     QQmlDelegateModelItemMetaType *m_cacheMetaType;
     QPointer<QQmlContext> m_context;
     QQmlDelegateModelParts *m_parts;
diff --git a/src/qml/util/qqmladaptormodel.cpp b/src/qml/util/qqmladaptormodel.cpp
index c430074f56..15844017aa 100644
--- a/src/qml/util/qqmladaptormodel.cpp
+++ b/src/qml/util/qqmladaptormodel.cpp
@@ -956,7 +956,7 @@ void QQmlAdaptorModel::setModel(const QVariant &variant, QQmlDelegateModel *vdm,
     list.setList(variant, engine);
 
     if (QObject *object = qvariant_cast<QObject *>(list.list())) {
-        setObject(object);
+        setObject(object, vdm);
         if (QAbstractItemModel *model = qobject_cast<QAbstractItemModel *>(object)) {
             accessors = new VDMAbstractItemModelDataType(this);
 
@@ -978,14 +978,14 @@ void QQmlAdaptorModel::setModel(const QVariant &variant, QQmlDelegateModel *vdm,
             accessors = new VDMObjectDelegateDataType;
         }
     } else if (list.type() == QQmlListAccessor::ListProperty) {
-        setObject(static_cast<const QQmlListReference *>(variant.constData())->object());
+        setObject(static_cast<const QQmlListReference *>(variant.constData())->object(), vdm);
         accessors = new VDMObjectDelegateDataType;
     } else if (list.type() != QQmlListAccessor::Invalid
             && list.type() != QQmlListAccessor::Instance) { // Null QObject
-        setObject(nullptr);
+        setObject(nullptr, vdm);
         accessors = &qt_vdm_list_accessors;
     } else {
-        setObject(nullptr);
+        setObject(nullptr, vdm);
         accessors = &qt_vdm_null_accessors;
     }
 }
diff --git a/src/qml/util/qqmladaptormodel_p.h b/src/qml/util/qqmladaptormodel_p.h
index b706fcb5f2..3b2d180ca7 100644
--- a/src/qml/util/qqmladaptormodel_p.h
+++ b/src/qml/util/qqmladaptormodel_p.h
@@ -68,7 +68,7 @@ class QQmlDelegateModel;
 class QQmlDelegateModelItem;
 class QQmlDelegateModelItemMetaType;
 
-class QQmlAdaptorModel : public QQmlGuard<QObject>
+class QQmlAdaptorModel : public QQmlStrongJSQObjectReference<QObject>
 {
 public:
     class Accessors
diff --git a/tests/auto/quick/qquickrepeater/data/ownership.qml b/tests/auto/quick/qquickrepeater/data/ownership.qml
new file mode 100644
index 0000000000..e13df3ab3a
--- /dev/null
+++ b/tests/auto/quick/qquickrepeater/data/ownership.qml
@@ -0,0 +1,4 @@
+import QtQuick 2.0
+
+Repeater {
+}
diff --git a/tests/auto/quick/qquickrepeater/tst_qquickrepeater.cpp b/tests/auto/quick/qquickrepeater/tst_qquickrepeater.cpp
index 791c3ae19a..0860956224 100644
--- a/tests/auto/quick/qquickrepeater/tst_qquickrepeater.cpp
+++ b/tests/auto/quick/qquickrepeater/tst_qquickrepeater.cpp
@@ -37,6 +37,7 @@
 #include <QtQuick/private/qquicktext_p.h>
 #include <QtQml/private/qqmllistmodel_p.h>
 #include <QtQml/private/qqmlobjectmodel_p.h>
+#include <QtGui/qstandarditemmodel.h>
 
 #include "../../shared/util.h"
 #include "../shared/viewtestutil.h"
@@ -77,6 +78,7 @@ private slots:
     void objectModel();
     void QTBUG54859_asynchronousMove();
     void package();
+    void ownership();
 };
 
 class TestObject : public QObject
@@ -1038,6 +1040,68 @@ void tst_QQuickRepeater::package()
     }
 }
 
+void tst_QQuickRepeater::ownership()
+{
+    QQmlEngine engine;
+
+    QQmlComponent component(&engine, testFileUrl("ownership.qml"));
+
+    QScopedPointer<QAbstractItemModel> aim(new QStandardItemModel);
+    QPointer<QAbstractItemModel> modelGuard(aim.data());
+    QQmlEngine::setObjectOwnership(aim.data(), QQmlEngine::JavaScriptOwnership);
+    {
+        QJSValue wrapper = engine.newQObject(aim.data());
+    }
+
+    QScopedPointer<QObject> repeater(component.create());
+    QVERIFY(!repeater.isNull());
+
+    QVERIFY(!QQmlData::keepAliveDuringGarbageCollection(aim.data()));
+
+    repeater->setProperty("model", QVariant::fromValue<QObject*>(aim.data()));
+
+    QVERIFY(!QQmlData::keepAliveDuringGarbageCollection(aim.data()));
+
+    engine.collectGarbage();
+    QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete);
+
+    QVERIFY(modelGuard);
+
+    QScopedPointer<QQmlComponent> delegate(new QQmlComponent(&engine));
+    delegate->setData(QByteArrayLiteral("import QtQuick 2.0\nItem{}"), dataDirectoryUrl().resolved(QUrl("inline.qml")));
+    QPointer<QQmlComponent> delegateGuard(delegate.data());
+    QQmlEngine::setObjectOwnership(delegate.data(), QQmlEngine::JavaScriptOwnership);
+    {
+        QJSValue wrapper = engine.newQObject(delegate.data());
+    }
+
+    QVERIFY(!QQmlData::keepAliveDuringGarbageCollection(delegate.data()));
+
+    repeater->setProperty("delegate", QVariant::fromValue<QObject*>(delegate.data()));
+
+    QVERIFY(!QQmlData::keepAliveDuringGarbageCollection(delegate.data()));
+
+    engine.collectGarbage();
+    QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete);
+
+    QVERIFY(delegateGuard);
+
+    repeater->setProperty("model", QVariant());
+    repeater->setProperty("delegate", QVariant());
+
+    QVERIFY(delegateGuard);
+    QVERIFY(modelGuard);
+
+    delegate.take();
+    aim.take();
+
+    engine.collectGarbage();
+    QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete);
+
+    QVERIFY(!delegateGuard);
+    QVERIFY(!modelGuard);
+}
+
 QTEST_MAIN(tst_QQuickRepeater)
 
 #include "tst_qquickrepeater.moc"