From b2ee7364be2f11dc16b80a85141cfdd54e801b10 Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Tue, 29 Jun 2021 09:36:58 +0200 Subject: [PATCH] QQmlListModel: Guard QObject pointers in the list If a QObject in the list is deleted from elsewhere, we need to zero it. Amends commit 3edac1d0a70961e6f86564cc0a98339df6ac5964. Fixes: QTBUG-94833 Pick-to: 6.2 Change-Id: Ic33a5a0baa160824a34f353806dbfd876fa7123a Reviewed-by: Maximilian Goldstein Reviewed-by: Paul Olav Tvete Reviewed-by: Qt CI Bot --- src/qmlmodels/qqmllistmodel.cpp | 49 ++++++++++--------- src/qmlmodels/qqmllistmodel_p_p.h | 48 +++++++++++++++--- .../qml/qqmllistmodel/tst_qqmllistmodel.cpp | 27 ++++++++++ 3 files changed, 93 insertions(+), 31 deletions(-) diff --git a/src/qmlmodels/qqmllistmodel.cpp b/src/qmlmodels/qqmllistmodel.cpp index 833f102e79..d983d7f88e 100644 --- a/src/qmlmodels/qqmllistmodel.cpp +++ b/src/qmlmodels/qqmllistmodel.cpp @@ -129,7 +129,7 @@ const ListLayout::Role &ListLayout::getRoleOrCreate(QV4::String *key, Role::Data const ListLayout::Role &ListLayout::createRole(const QString &key, ListLayout::Role::DataType type) { - const int dataSizes[] = { sizeof(StringOrTranslation), sizeof(double), sizeof(bool), sizeof(ListModel *), sizeof(QPointer), sizeof(QVariantMap), sizeof(QDateTime), sizeof(QUrl), sizeof(QJSValue) }; + const int dataSizes[] = { sizeof(StringOrTranslation), sizeof(double), sizeof(bool), sizeof(ListModel *), sizeof(ListElement::GuardedQObjectPointer), sizeof(QVariantMap), sizeof(QDateTime), sizeof(QUrl), sizeof(QJSValue) }; const int dataAlignments[] = { alignof(StringOrTranslation), alignof(double), alignof(bool), alignof(ListModel *), alignof(QObject *), alignof(QVariantMap), alignof(QDateTime), alignof(QUrl), alignof(QJSValue) }; Role *r = new Role; @@ -845,7 +845,8 @@ StringOrTranslation *ListElement::getStringProperty(const ListLayout::Role &role QObject *ListElement::getQObjectProperty(const ListLayout::Role &role) { char *mem = getPropertyMemory(role); - QPointer *o = reinterpret_cast *>(mem); + GuardedQObjectPointer *o + = reinterpret_cast(mem); return o->data(); } @@ -893,13 +894,13 @@ QJSValue *ListElement::getFunctionProperty(const ListLayout::Role &role) return f; } -QTaggedPointer * +ListElement::GuardedQObjectPointer * ListElement::getGuardProperty(const ListLayout::Role &role) { char *mem = getPropertyMemory(role); bool existingGuard = false; - for (size_t i = 0; i < sizeof(QTaggedPointer); + for (size_t i = 0; i < sizeof(GuardedQObjectPointer); ++i) { if (mem[i] != 0) { existingGuard = true; @@ -907,10 +908,10 @@ ListElement::getGuardProperty(const ListLayout::Role &role) } } - QTaggedPointer *o = nullptr; + GuardedQObjectPointer *o = nullptr; if (existingGuard) - o = reinterpret_cast *>(mem); + o = reinterpret_cast(mem); return o; } @@ -966,8 +967,8 @@ QVariant ListElement::getProperty(const ListLayout::Role &role, const QQmlListMo break; case ListLayout::Role::QObject: { - QTaggedPointer *guard = - reinterpret_cast *>( + GuardedQObjectPointer *guard = + reinterpret_cast( mem); QObject *object = guard->data(); if (object) @@ -1084,16 +1085,18 @@ int ListElement::setListProperty(const ListLayout::Role &role, ListModel *m) } static void -restoreQObjectOwnership(QTaggedPointer *pointer) +restoreQObjectOwnership(ListElement::GuardedQObjectPointer *pointer) { - QQmlData *data = QQmlData::get(pointer->data(), false); - Q_ASSERT(data); + 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); - data->explicitIndestructibleSet = (pointer->tag() & ListElement::ExplicitlySet); + // Only restore the previous state if the object hasn't become explicitly + // owned + if (!data->explicitIndestructibleSet) { + data->indestructible = (pointer->tag() & ListElement::Indestructible); + data->explicitIndestructibleSet = (pointer->tag() & ListElement::ExplicitlySet); + } } } @@ -1110,7 +1113,7 @@ static void setQObjectOwnership(char *mem, QObject *o) ddata->indestructible = true; ddata->explicitIndestructibleSet = false; - new (mem) QTaggedPointer( + new (mem) ListElement::GuardedQObjectPointer( o, static_cast(ownership)); } @@ -1120,10 +1123,10 @@ int ListElement::setQObjectProperty(const ListLayout::Role &role, QObject *o) if (role.type == ListLayout::Role::QObject) { char *mem = getPropertyMemory(role); - QTaggedPointer *g = - reinterpret_cast *>(mem); + GuardedQObjectPointer *g = + reinterpret_cast(mem); bool existingGuard = false; - for (size_t i = 0; i < sizeof(QTaggedPointer); + for (size_t i = 0; i < sizeof(GuardedQObjectPointer); ++i) { if (mem[i] != 0) { existingGuard = true; @@ -1135,7 +1138,7 @@ int ListElement::setQObjectProperty(const ListLayout::Role &role, QObject *o) changed = g->data() != o; if (changed) restoreQObjectOwnership(g); - g->~QTaggedPointer(); + g->~GuardedQObjectPointer(); } else { changed = true; } @@ -1452,13 +1455,13 @@ void ListElement::destroy(ListLayout *layout) break; case ListLayout::Role::QObject: { - QTaggedPointer *guard = + GuardedQObjectPointer *guard = getGuardProperty(r); if (guard) { restoreQObjectOwnership(guard); - guard->~QTaggedPointer(); + guard->~GuardedQObjectPointer(); } } break; diff --git a/src/qmlmodels/qqmllistmodel_p_p.h b/src/qmlmodels/qqmllistmodel_p_p.h index 7f41960ae3..6c40934c04 100644 --- a/src/qmlmodels/qqmllistmodel_p_p.h +++ b/src/qmlmodels/qqmllistmodel_p_p.h @@ -278,6 +278,45 @@ private: class ListElement { 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); @@ -285,13 +324,6 @@ public: static QVector sync(ListElement *src, ListLayout *srcLayout, ListElement *target, ListLayout *targetLayout); - enum - { - BLOCK_SIZE = 64 - sizeof(int) - sizeof(ListElement *) - sizeof(ModelNodeMetaObject *) - }; - - enum ObjectIndestructible { Indestructible = 1, ExplicitlySet = 2 }; - private: void destroy(ListLayout *layout); @@ -328,7 +360,7 @@ private: ListModel *getListProperty(const ListLayout::Role &role); StringOrTranslation *getStringProperty(const ListLayout::Role &role); QObject *getQObjectProperty(const ListLayout::Role &role); - QTaggedPointer *getGuardProperty(const ListLayout::Role &role); + GuardedQObjectPointer *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/tst_qqmllistmodel.cpp b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp index 812f37e7dd..e372db271e 100644 --- a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp +++ b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp @@ -133,6 +133,7 @@ private slots: void undefinedAppendShouldCauseError(); void nullPropertyCrash(); void objectDestroyed(); + void destroyObject(); }; bool tst_qqmllistmodel::compareVariantList(const QVariantList &testList, QVariant object) @@ -1788,6 +1789,32 @@ void tst_qqmllistmodel::objectDestroyed() QTRY_VERIFY(destroyed); } +void tst_qqmllistmodel::destroyObject() +{ + QQmlEngine engine; + QQmlComponent component(&engine); + component.setData( + R"(import QtQuick + ListModel { + id: model + Component.onCompleted: { model.append({"a": contextObject}); } + })", + QUrl()); + QVERIFY2(component.isReady(), qPrintable(component.errorString())); + QScopedPointer element(new QObject); + engine.rootContext()->setContextProperty(u"contextObject"_qs, element.data()); + + QScopedPointer o(component.create()); + QVERIFY(!o.isNull()); + + QQmlListModel *model = qobject_cast(o.data()); + QVERIFY(model); + QCOMPARE(model->count(), 1); + QCOMPARE(model->get(0).property("a").toQObject(), element.data()); + element.reset(); + QCOMPARE(model->get(0).property("a").toQObject(), nullptr); +} + QTEST_MAIN(tst_qqmllistmodel) #include "tst_qqmllistmodel.moc"