Cleanup QQmlGuard and related classes

An investigation of uses of QQmlGuard (+ related classes) yielded the
following results:

- objectDestroyed is the only virtual method of QQmlGuard (besides the
  dtor)
- we never have an owning pointer of type QQmlGuard * to one of its
  subclasess
=> Therefore, we can replace the use of virtual methods with a function
pointer, which avoids the issue of duplicated vtables.

None of the objectDestroyed actually cares about the object we pass to
them, so we can leave it out in the function pointer. As everything is
private API, we could easily bring it back if the need arises in the
future.
By moving the function pointer into QQmlGuardImpl we also avoid UB in
qqmlengine.cpp, which cast any QQmlGuardImpl pointer to
QQmlGuard<QObject>. This, however, is wrong as QQmlGuard<T> does not
inherit from QQmlGuard<QObject>, even if T inherits from QObject. As we
now can sipmly access the pointer from QQmlGuardImpl, we can side-step
any casting woes alltogether.

Moreover, we use this opportunity to let QQmlStrongJSObjectReference
drectly inherit from QQmlGuardImpl. This requires duplicating some of
the QQmlGuard API, but on the other hand avoids busy-work to hide no
longer desired API.

Unfortunately, we can no longer inherit privately from QQmlGuardImpl, as
otherwise the various subclasses can no longer cast the QQmlGuardImpl we
pass to them to their own type in the objectDestroyed(Impl) methods.

QQmlGuard(Impl) still could benefit from a move ctor/assignment
operator; those will be added in a later commit.

Task-number: QTBUG-45582
Change-Id: I995148a428e541ced5c79b3a61d91c4bb7e03308
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

10 files changed, 82 insertions, 51 deletions

diff --git a/src/qml/qml/qqmlcontextdata_p.h b/src/qml/qml/qqmlcontextdata_p.h
index 7d759dfa82..932d917499 100644
--- a/src/qml/qml/qqmlcontextdata_p.h
+++ b/src/qml/qml/qqmlcontextdata_p.h
@@ -320,9 +320,8 @@ private:
             ObjectWasSet
         };
 
-        inline ContextGuard() : m_context(nullptr) {}
+        inline ContextGuard() : QQmlGuard<QObject>(&ContextGuard::objectDestroyedImpl, nullptr), m_context(nullptr) {}
         inline ContextGuard &operator=(QObject *obj);
-        inline void objectDestroyed(QObject *) override;
 
         inline bool wasSet() const;
 
@@ -333,6 +332,7 @@ private:
         }
 
     private:
+        inline static void objectDestroyedImpl(QQmlGuardImpl *);
         // Not refcounted, as it always belongs to the QQmlContextData.
         QTaggedPointer<QQmlContextData, Tag> m_context;
         QQmlNotifier m_bindings;
@@ -459,11 +459,12 @@ QQmlContextData::ContextGuard &QQmlContextData::ContextGuard::operator=(QObject
     return *this;
 }
 
-void QQmlContextData::ContextGuard::objectDestroyed(QObject *)
+ void QQmlContextData::ContextGuard::objectDestroyedImpl(QQmlGuardImpl *impl)
 {
-    if (QObject *contextObject = m_context->contextObject()) {
+    auto This = static_cast<QQmlContextData::ContextGuard *>(impl);
+    if (QObject *contextObject = This->m_context->contextObject()) {
         if (!QObjectPrivate::get(contextObject)->wasDeleted)
-            m_bindings.notify();
+            This->m_bindings.notify();
     }
 }
 
diff --git a/src/qml/qml/qqmlengine.cpp b/src/qml/qml/qqmlengine.cpp
index 26ebe0f88f..54f0698f79 100644
--- a/src/qml/qml/qqmlengine.cpp
+++ b/src/qml/qml/qqmlengine.cpp
@@ -1323,9 +1323,10 @@ void QQmlData::destroyed(QObject *object)
     ownContext.reset();
 
     while (guards) {
-        QQmlGuard<QObject> *guard = static_cast<QQmlGuard<QObject> *>(guards);
-        *guard = (QObject *)nullptr;
-        guard->objectDestroyed(object);
+        auto *guard = guards;
+        guard->setObject(nullptr);
+        if (guard->objectDestroyed)
+            guard->objectDestroyed(guard);
     }
 
     disconnectNotifiers();
diff --git a/src/qml/qml/qqmlguard_p.h b/src/qml/qml/qqmlguard_p.h
index 4abc9b632a..4286414bba 100644
--- a/src/qml/qml/qqmlguard_p.h
+++ b/src/qml/qml/qqmlguard_p.h
@@ -59,6 +59,8 @@ QT_BEGIN_NAMESPACE
 class QQmlGuardImpl
 {
 public:
+    using ObjectDestroyedFn = void(*)(QQmlGuardImpl *);
+
     inline QQmlGuardImpl();
     inline QQmlGuardImpl(QObject *);
     inline QQmlGuardImpl(const QQmlGuardImpl &);
@@ -67,6 +69,7 @@ public:
     QObject *o = nullptr;
     QQmlGuardImpl  *next = nullptr;
     QQmlGuardImpl **prev = nullptr;
+    ObjectDestroyedFn objectDestroyed = nullptr;
 
     inline void addGuard();
     inline void remGuard();
@@ -77,14 +80,14 @@ public:
 
 class QObject;
 template<class T>
-class QQmlGuard : private QQmlGuardImpl
+class QQmlGuard : protected QQmlGuardImpl
 {
     friend class QQmlData;
 public:
     inline QQmlGuard();
+    inline QQmlGuard(ObjectDestroyedFn objectDestroyed, T *);
     inline QQmlGuard(T *);
     inline QQmlGuard(const QQmlGuard<T> &);
-    inline virtual ~QQmlGuard();
 
     inline QQmlGuard<T> &operator=(const QQmlGuard<T> &o);
     inline QQmlGuard<T> &operator=(T *);
@@ -98,24 +101,29 @@ public:
     T &operator*() const { return *object(); }
     operator T *() const noexcept { return object(); }
     T *data() const noexcept { return object(); }
-
-protected:
-    virtual void objectDestroyed(T *) {}
 };
 
 template <typename T>
-class QQmlStrongJSQObjectReference : public QQmlGuard<T>
+class QQmlStrongJSQObjectReference : protected QQmlGuardImpl
 {
 public:
+    T *object() const noexcept { return static_cast<T *>(o); }
+
+    using QQmlGuardImpl::isNull;
+
+    T *operator->() const noexcept { return object(); }
+    T &operator*() const { return *object(); }
+    operator T *() const noexcept { return object(); }
+    T *data() const noexcept { return object(); }
     void setObject(T *obj, QObject *parent) {
-        T *old = this->object();
+        T *old = object();
         if (obj == old)
             return;
 
         if (m_jsOwnership && old && old->parent() == parent)
             QQml_setParent_noEvent(old, nullptr);
 
-        this->QQmlGuard<T>::operator=(obj);
+        QQmlGuardImpl::setObject(obj);
 
         if (obj && !obj->parent() && !QQmlData::keepAliveDuringGarbageCollection(obj)) {
             m_jsOwnership = true;
@@ -126,8 +134,6 @@ public:
     }
 
 private:
-    using QQmlGuard<T>::setObject;
-    using QQmlGuard<T>::operator=;
     bool m_jsOwnership = false;
 };
 
@@ -147,8 +153,15 @@ QQmlGuardImpl::QQmlGuardImpl(QObject *g)
     if (o) addGuard();
 }
 
+/*
+    \internal
+    Copying a QQmlGuardImpl leaves the old one in the intrinsic linked list of guards.
+    The fresh copy does not contain the list pointer of the existing guard; instead
+    only the object and objectDestroyed pointers are copied, and if there is an object
+    we add the new guard to the object's list of guards.
+ */
 QQmlGuardImpl::QQmlGuardImpl(const QQmlGuardImpl &g)
-: o(g.o)
+: o(g.o), objectDestroyed(g.objectDestroyed)
 {
     if (o) addGuard();
 }
@@ -189,25 +202,28 @@ QQmlGuard<T>::QQmlGuard()
 }
 
 template<class T>
-QQmlGuard<T>::QQmlGuard(T *g)
-: QQmlGuardImpl(g)
+QQmlGuard<T>::QQmlGuard(ObjectDestroyedFn objDestroyed, T *obj)
+    : QQmlGuardImpl(obj)
 {
+    objectDestroyed = objDestroyed;
 }
 
 template<class T>
-QQmlGuard<T>::QQmlGuard(const QQmlGuard<T> &g)
+QQmlGuard<T>::QQmlGuard(T *g)
 : QQmlGuardImpl(g)
 {
 }
 
 template<class T>
-QQmlGuard<T>::~QQmlGuard()
+QQmlGuard<T>::QQmlGuard(const QQmlGuard<T> &g)
+: QQmlGuardImpl(g)
 {
 }
 
 template<class T>
 QQmlGuard<T> &QQmlGuard<T>::operator=(const QQmlGuard<T> &g)
 {
+    objectDestroyed = g.objectDestroyed;
     setObject(g.object());
     return *this;
 }
@@ -215,6 +231,10 @@ QQmlGuard<T> &QQmlGuard<T>::operator=(const QQmlGuard<T> &g)
 template<class T>
 QQmlGuard<T> &QQmlGuard<T>::operator=(T *g)
 {
+    /* this does not touch objectDestroyed, as operator= is only a convenience
+     * for setObject. All logic involving objectDestroyed is (sub-)class specific
+     * and remains unaffected.
+     */
     setObject(g);
     return *this;
 }
diff --git a/src/qml/qml/qqmlvmemetaobject.cpp b/src/qml/qml/qqmlvmemetaobject.cpp
index 93abe3c83a..ef41d81868 100644
--- a/src/qml/qml/qqmlvmemetaobject.cpp
+++ b/src/qml/qml/qqmlvmemetaobject.cpp
@@ -165,31 +165,28 @@ static void list_removeLast(QQmlListProperty<QObject> *prop)
 }
 
 QQmlVMEVariantQObjectPtr::QQmlVMEVariantQObjectPtr()
-    : QQmlGuard<QObject>(nullptr), m_target(nullptr), m_index(-1)
+    : QQmlGuard<QObject>(QQmlVMEVariantQObjectPtr::objectDestroyedImpl, nullptr), m_target(nullptr), m_index(-1)
 {
 }
 
-QQmlVMEVariantQObjectPtr::~QQmlVMEVariantQObjectPtr()
+void QQmlVMEVariantQObjectPtr::objectDestroyedImpl(QQmlGuardImpl *guard)
 {
-}
-
-void QQmlVMEVariantQObjectPtr::objectDestroyed(QObject *)
-{
-    if (!m_target || QQmlData::wasDeleted(m_target->object))
+    auto This = static_cast<QQmlVMEVariantQObjectPtr *>(guard);
+    if (!This->m_target || QQmlData::wasDeleted(This->m_target->object))
         return;
 
-    if (m_index >= 0) {
-        QV4::ExecutionEngine *v4 = m_target->propertyAndMethodStorage.engine();
+    if (This->m_index >= 0) {
+        QV4::ExecutionEngine *v4 = This->m_target->propertyAndMethodStorage.engine();
         if (v4) {
             QV4::Scope scope(v4);
-            QV4::Scoped<QV4::MemberData> sp(scope, m_target->propertyAndMethodStorage.value());
+            QV4::Scoped<QV4::MemberData> sp(scope, This->m_target->propertyAndMethodStorage.value());
             if (sp) {
-                QV4::PropertyIndex index{ sp->d(), sp->d()->values.values + m_index };
+                QV4::PropertyIndex index{ sp->d(), sp->d()->values.values + This->m_index };
                 index.set(v4, QV4::Value::nullValue());
             }
         }
 
-        m_target->activate(m_target->object, m_target->methodOffset() + m_index, nullptr);
+        This->m_target->activate(This->m_target->object, This->m_target->methodOffset() + This->m_index, nullptr);
     }
 }
 
diff --git a/src/qml/qml/qqmlvmemetaobject_p.h b/src/qml/qml/qqmlvmemetaobject_p.h
index d8ae670597..e7ddf4d144 100644
--- a/src/qml/qml/qqmlvmemetaobject_p.h
+++ b/src/qml/qml/qqmlvmemetaobject_p.h
@@ -79,13 +79,14 @@ class QQmlVMEVariantQObjectPtr : public QQmlGuard<QObject>
 {
 public:
     inline QQmlVMEVariantQObjectPtr();
-    inline ~QQmlVMEVariantQObjectPtr() override;
 
-    inline void objectDestroyed(QObject *) override;
     inline void setGuardedValue(QObject *obj, QQmlVMEMetaObject *target, int index);
 
     QQmlVMEMetaObject *m_target;
     int m_index;
+
+private:
+    static void objectDestroyedImpl(QQmlGuardImpl *guard);
 };
 
 
diff --git a/src/qmlmodels/qqmladaptormodel.cpp b/src/qmlmodels/qqmladaptormodel.cpp
index 9ee9c91cd4..1463a84af9 100644
--- a/src/qmlmodels/qqmladaptormodel.cpp
+++ b/src/qmlmodels/qqmladaptormodel.cpp
@@ -953,7 +953,8 @@ QQmlAdaptorModel::Accessors::~Accessors()
 }
 
 QQmlAdaptorModel::QQmlAdaptorModel()
-    : accessors(&qt_vdm_null_accessors)
+    : QQmlGuard<QObject>(QQmlAdaptorModel::objectDestroyedImpl, nullptr)
+    , accessors(&qt_vdm_null_accessors)
 {
 }
 
@@ -1046,9 +1047,10 @@ void QQmlAdaptorModel::useImportVersion(QTypeRevision revision)
     modelItemRevision = revision;
 }
 
-void QQmlAdaptorModel::objectDestroyed(QObject *)
+void QQmlAdaptorModel::objectDestroyedImpl(QQmlGuardImpl *guard)
 {
-    setModel(QVariant());
+    auto This = static_cast<QQmlAdaptorModel *>(guard);
+    This->setModel(QVariant());
 }
 
 QQmlAdaptorModelEngineData::QQmlAdaptorModelEngineData(QV4::ExecutionEngine *v4)
diff --git a/src/qmlmodels/qqmladaptormodel_p.h b/src/qmlmodels/qqmladaptormodel_p.h
index 58991fbb15..06fe6068d7 100644
--- a/src/qmlmodels/qqmladaptormodel_p.h
+++ b/src/qmlmodels/qqmladaptormodel_p.h
@@ -170,8 +170,8 @@ public:
     inline bool canFetchMore() const { return accessors->canFetchMore(*this); }
     inline void fetchMore() { return accessors->fetchMore(*this); }
 
-protected:
-    void objectDestroyed(QObject *) override;
+private:
+    static void objectDestroyedImpl(QQmlGuardImpl *);
 };
 
 class QQmlAdaptorModelProxyInterface
diff --git a/src/qmlmodels/qquickpackage.cpp b/src/qmlmodels/qquickpackage.cpp
index bf83afb329..999919562b 100644
--- a/src/qmlmodels/qquickpackage.cpp
+++ b/src/qmlmodels/qquickpackage.cpp
@@ -90,11 +90,14 @@ public:
 
     struct DataGuard : public QQmlGuard<QObject>
     {
-        DataGuard(QObject *obj, QList<DataGuard> *l) : list(l) { (QQmlGuard<QObject>&)*this = obj; }
+        DataGuard(QObject *obj, QList<DataGuard> *l) : QQmlGuard<QObject>(DataGuard::objectDestroyedImpl, nullptr), list(l) { (QQmlGuard<QObject>&)*this = obj; }
         QList<DataGuard> *list;
-        void objectDestroyed(QObject *) override {
+
+    private:
+        static void objectDestroyedImpl(QQmlGuardImpl *guard) {
+            auto This = static_cast<DataGuard *>(guard);
             // we assume priv will always be destroyed after objectDestroyed calls
-            list->removeOne(*this);
+            This->list->removeOne(*This);
         }
     };
 
diff --git a/src/quick/items/qquickdrag_p.h b/src/quick/items/qquickdrag_p.h
index e17a28d07e..a84af80ee9 100644
--- a/src/quick/items/qquickdrag_p.h
+++ b/src/quick/items/qquickdrag_p.h
@@ -73,11 +73,11 @@ class QQuickDragGrabber
     class Item : public QQmlGuard<QQuickItem>
     {
     public:
-        Item(QQuickItem *item) : QQmlGuard<QQuickItem>(item) {}
+        Item(QQuickItem *item) : QQmlGuard<QQuickItem>(Item::objectDestroyedImpl, item) {}
 
         QIntrusiveListNode node;
-    protected:
-        void objectDestroyed(QQuickItem *) override { delete this; }
+    private:
+        static void objectDestroyedImpl(QQmlGuardImpl *guard) { delete static_cast<Item *>(guard); }
     };
 
     typedef QIntrusiveList<Item, &Item::node> ItemList;
diff --git a/src/quick/util/qquickstate_p_p.h b/src/quick/util/qquickstate_p_p.h
index d75cfc32ee..3f0545df91 100644
--- a/src/quick/util/qquickstate_p_p.h
+++ b/src/quick/util/qquickstate_p_p.h
@@ -212,13 +212,19 @@ public:
 
     struct OperationGuard : public QQmlGuard<QQuickStateOperation>
     {
-        OperationGuard(QObject *obj, QList<OperationGuard> *l) : list(l) {
+        OperationGuard(QObject *obj, QList<OperationGuard> *l) : QQmlGuard<QQuickStateOperation>(
+                                                                     OperationGuard::objectDestroyedImpl, nullptr)
+                                                               ,list(l)
+        {
             setObject(static_cast<QQuickStateOperation *>(obj));
         }
         QList<OperationGuard> *list;
-        void objectDestroyed(QQuickStateOperation *) override {
+
+    private:
+        static void objectDestroyedImpl(QQmlGuardImpl *guard) {
+            auto This = static_cast<OperationGuard *>(guard);
             // we assume priv will always be destroyed after objectDestroyed calls
-            list->removeOne(*this);
+            This->list->removeOne(*This);
         }
     };
     QList<OperationGuard> operations;