Destroy incubated objects if they are not claimed when ready.

If model data is removed while an object is incubating for it the view
cannot cancel the incubation because the index is invalidated. So if
the object has not been referenced after signalling it is ready assume
it's been abandoned and destroy it.

Change-Id: I32708e940cfbccbe0d330ad7822f29d3c1f39d13
Reviewed-by: Martin Jones <martin.jones@nokia.com>

3 files changed, 334 insertions, 27 deletions

diff --git a/src/quick/items/qquickvisualdatamodel.cpp b/src/quick/items/qquickvisualdatamodel.cpp
index 197a60b80b..b8e51d75eb 100644
--- a/src/quick/items/qquickvisualdatamodel.cpp
+++ b/src/quick/items/qquickvisualdatamodel.cpp
@@ -518,10 +518,10 @@ void QQuickVisualDataModel::cancel(int index)
         if (cacheItem->object && !cacheItem->isObjectReferenced()) {
             QObject *object = cacheItem->object;
             cacheItem->destroyObject();
-            if (QQuickPackage *package = qmlobject_cast<QQuickPackage *>(object))
-                d->emitDestroyingPackage(package);
-            else if (QQuickItem *item = qmlobject_cast<QQuickItem *>(object))
+            if (QQuickItem *item = qmlobject_cast<QQuickItem *>(object))
                 d->emitDestroyingItem(item);
+            else if (QQuickPackage *package = qmlobject_cast<QQuickPackage *>(object))
+                d->emitDestroyingPackage(package);
             cacheItem->scriptRef -= 1;
         }
         if (!cacheItem->isReferenced()) {
@@ -770,6 +770,16 @@ void QQuickVisualDataModelPrivate::releaseIncubator(QVDMIncubationTask *incubati
     }
 }
 
+void QQuickVisualDataModelPrivate::removeCacheItem(QQuickVisualDataModelItem *cacheItem)
+{
+    int cidx = m_cache.indexOf(cacheItem);
+    if (cidx >= 0) {
+        m_compositor.clearFlags(Compositor::Cache, cidx, 1, Compositor::CacheFlag);
+        m_cache.removeAt(cidx);
+    }
+    Q_ASSERT(m_cache.count() == m_compositor.count(Compositor::Cache));
+}
+
 void QQuickVisualDataModelPrivate::incubatorStatusChanged(QVDMIncubationTask *incubationTask, QQmlIncubator::Status status)
 {
     Q_Q(QQuickVisualDataModel);
@@ -778,29 +788,32 @@ void QQuickVisualDataModelPrivate::incubatorStatusChanged(QVDMIncubationTask *in
 
     QQuickVisualDataModelItem *cacheItem = incubationTask->incubating;
     cacheItem->incubationTask = 0;
+    incubationTask->incubating = 0;
+    releaseIncubator(incubationTask);
 
     if (status == QQmlIncubator::Ready) {
-        incubationTask->incubating = 0;
-        releaseIncubator(incubationTask);
         if (QQuickItem *item = qmlobject_cast<QQuickItem *>(cacheItem->object))
             emitCreatedItem(cacheItem, item);
         else if (QQuickPackage *package = qmlobject_cast<QQuickPackage *>(cacheItem->object))
             emitCreatedPackage(cacheItem, package);
     } else if (status == QQmlIncubator::Error) {
+        qmlInfo(q, m_delegate->errors()) << "Error creating delegate";
+    }
+
+    if (!cacheItem->isObjectReferenced()) {
+        if (QQuickItem *item = qmlobject_cast<QQuickItem *>(cacheItem->object))
+            emitDestroyingItem(item);
+        else if (QQuickPackage *package = qmlobject_cast<QQuickPackage *>(cacheItem->object))
+            emitDestroyingPackage(package);
+        delete cacheItem->object;
+        cacheItem->object = 0;
         cacheItem->scriptRef -= 1;
         cacheItem->contextData->destroy();
         cacheItem->contextData = 0;
         if (!cacheItem->isReferenced()) {
-            int cidx = m_cache.indexOf(cacheItem);
-            if (cidx >= 0) {
-                m_compositor.clearFlags(Compositor::Cache, cidx, 1, Compositor::CacheFlag);
-                m_cache.removeAt(cidx);
-            }
+            removeCacheItem(cacheItem);
             delete cacheItem;
-            Q_ASSERT(m_cache.count() == m_compositor.count(Compositor::Cache));
         }
-        releaseIncubator(incubationTask);
-        qmlInfo(q, m_delegate->errors()) << "Error creating delegate";
     }
 }
 
@@ -820,7 +833,7 @@ void QQuickVisualDataModelPrivate::setInitialState(QVDMIncubationTask *incubatio
         emitInitItem(cacheItem, item);
 }
 
-QObject *QQuickVisualDataModelPrivate::object(Compositor::Group group, int index, bool asynchronous, bool reference)
+QObject *QQuickVisualDataModelPrivate::object(Compositor::Group group, int index, bool asynchronous)
 {
     Q_Q(QQuickVisualDataModel);
     if (!m_delegate || index < 0 || index >= m_compositor.count(group)) {
@@ -847,6 +860,11 @@ QObject *QQuickVisualDataModelPrivate::object(Compositor::Group group, int index
         Q_ASSERT(m_cache.count() == m_compositor.count(Compositor::Cache));
     }
 
+    // Bump the reference counts temporarily so neither the content data or the delegate object
+    // are deleted if incubatorStatusChanged() is called synchronously.
+    cacheItem->scriptRef += 1;
+    cacheItem->referenceObject();
+
     if (cacheItem->incubationTask) {
         if (!asynchronous && cacheItem->incubationTask->incubationMode() == QQmlIncubator::Asynchronous) {
             // previously requested async - now needed immediately
@@ -884,9 +902,19 @@ QObject *QQuickVisualDataModelPrivate::object(Compositor::Group group, int index
 
     if (index == m_compositor.count(group) - 1 && m_adaptorModel.canFetchMore())
         QCoreApplication::postEvent(q, new QEvent(QEvent::UpdateRequest));
-    if (cacheItem->object && reference)
-        cacheItem->referenceObject();
-    return cacheItem->object;
+
+    // Remove the temporary reference count.
+    cacheItem->scriptRef -= 1;
+    if (cacheItem->object)
+        return cacheItem->object;
+
+    cacheItem->releaseObject();
+    if (!cacheItem->isReferenced()) {
+        removeCacheItem(cacheItem);
+        delete cacheItem;
+    }
+
+    return 0;
 }
 
 /*
@@ -905,7 +933,7 @@ QQuickItem *QQuickVisualDataModel::item(int index, bool asynchronous)
         return 0;
     }
 
-    QObject *object = d->object(d->m_compositorGroup, index, asynchronous, true);
+    QObject *object = d->object(d->m_compositorGroup, index, asynchronous);
     if (!object)
         return 0;
 
@@ -1707,12 +1735,7 @@ void QQuickVisualDataModelItem::Dispose()
 
     if (metaType->model) {
         QQuickVisualDataModelPrivate *model = QQuickVisualDataModelPrivate::get(metaType->model);
-        const int cacheIndex = model->m_cache.indexOf(this);
-        if (cacheIndex != -1) {
-            model->m_compositor.clearFlags(Compositor::Cache, cacheIndex, 1, Compositor::CacheFlag);
-            model->m_cache.removeAt(cacheIndex);
-            Q_ASSERT(model->m_cache.count() == model->m_compositor.count(Compositor::Cache));
-        }
+        model->removeCacheItem(this);
     }
     delete this;
 }
@@ -2325,12 +2348,13 @@ void QQuickVisualDataGroup::create(QQmlV8Function *args)
         return;
     }
 
-    QObject *object = model->object(group, index, false, false);
+    QObject *object = model->object(group, index, false);
     if (object) {
         QVector<Compositor::Insert> inserts;
         Compositor::iterator it = model->m_compositor.find(group, index);
         model->m_compositor.setFlags(it, 1, d->group, Compositor::PersistedFlag, &inserts);
         model->itemsInserted(inserts);
+        model->m_cache.at(it.cacheIndex)->releaseObject();
     }
 
     args->returnValue(args->engine()->newQObject(object));
@@ -2797,7 +2821,7 @@ QQuickItem *QQuickVisualPartsModel::item(int index, bool asynchronous)
         return 0;
     }
 
-    QObject *object = model->object(m_compositorGroup, index, asynchronous, true);
+    QObject *object = model->object(m_compositorGroup, index, asynchronous);
 
     if (QQuickPackage *package = qmlobject_cast<QQuickPackage *>(object)) {
         QObject *part = package->part(m_part);
diff --git a/src/quick/items/qquickvisualdatamodel_p_p.h b/src/quick/items/qquickvisualdatamodel_p_p.h
index dc730c6105..7389da8c17 100644
--- a/src/quick/items/qquickvisualdatamodel_p_p.h
+++ b/src/quick/items/qquickvisualdatamodel_p_p.h
@@ -244,7 +244,7 @@ public:
     void init();
     void connectModel(QQuickVisualAdaptorModel *model);
 
-    QObject *object(Compositor::Group group, int index, bool asynchronous, bool reference);
+    QObject *object(Compositor::Group group, int index, bool asynchronous);
     QQuickVisualDataModel::ReleaseFlags release(QObject *object);
     QString stringValue(Compositor::Group group, int index, const QString &name);
     void emitCreatedPackage(QQuickVisualDataModelItem *cacheItem, QQuickPackage *package);
@@ -255,6 +255,7 @@ public:
         emit q_func()->initItem(cacheItem->index[m_compositorGroup], item); }
     void emitDestroyingPackage(QQuickPackage *package);
     void emitDestroyingItem(QQuickItem *item) { emit q_func()->destroyingItem(item); }
+    void removeCacheItem(QQuickVisualDataModelItem *cacheItem);
 
     void updateFilterGroup();
 
diff --git a/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp b/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp
index 82c8d44068..2d6cfa02d7 100644
--- a/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp
+++ b/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp
@@ -40,6 +40,7 @@
 ****************************************************************************/
 #include "../../shared/util.h"
 #include "../shared/visualtestutil.h"
+#include "../shared/viewtestutil.h"
 
 #include <qtest.h>
 #include <QtTest/QSignalSpy>
@@ -47,6 +48,7 @@
 #include <QtQml/qqmlcomponent.h>
 #include <QtQml/qqmlcontext.h>
 #include <QtQml/qqmlexpression.h>
+#include <QtQml/qqmlincubator.h>
 #include <QtQuick/qquickview.h>
 #include <private/qquicklistview_p.h>
 #include <QtQuick/private/qquicktext_p.h>
@@ -57,6 +59,7 @@
 #include <math.h>
 
 using namespace QQuickVisualTestUtil;
+using namespace QQuickViewTestUtil;
 
 template <typename T, int N> int lengthOf(const T (&)[N]) { return N; }
 
@@ -186,6 +189,45 @@ private:
     QString m_color;
 };
 
+class ItemRequester : public QObject
+{
+    Q_OBJECT
+public:
+    ItemRequester(QObject *parent = 0)
+        : QObject(parent)
+        , itemInitialized(0)
+        , itemCreated(0)
+        , itemDestroyed(0)
+        , indexInitialized(-1)
+        , indexCreated(-1)
+    {
+    }
+
+    QQuickItem *itemInitialized;
+    QQuickItem *itemCreated;
+    QQuickItem *itemDestroyed;
+    int indexInitialized;
+    int indexCreated;
+
+public Q_SLOTS:
+    void initItem(int index, QQuickItem *item)
+    {
+        itemInitialized = item;
+        indexInitialized = index;
+    }
+
+    void createdItem(int index, QQuickItem *item)
+    {
+        itemCreated = item;
+        indexCreated = index;
+    }
+
+    void destroyingItem(QQuickItem *item)
+    {
+        itemDestroyed = item;
+    }
+};
+
 QML_DECLARE_TYPE(SingleRoleModel)
 QML_DECLARE_TYPE(DataObject)
 #ifndef QT_NO_WIDGETS
@@ -238,6 +280,13 @@ private slots:
     void warnings_data();
     void warnings();
     void invalidAttachment();
+    void asynchronousInsert_data();
+    void asynchronousInsert();
+    void asynchronousRemove_data();
+    void asynchronousRemove();
+    void asynchronousMove();
+    void asynchronousMove_data();
+    void asynchronousCancel();
 
 private:
     template <int N> void groups_verify(
@@ -263,6 +312,7 @@ private:
             const bool (&sMember)[N]);
 
     bool failed;
+    QQmlIncubationController controller;
     QQmlEngine engine;
 };
 
@@ -296,6 +346,8 @@ void tst_qquickvisualdatamodel::initTestCase()
     qmlRegisterType<StandardItem>("tst_qquickvisualdatamodel", 1, 0, "StandardItem");
     qmlRegisterType<StandardItemModel>("tst_qquickvisualdatamodel", 1, 0, "StandardItemModel");
 #endif
+
+    engine.setIncubationController(&controller);
 }
 
 void tst_qquickvisualdatamodel::cleanupTestCase()
@@ -3480,6 +3532,236 @@ void tst_qquickvisualdatamodel::invalidAttachment()
     QVERIFY(!property.value<QQuickVisualDataModel *>());
 }
 
+void tst_qquickvisualdatamodel::asynchronousInsert_data()
+{
+    QTest::addColumn<int>("requestIndex");
+    QTest::addColumn<int>("insertIndex");
+    QTest::addColumn<int>("insertCount");
+    QTest::addColumn<int>("completeIndex");
+
+    QTest::newRow("insert before") << 4 << 1 << 3 << 7;
+    QTest::newRow("insert after")  << 4 << 6 << 3 << 4;
+    QTest::newRow("insert at")     << 4 << 4 << 3 << 7;
+}
+
+void tst_qquickvisualdatamodel::asynchronousInsert()
+{
+    QFETCH(int, requestIndex);
+    QFETCH(int, insertIndex);
+    QFETCH(int, insertCount);
+    QFETCH(int, completeIndex);
+
+    QCOMPARE(controller.incubatingObjectCount(), 0);
+
+    QQmlComponent c(&engine, testFileUrl("visualdatamodel.qml"));
+
+    QmlListModel model;
+    for (int i = 0; i < 8; i++)
+        model.addItem("Original item" + QString::number(i), "");
+
+    engine.rootContext()->setContextProperty("myModel", &model);
+
+    QQuickVisualDataModel *visualModel = qobject_cast<QQuickVisualDataModel*>(c.create());
+    QVERIFY(visualModel);
+
+    ItemRequester requester;
+    connect(visualModel, SIGNAL(initItem(int,QQuickItem*)), &requester, SLOT(initItem(int,QQuickItem*)));
+    connect(visualModel, SIGNAL(createdItem(int,QQuickItem*)), &requester, SLOT(createdItem(int,QQuickItem*)));
+    connect(visualModel, SIGNAL(destroyingItem(QQuickItem*)), &requester, SLOT(destroyingItem(QQuickItem*)));
+
+    QQuickItem *item = visualModel->item(requestIndex, true);
+    QVERIFY(!item);
+
+    QVERIFY(!requester.itemInitialized);
+    QVERIFY(!requester.itemCreated);
+    QVERIFY(!requester.itemDestroyed);
+
+    QList<QPair<QString, QString> > newItems;
+    for (int i = 0; i < insertCount; i++)
+        newItems.append(qMakePair(QLatin1String("New item") + QString::number(i), QString(QLatin1String(""))));
+    model.insertItems(insertIndex, newItems);
+
+    item = visualModel->item(completeIndex, false);
+    QVERIFY(item);
+
+    QCOMPARE(requester.itemInitialized, item);
+    QCOMPARE(requester.indexInitialized, completeIndex);
+    QCOMPARE(requester.itemCreated, item);
+    QCOMPARE(requester.indexCreated, completeIndex);
+    QVERIFY(!requester.itemDestroyed);
+
+    QCOMPARE(controller.incubatingObjectCount(), 0);
+
+    visualModel->release(item);
+
+    QCOMPARE(requester.itemDestroyed, item);
+}
+
+void tst_qquickvisualdatamodel::asynchronousRemove_data()
+{
+    QTest::addColumn<int>("requestIndex");
+    QTest::addColumn<int>("removeIndex");
+    QTest::addColumn<int>("removeCount");
+    QTest::addColumn<int>("completeIndex");
+
+    QTest::newRow("remove before")    << 4 << 1 << 3 << 1;
+    QTest::newRow("remove after")     << 4 << 6 << 2 << 4;
+    QTest::newRow("remove requested") << 4 << 4 << 3 << -1;
+}
+
+void tst_qquickvisualdatamodel::asynchronousRemove()
+{
+    QFETCH(int, requestIndex);
+    QFETCH(int, removeIndex);
+    QFETCH(int, removeCount);
+    QFETCH(int, completeIndex);
+
+    QCOMPARE(controller.incubatingObjectCount(), 0);
+
+    QQmlComponent c(&engine, testFileUrl("visualdatamodel.qml"));
+
+    QmlListModel model;
+    for (int i = 0; i < 8; i++)
+        model.addItem("Original item" + QString::number(i), "");
+
+    engine.rootContext()->setContextProperty("myModel", &model);
+
+    QQuickVisualDataModel *visualModel = qobject_cast<QQuickVisualDataModel*>(c.create());
+    QVERIFY(visualModel);
+
+    ItemRequester requester;
+    connect(visualModel, SIGNAL(initItem(int,QQuickItem*)), &requester, SLOT(initItem(int,QQuickItem*)));
+    connect(visualModel, SIGNAL(createdItem(int,QQuickItem*)), &requester, SLOT(createdItem(int,QQuickItem*)));
+    connect(visualModel, SIGNAL(destroyingItem(QQuickItem*)), &requester, SLOT(destroyingItem(QQuickItem*)));
+
+    QQuickItem *item = visualModel->item(requestIndex, true);
+    QVERIFY(!item);
+
+    QVERIFY(!requester.itemInitialized);
+    QVERIFY(!requester.itemCreated);
+    QVERIFY(!requester.itemDestroyed);
+
+    model.removeItems(removeIndex, removeCount);
+
+    if (completeIndex == -1) {
+        QElapsedTimer timer;
+        timer.start();
+        do {
+            controller.incubateFor(50);
+        } while (timer.elapsed() < 1000 && controller.incubatingObjectCount() > 0);
+
+        QVERIFY(requester.itemInitialized);
+        QCOMPARE(requester.itemCreated, requester.itemInitialized);
+        QCOMPARE(requester.itemDestroyed, requester.itemInitialized);
+    } else {
+        item = visualModel->item(completeIndex, false);
+        QVERIFY(item);
+
+        QCOMPARE(requester.itemInitialized, item);
+        QCOMPARE(requester.indexInitialized, completeIndex);
+        QCOMPARE(requester.itemCreated, item);
+        QCOMPARE(requester.indexCreated, completeIndex);
+        QVERIFY(!requester.itemDestroyed);
+
+        QCOMPARE(controller.incubatingObjectCount(), 0);
+
+        visualModel->release(item);
+
+        QCOMPARE(requester.itemDestroyed, item);
+    }
+}
+
+void tst_qquickvisualdatamodel::asynchronousMove_data()
+{
+    QTest::addColumn<int>("requestIndex");
+    QTest::addColumn<int>("from");
+    QTest::addColumn<int>("to");
+    QTest::addColumn<int>("count");
+    QTest::addColumn<int>("completeIndex");
+
+    QTest::newRow("move before")          << 4 << 1 << 0 << 3 << 4;
+    QTest::newRow("move after")           << 4 << 6 << 5 << 2 << 4;
+    QTest::newRow("move requested")       << 4 << 4 << 3 << 2 << 3;
+    QTest::newRow("move before to after") << 4 << 1 << 4 << 3 << 1;
+    QTest::newRow("move after to before") << 4 << 6 << 2 << 2 << 6;
+}
+
+void tst_qquickvisualdatamodel::asynchronousMove()
+{
+    QFETCH(int, requestIndex);
+    QFETCH(int, from);
+    QFETCH(int, to);
+    QFETCH(int, count);
+    QFETCH(int, completeIndex);
+
+    QCOMPARE(controller.incubatingObjectCount(), 0);
+
+    QQmlComponent c(&engine, testFileUrl("visualdatamodel.qml"));
+
+    QmlListModel model;
+    for (int i = 0; i < 8; i++)
+        model.addItem("Original item" + QString::number(i), "");
+
+    engine.rootContext()->setContextProperty("myModel", &model);
+
+    QQuickVisualDataModel *visualModel = qobject_cast<QQuickVisualDataModel*>(c.create());
+    QVERIFY(visualModel);
+
+    ItemRequester requester;
+    connect(visualModel, SIGNAL(initItem(int,QQuickItem*)), &requester, SLOT(initItem(int,QQuickItem*)));
+    connect(visualModel, SIGNAL(createdItem(int,QQuickItem*)), &requester, SLOT(createdItem(int,QQuickItem*)));
+    connect(visualModel, SIGNAL(destroyingItem(QQuickItem*)), &requester, SLOT(destroyingItem(QQuickItem*)));
+
+    QQuickItem *item = visualModel->item(requestIndex, true);
+    QVERIFY(!item);
+
+    QVERIFY(!requester.itemInitialized);
+    QVERIFY(!requester.itemCreated);
+    QVERIFY(!requester.itemDestroyed);
+
+    model.moveItems(from, to, count);
+
+    item = visualModel->item(completeIndex, false);
+    QVERIFY(item);
+
+
+    QCOMPARE(requester.itemInitialized, item);
+    QCOMPARE(requester.indexInitialized, completeIndex);
+    QCOMPARE(requester.itemCreated, item);
+    QCOMPARE(requester.indexCreated, completeIndex);
+    QVERIFY(!requester.itemDestroyed);
+
+    QCOMPARE(controller.incubatingObjectCount(), 0);
+
+    visualModel->release(item);
+
+    QCOMPARE(requester.itemDestroyed, item);
+}
+
+void tst_qquickvisualdatamodel::asynchronousCancel()
+{
+    const int requestIndex = 4;
+
+    QCOMPARE(controller.incubatingObjectCount(), 0);
+
+    QQmlComponent c(&engine, testFileUrl("visualdatamodel.qml"));
+
+    QmlListModel model;
+    for (int i = 0; i < 8; i++)
+        model.addItem("Original item" + QString::number(i), "");
+
+    engine.rootContext()->setContextProperty("myModel", &model);
+
+    QQuickVisualDataModel *visualModel = qobject_cast<QQuickVisualDataModel*>(c.create());
+    QVERIFY(visualModel);
+
+    QQuickItem *item = visualModel->item(requestIndex, true);
+    QVERIFY(!item);
+    QCOMPARE(controller.incubatingObjectCount(), 1);
+
+    visualModel->cancel(requestIndex);
+    QCOMPARE(controller.incubatingObjectCount(), 0);
+}
 
 QTEST_MAIN(tst_qquickvisualdatamodel)