From 718f3469f693eb179f1504a41b18280656a2325d Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Fri, 27 Jan 2023 13:55:46 +0100 Subject: ListModel: Use PersistentValue to keep track of objects This is the simplest way to prevent the garbage collector from destroying the object while at the same time allowing a manual destruction. Pick-to: 6.5 6.4 6.2 Task-number: QTBUG-95895 Task-number: QTBUG-96167 Fixes: QTBUG-91390 Change-Id: Ic3f3146bc555991068ce3367971e050f745d235d Reviewed-by: Fabian Kosmale --- src/qmlmodels/qqmllistmodel.cpp | 115 +++++---------------- src/qmlmodels/qqmllistmodel_p_p.h | 45 +------- .../qqmllistmodel/data/protectQObjectFromGC.qml | 21 ++++ tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp | 22 +++- 4 files changed, 71 insertions(+), 132 deletions(-) create mode 100644 tests/auto/qml/qqmllistmodel/data/protectQObjectFromGC.qml diff --git a/src/qmlmodels/qqmllistmodel.cpp b/src/qmlmodels/qqmllistmodel.cpp index 593880f3c9..7a24faed74 100644 --- a/src/qmlmodels/qqmllistmodel.cpp +++ b/src/qmlmodels/qqmllistmodel.cpp @@ -98,7 +98,7 @@ const ListLayout::Role &ListLayout::createRole(const QString &key, ListLayout::R sizeof(double), sizeof(bool), sizeof(ListModel *), - sizeof(ListElement::GuardedQObjectPointer), + sizeof(QV4::PersistentValue), sizeof(QVariantMap), sizeof(QDateTime), sizeof(QUrl), @@ -618,10 +618,9 @@ void ListModel::set(int elementIndex, QV4::Object *object, QVector *roles) roleIndex = e->setFunctionProperty(r, jsv); } else if (QV4::Object *o = propertyValue->as()) { if (QV4::QObjectWrapper *wrapper = o->as()) { - QObject *o = wrapper->object(); const ListLayout::Role &role = m_layout->getRoleOrCreate(propertyName, ListLayout::Role::QObject); if (role.type == ListLayout::Role::QObject) - roleIndex = e->setQObjectProperty(role, o); + roleIndex = e->setQObjectProperty(role, wrapper); } else if (QVariant maybeUrl = QV4::ExecutionEngine::toVariant( o->asReturnedValue(), QMetaType::fromType(), true); maybeUrl.metaType() == QMetaType::fromType()) { @@ -710,10 +709,9 @@ void ListModel::set(int elementIndex, QV4::Object *object, ListModel::SetElement } } else if (QV4::Object *o = propertyValue->as()) { if (QV4::QObjectWrapper *wrapper = o->as()) { - QObject *o = wrapper->object(); const ListLayout::Role &r = m_layout->getRoleOrCreate(propertyName, ListLayout::Role::QObject); if (r.type == ListLayout::Role::QObject) - e->setQObjectPropertyFast(r, o); + e->setQObjectPropertyFast(r, wrapper); } else { QVariant maybeUrl = QV4::ExecutionEngine::toVariant( o->asReturnedValue(), QMetaType::fromType(), true); @@ -838,12 +836,11 @@ StringOrTranslation *ListElement::getStringProperty(const ListLayout::Role &role return s; } -QObject *ListElement::getQObjectProperty(const ListLayout::Role &role) +QV4::QObjectWrapper *ListElement::getQObjectProperty(const ListLayout::Role &role) { char *mem = getPropertyMemory(role); - GuardedQObjectPointer *o - = reinterpret_cast(mem); - return o->data(); + QV4::PersistentValue *g = reinterpret_cast(mem); + return g->as(); } QVariantMap *ListElement::getVariantMapProperty(const ListLayout::Role &role) @@ -890,13 +887,13 @@ QJSValue *ListElement::getFunctionProperty(const ListLayout::Role &role) return f; } -ListElement::GuardedQObjectPointer * +QV4::PersistentValue * ListElement::getGuardProperty(const ListLayout::Role &role) { char *mem = getPropertyMemory(role); bool existingGuard = false; - for (size_t i = 0; i < sizeof(GuardedQObjectPointer); + for (size_t i = 0; i < sizeof(QV4::PersistentValue); ++i) { if (mem[i] != 0) { existingGuard = true; @@ -904,12 +901,12 @@ ListElement::getGuardProperty(const ListLayout::Role &role) } } - GuardedQObjectPointer *o = nullptr; + QV4::PersistentValue *g = nullptr; if (existingGuard) - o = reinterpret_cast(mem); + g = reinterpret_cast(mem); - return o; + return g; } ListModel *ListElement::getListProperty(const ListLayout::Role &role) @@ -965,12 +962,8 @@ QVariant ListElement::getProperty(const ListLayout::Role &role, const QQmlListMo break; case ListLayout::Role::QObject: { - GuardedQObjectPointer *guard = - reinterpret_cast( - mem); - QObject *object = guard->data(); - if (object) - data = QVariant::fromValue(object); + QV4::PersistentValue *guard = reinterpret_cast(mem); + data = QVariant::fromValue(guard->as()->object()); } break; case ListLayout::Role::VariantMap: @@ -1082,67 +1075,17 @@ int ListElement::setListProperty(const ListLayout::Role &role, ListModel *m) return roleIndex; } -static void -restoreQObjectOwnership(ListElement::GuardedQObjectPointer *pointer) -{ - if (QObject *o = pointer->data()) { - QQmlData *data = QQmlData::get(o, false); - Q_ASSERT(data); - - // Only restore the previous state if the object hasn't become explicitly - // owned - if (!data->explicitIndestructibleSet) - data->indestructible = (pointer->tag() & ListElement::Indestructible); - } -} - -static void setQObjectOwnership(char *mem, QObject *o) -{ - QQmlData *ddata = QQmlData::get(o, false); - const int ownership = (!ddata || ddata->indestructible ? ListElement::Indestructible : 0) - | (ddata && ddata->explicitIndestructibleSet ? ListElement::ExplicitlySet : 0); - - // If ddata didn't exist above, force its creation now - if (!ddata) - ddata = QQmlData::get(o, true); - - if (!ddata->explicitIndestructibleSet) - ddata->indestructible = ownership != 0; - - new (mem) ListElement::GuardedQObjectPointer( - o, static_cast(ownership)); -} - -int ListElement::setQObjectProperty(const ListLayout::Role &role, QObject *o) +int ListElement::setQObjectProperty(const ListLayout::Role &role, QV4::QObjectWrapper *o) { int roleIndex = -1; if (role.type == ListLayout::Role::QObject) { char *mem = getPropertyMemory(role); - GuardedQObjectPointer *g = - reinterpret_cast(mem); - bool existingGuard = false; - for (size_t i = 0; i < sizeof(GuardedQObjectPointer); - ++i) { - if (mem[i] != 0) { - existingGuard = true; - break; - } - } - bool changed; - if (existingGuard) { - changed = g->data() != o; - if (changed) - restoreQObjectOwnership(g); - g->~GuardedQObjectPointer(); - } else { - changed = true; - } - - setQObjectOwnership(mem, o); - - if (changed) - roleIndex = role.index; + if (isMemoryUsed(mem)) + reinterpret_cast(mem)->set(o->engine(), *o); + else + new (mem) QV4::PersistentValue(o->engine(), o); + roleIndex = role.index; } return roleIndex; @@ -1275,11 +1218,10 @@ void ListElement::setBoolPropertyFast(const ListLayout::Role &role, bool b) *value = b; } -void ListElement::setQObjectPropertyFast(const ListLayout::Role &role, QObject *o) +void ListElement::setQObjectPropertyFast(const ListLayout::Role &role, QV4::QObjectWrapper *o) { char *mem = getPropertyMemory(role); - - setQObjectOwnership(mem, o); + new (mem) QV4::PersistentValue(o->engine(), o); } void ListElement::setListPropertyFast(const ListLayout::Role &role, ListModel *m) @@ -1396,7 +1338,7 @@ QVector ListElement::sync(ListElement *src, ListLayout *srcLayout, ListElem break; case ListLayout::Role::QObject: { - QObject *object = src->getQObjectProperty(srcRole); + QV4::QObjectWrapper *object = src->getQObjectProperty(srcRole); roleIndex = target->setQObjectProperty(targetRole, object); } break; @@ -1451,14 +1393,8 @@ void ListElement::destroy(ListLayout *layout) break; case ListLayout::Role::QObject: { - GuardedQObjectPointer *guard = - getGuardProperty(r); - - if (guard) { - restoreQObjectOwnership(guard); - - guard->~GuardedQObjectPointer(); - } + if (QV4::PersistentValue *guard = getGuardProperty(r)) + guard->~PersistentValue(); } break; case ListLayout::Role::VariantMap: @@ -1595,8 +1531,7 @@ int ListElement::setJsProperty(const ListLayout::Role &role, const QV4::Value &d QV4::ScopedObject o(scope, d); QV4::QObjectWrapper *wrapper = o->as(); if (role.type == ListLayout::Role::QObject && wrapper) { - QObject *o = wrapper->object(); - roleIndex = setQObjectProperty(role, o); + roleIndex = setQObjectProperty(role, wrapper); } else if (role.type == ListLayout::Role::VariantMap) { roleIndex = setVariantMapProperty(role, o); } else if (role.type == ListLayout::Role::Url) { diff --git a/src/qmlmodels/qqmllistmodel_p_p.h b/src/qmlmodels/qqmllistmodel_p_p.h index 4874f0deaa..662a12f9e7 100644 --- a/src/qmlmodels/qqmllistmodel_p_p.h +++ b/src/qmlmodels/qqmllistmodel_p_p.h @@ -245,43 +245,6 @@ public: enum ObjectIndestructible { Indestructible = 1, ExplicitlySet = 2 }; enum { BLOCK_SIZE = 64 - sizeof(int) - sizeof(ListElement *) - sizeof(ModelNodeMetaObject *) }; - // This is a basic guarded QObject pointer, with tag. It cannot be copied or moved. - class GuardedQObjectPointer - { - Q_DISABLE_COPY_MOVE(GuardedQObjectPointer) - - using RefCountData = QtSharedPointer::ExternalRefCountData; - using Storage = QTaggedPointer; - - public: - GuardedQObjectPointer(QObject *o, ObjectIndestructible ownership) - : storage(o, ownership) - , refCount(o ? RefCountData::getAndRef(o) : nullptr) - {} - - ~GuardedQObjectPointer() - { - if (refCount && !refCount->weakref.deref()) - delete refCount; - } - - QObject *data() const - { - return (refCount == nullptr || refCount->strongref.loadRelaxed() == 0) - ? nullptr - : storage.data(); - } - - ObjectIndestructible tag() const - { - return storage.tag(); - } - - private: - Storage storage; - RefCountData *refCount = nullptr; - }; - ListElement(); ListElement(int existingUid); ~ListElement(); @@ -300,7 +263,7 @@ private: int setDoubleProperty(const ListLayout::Role &role, double n); int setBoolProperty(const ListLayout::Role &role, bool b); int setListProperty(const ListLayout::Role &role, ListModel *m); - int setQObjectProperty(const ListLayout::Role &role, QObject *o); + int setQObjectProperty(const ListLayout::Role &role, QV4::QObjectWrapper *o); int setVariantMapProperty(const ListLayout::Role &role, QV4::Object *o); int setVariantMapProperty(const ListLayout::Role &role, QVariantMap *m); int setDateTimeProperty(const ListLayout::Role &role, const QDateTime &dt); @@ -311,7 +274,7 @@ private: void setStringPropertyFast(const ListLayout::Role &role, const QString &s); void setDoublePropertyFast(const ListLayout::Role &role, double n); void setBoolPropertyFast(const ListLayout::Role &role, bool b); - void setQObjectPropertyFast(const ListLayout::Role &role, QObject *o); + void setQObjectPropertyFast(const ListLayout::Role &role, QV4::QObjectWrapper *o); void setListPropertyFast(const ListLayout::Role &role, ListModel *m); void setVariantMapFast(const ListLayout::Role &role, QV4::Object *o); void setDateTimePropertyFast(const ListLayout::Role &role, const QDateTime &dt); @@ -323,8 +286,8 @@ private: QVariant getProperty(const ListLayout::Role &role, const QQmlListModel *owner, QV4::ExecutionEngine *eng); ListModel *getListProperty(const ListLayout::Role &role); StringOrTranslation *getStringProperty(const ListLayout::Role &role); - QObject *getQObjectProperty(const ListLayout::Role &role); - GuardedQObjectPointer *getGuardProperty(const ListLayout::Role &role); + QV4::QObjectWrapper *getQObjectProperty(const ListLayout::Role &role); + QV4::PersistentValue *getGuardProperty(const ListLayout::Role &role); QVariantMap *getVariantMapProperty(const ListLayout::Role &role); QDateTime *getDateTimeProperty(const ListLayout::Role &role); QUrl *getUrlProperty(const ListLayout::Role &role); diff --git a/tests/auto/qml/qqmllistmodel/data/protectQObjectFromGC.qml b/tests/auto/qml/qqmllistmodel/data/protectQObjectFromGC.qml new file mode 100644 index 0000000000..43b375b681 --- /dev/null +++ b/tests/auto/qml/qqmllistmodel/data/protectQObjectFromGC.qml @@ -0,0 +1,21 @@ +import QtQml +import QtQml.Models + +ListModel { + id: filesModel + property Component testComponent: Component { + id: testComponent + QtObject { + required property string name + } + } + Component.onCompleted: { + filesModel.clear() + for(let i = 0; i < 10; i++) { + filesModel.append({ + path: testComponent.createObject(null, { name: "" + i }) + }) + } + gc() + } +} diff --git a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp index 8b07398832..e522f280a3 100644 --- a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp +++ b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp @@ -118,6 +118,7 @@ private slots: void destroyComponentObject(); void objectOwnershipFlip(); void enumsInListElement(); + void protectQObjectFromGC(); }; bool tst_qqmllistmodel::compareVariantList(const QVariantList &testList, QVariant object) @@ -1855,7 +1856,7 @@ void tst_qqmllistmodel::destroyComponentObject() Q_RETURN_ARG(QVariant, retVal)); QVERIFY(retVal.toBool()); QTRY_VERIFY(created.isNull()); - QTRY_VERIFY(list->get(0).property("obj").isUndefined()); + QTRY_VERIFY(list->get(0).property("obj").isNull()); QCOMPARE(list->count(), 1); } @@ -1909,6 +1910,25 @@ void tst_qqmllistmodel::enumsInListElement() } } +void tst_qqmllistmodel::protectQObjectFromGC() +{ + QQmlEngine engine; + QQmlComponent component(&engine, testFileUrl("protectQObjectFromGC.qml")); + QVERIFY2(component.isReady(), qPrintable(component.errorString())); + QScopedPointer root(component.create()); + QVERIFY(!root.isNull()); + + QQmlListModel *listModel = qobject_cast(root.data()); + QVERIFY(listModel); + QCOMPARE(listModel->count(), 10); + + for (int i = 0; i < 10; ++i) { + QObject *element = qjsvalue_cast(listModel->get(i).property("path")); + QVERIFY(element); + QCOMPARE(element->property("name").toString(), QString::number(i)); + } +} + QTEST_MAIN(tst_qqmllistmodel) #include "tst_qqmllistmodel.moc" -- cgit v1.2.3