Fix interaction of garbage collector with JS objects during QML type instantiation

It may happen that during the lengthy process of instantiating a tree of
objects for QML, the garbage collector runs.

For objects created by QML we support different ownership models, for example
in QtQuick visual parents keep their visual children alive, despite perhaps a
lack of QObject parentship. That ownership becomes active once the QML
autoparent function has assigned the correct visual parent, which happens after
object instantiation (after QQmlObjectCreator).

Similarly when a composite type is created, its QObject parent is only set
after all properties have been set. The root QObject is kept alive through a
special boolean, but if the sub-objects aren't children yet, their JS wrapper
might get deleted. For composite types with var properties, that also means
their var properties get deleted, such as the model property of TableView.qml
in the bug report.

In the future we want to support creating QWidget hierarchies with QML, which
also for layouts may rely on a delayed parent assignment for layouts.

To accommodate all this, this patch introduces an array on the JS stack that
keeps track of all JS wrappers for all QObjects created. This array is alive
during object tree creation. Afterwards, the different ownership models take
over, for example the auto parent function assigning a visual parent.

This patch also fixes an off-by-one in the total object count calculation
for composite types, where when instantiating a composite type as a sub-object
we counted the sub composite's object count but forgot the object itself.

Task-number: QTBUG-38835
Task-number: QTBUG-39966
Change-Id: I6104b2434510642081e0c54793ed296adeca7481
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

3 files changed, 133 insertions, 0 deletions

diff --git a/tests/auto/qml/qqmlecmascript/testtypes.cpp b/tests/auto/qml/qqmlecmascript/testtypes.cpp
index eb06b9e57d..560d86005c 100644
--- a/tests/auto/qml/qqmlecmascript/testtypes.cpp
+++ b/tests/auto/qml/qqmlecmascript/testtypes.cpp
@@ -300,6 +300,62 @@ static QObject *create_singletonWithEnum(QQmlEngine *, QJSEngine *)
     return new SingletonWithEnum;
 }
 
+QObjectContainer::QObjectContainer()
+    : widgetParent(0)
+    , gcOnAppend(false)
+{}
+
+QQmlListProperty<QObject> QObjectContainer::data()
+{
+    return QQmlListProperty<QObject>(this, 0, children_append, children_count, children_at, children_clear);
+}
+
+void QObjectContainer::children_append(QQmlListProperty<QObject> *prop, QObject *o)
+{
+    QObjectContainer *that = static_cast<QObjectContainer*>(prop->object);
+    that->dataChildren.append(o);
+    QObject::connect(o, SIGNAL(destroyed(QObject*)), prop->object, SLOT(childDestroyed(QObject*)));
+
+    if (that->gcOnAppend) {
+        QQmlEngine *engine = qmlEngine(that);
+        engine->collectGarbage();
+        QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
+        QCoreApplication::processEvents();
+    }
+}
+
+int QObjectContainer::children_count(QQmlListProperty<QObject> *prop)
+{
+    return static_cast<QObjectContainer*>(prop->object)->dataChildren.count();
+}
+
+QObject *QObjectContainer::children_at(QQmlListProperty<QObject> *prop, int index)
+{
+    return static_cast<QObjectContainer*>(prop->object)->dataChildren.at(index);
+}
+
+void QObjectContainer::children_clear(QQmlListProperty<QObject> *prop)
+{
+    QObjectContainer *that = static_cast<QObjectContainer*>(prop->object);
+    foreach (QObject *c, that->dataChildren)
+        QObject::disconnect(c, SIGNAL(destroyed(QObject*)), that, SLOT(childDestroyed(QObject*)));
+    that->dataChildren.clear();
+}
+
+void QObjectContainer::childDestroyed(QObject *child) {
+    dataChildren.removeAll(child);
+}
+
+void FloatingQObject::classBegin()
+{
+    setParent(0);
+}
+
+void FloatingQObject::componentComplete()
+{
+    Q_ASSERT(!parent());
+}
+
 void registerTypes()
 {
     qmlRegisterType<MyQmlObject>("Qt.test", 1,0, "MyQmlObjectAlias");
@@ -381,6 +437,10 @@ void registerTypes()
     qmlRegisterSingletonType<testImportOrderApi>("Qt.test.importOrderApi2",1,0,"Data",testImportOrder_api2);
 
     qmlRegisterSingletonType<SingletonWithEnum>("Qt.test.singletonWithEnum", 1, 0, "SingletonWithEnum", create_singletonWithEnum);
+
+    qmlRegisterType<QObjectContainer>("Qt.test", 1, 0, "QObjectContainer");
+    qmlRegisterType<QObjectContainerWithGCOnAppend>("Qt.test", 1, 0, "QObjectContainerWithGCOnAppend");
+    qmlRegisterType<FloatingQObject>("Qt.test", 1, 0, "FloatingQObject");
 }
 
 #include "testtypes.moc"
diff --git a/tests/auto/qml/qqmlecmascript/testtypes.h b/tests/auto/qml/qqmlecmascript/testtypes.h
index 928d594f62..d5a1220f23 100644
--- a/tests/auto/qml/qqmlecmascript/testtypes.h
+++ b/tests/auto/qml/qqmlecmascript/testtypes.h
@@ -1661,6 +1661,51 @@ public:
     };
 };
 
+// Like QtObject, but with default property
+class QObjectContainer : public QObject
+{
+    Q_OBJECT
+    Q_CLASSINFO("DefaultProperty", "data")
+    Q_PROPERTY(QQmlListProperty<QObject> data READ data DESIGNABLE false)
+public:
+    QObjectContainer();
+
+    QQmlListProperty<QObject> data();
+
+    static void children_append(QQmlListProperty<QObject> *prop, QObject *o);
+    static int children_count(QQmlListProperty<QObject> *prop);
+    static QObject *children_at(QQmlListProperty<QObject> *prop, int index);
+    static void children_clear(QQmlListProperty<QObject> *prop);
+
+    QList<QObject*> dataChildren;
+    QWidget *widgetParent;
+    bool gcOnAppend;
+
+protected slots:
+    void childDestroyed(QObject *child);
+};
+
+class QObjectContainerWithGCOnAppend : public QObjectContainer
+{
+    Q_OBJECT
+public:
+    QObjectContainerWithGCOnAppend()
+    {
+        gcOnAppend = true;
+    }
+};
+
+class FloatingQObject : public QObject, public QQmlParserStatus
+{
+    Q_OBJECT
+    Q_INTERFACES(QQmlParserStatus)
+public:
+    FloatingQObject() {}
+
+    virtual void classBegin();
+    virtual void componentComplete();
+};
+
 void registerTypes();
 
 #endif // TESTTYPES_H
diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index a1e36b42e6..a9486a8e63 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -322,6 +322,7 @@ private slots:
     void varPropertyAccessOnObjectWithInvalidContext();
     void importedScriptsAccessOnObjectWithInvalidContext();
     void contextObjectOnLazyBindings();
+    void garbageCollectionDuringCreation();
 
 private:
 //    static void propertyVarWeakRefCallback(v8::Persistent<v8::Value> object, void* parameter);
@@ -7603,6 +7604,33 @@ void tst_qqmlecmascript::contextObjectOnLazyBindings()
     QCOMPARE(subObject->property("testValue").toInt(), int(42));
 }
 
+void tst_qqmlecmascript::garbageCollectionDuringCreation()
+{
+    QQmlComponent component(&engine);
+    component.setData("import Qt.test 1.0\n"
+                      "QObjectContainerWithGCOnAppend {\n"
+                      "    objectName: \"root\"\n"
+                      "    FloatingQObject {\n"
+                      "        objectName: \"parentLessChild\"\n"
+                      "        property var blah;\n" // Ensure we have JS wrapper
+                      "    }\n"
+                      "}\n",
+                      QUrl());
+
+    QScopedPointer<QObject> object(component.create());
+    QVERIFY(!object.isNull());
+
+    QObjectContainer *container = qobject_cast<QObjectContainer*>(object.data());
+    QCOMPARE(container->dataChildren.count(), 1);
+
+    QObject *child = container->dataChildren.first();
+    QQmlData *ddata = QQmlData::get(child);
+    QVERIFY(!ddata->jsWrapper.isNullOrUndefined());
+
+    gc(engine);
+    QCOMPARE(container->dataChildren.count(), 0);
+}
+
 QTEST_MAIN(tst_qqmlecmascript)
 
 #include "tst_qqmlecmascript.moc"