From b13c2e69c30f6eeaee12d7e0d6da206cd98d60aa Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Wed, 26 Jul 2017 23:27:24 +0200 Subject: [PATCH] Fixed a potential infinite recursion for Qt bindings The problem is caused by an implementation detail: To monitor the lifetime of Qt objects, a monitoring object is attached to them. This used to happen through child objects but attaching such issues QChildEvents which - if routed over script objects - will itself required objects to be attached a monitoring object. The solution is to attach monitoring objects through QObject dynamic properties. --- src/gsi/gsiObject.cc | 3 +- src/gsiqt/gsiQtHelper.cc | 88 ++++++++++++++++++++++++++++++++++++++-- src/gsiqt/gsiQtHelper.h | 17 -------- src/pya/pyaObject.cc | 6 ++- src/rba/rbaInternal.cc | 3 +- 5 files changed, 92 insertions(+), 25 deletions(-) diff --git a/src/gsi/gsiObject.cc b/src/gsi/gsiObject.cc index 78067faa6..a6c6390da 100644 --- a/src/gsi/gsiObject.cc +++ b/src/gsi/gsiObject.cc @@ -91,7 +91,7 @@ Proxy::destroy () void Proxy::detach () { - if (m_cls_decl && m_cls_decl->is_managed ()) { + if (! m_destroyed && m_cls_decl && m_cls_decl->is_managed ()) { gsi::ObjectBase *gsi_object = m_cls_decl->gsi_object (m_obj, false); if (gsi_object) { gsi_object->status_changed_event ().remove (this, &Proxy::object_status_changed); @@ -217,6 +217,7 @@ void Proxy::object_status_changed (gsi::ObjectBase::StatusEventType type) { if (type == gsi::ObjectBase::ObjectDestroyed) { + m_destroyed = true; // NOTE: must be set before detach and indicates that the object was destroyed externally. detach (); } else if (type == gsi::ObjectBase::ObjectKeep) { m_owned = false; diff --git a/src/gsiqt/gsiQtHelper.cc b/src/gsiqt/gsiQtHelper.cc index 24fa8977f..1915da4fb 100644 --- a/src/gsiqt/gsiQtHelper.cc +++ b/src/gsiqt/gsiQtHelper.cc @@ -21,6 +21,73 @@ */ #include "gsiQtHelper.h" +#include "tlObject.h" + +#include + +namespace qt_gsi +{ + +/** + * @brief A tiny object being both a gsi::ObjectBase for lifetime monitoring and tl::Object for shared pointer management + * + * See QtWatcher for details. + */ +class QtLifetimeMonitor + : public tl::Object, public gsi::ObjectBase +{ + // .. nothing yet .. +}; + +/** + * @brief A helper object that is attached to a QObject to monitor it's lifetime + * + * This object will be put into the properties table of the QObject (more precisely: a copy). + * When the QObject is destroyed, it's properties are destroyed too and through reference + * counting the destruction of the QObject is detected. The monitoring itself is implemented + * through a gsi::ObjectBase object which plugs seamlessly into the gsi type system: + * + * QObject -> QtWatcher (+ temp copies) -> gsi::ObjectBase -> plugs into script objects + * + * NOTE: the initial idea was to use child objects attached to QObject. However, these + * interfere too much with Qt's internals - for example, attaching a child object emits + * a QChildEvent. That gets filtered in some eventFilter function which itself may cause + * QObjects to be decorated with helper objects ... resulting in a infinte recursion. + * + * Decoration by properties is a leaner solution. + */ +class QtWatcher +{ +public: + QtWatcher () + { + // .. nothing yet .. + } + + QtWatcher (QtLifetimeMonitor *monitor) + : mp_monitor (monitor) + { + // .. nothing yet .. + } + + gsi::ObjectBase *gsi_object () + { + if (mp_monitor) { + return static_cast (mp_monitor.get ()); + } else { + return 0; + } + } + +private: + QtWatcher (size_t id, gsi::ObjectBase *obj); + + tl::shared_ptr mp_monitor; +}; + +} + +Q_DECLARE_METATYPE (qt_gsi::QtWatcher) namespace qt_gsi { @@ -30,11 +97,24 @@ namespace qt_gsi */ gsi::ObjectBase *get_watcher_object (QObject *qobject, bool required) { - QtWatcherObject *watcher = qobject->findChild (); - if (! watcher && required) { - watcher = new QtWatcherObject (qobject); + const char *watcher_prop_name = "_gsi_qt::watcher"; + + QVariant prop = qobject->property (watcher_prop_name); + if (prop.isValid ()) { + + return prop.value ().gsi_object (); + + } else if (required) { + + QtWatcher w (new QtLifetimeMonitor ()); + qobject->setProperty (watcher_prop_name, QVariant::fromValue (w)); + return w.gsi_object (); + + } else { + + return 0; + } - return watcher; } } diff --git a/src/gsiqt/gsiQtHelper.h b/src/gsiqt/gsiQtHelper.h index 9dab7ed55..91f4d7a2a 100644 --- a/src/gsiqt/gsiQtHelper.h +++ b/src/gsiqt/gsiQtHelper.h @@ -32,23 +32,6 @@ namespace qt_gsi gsi::ObjectBase *get_watcher_object (QObject *qobject, bool required); -/** - * @brief A tiny watcher object linking QObject and gsi::ObjectBase - * This object is hooked into the watched object's child list to watch it's lifetime. - */ -class QtWatcherObject - : public QObject, public gsi::ObjectBase -{ -Q_OBJECT - -public: - QtWatcherObject (QObject *parent) - : QObject (parent), gsi::ObjectBase () - { - // .. nothing yet .. - } -}; - } #endif diff --git a/src/pya/pyaObject.cc b/src/pya/pyaObject.cc index 7a8168350..7a1d95fdb 100644 --- a/src/pya/pyaObject.cc +++ b/src/pya/pyaObject.cc @@ -347,6 +347,7 @@ PYAObjectBase::~PYAObjectBase () } catch (...) { tl::warn << "Caught unspecified exception in object destructor"; } + m_destroyed = true; } @@ -362,8 +363,9 @@ PYAObjectBase::object_status_changed (gsi::ObjectBase::StatusEventType type) bool prev_owner = m_owned; + m_destroyed = true; // NOTE: must be set before detach! + detach (); - m_destroyed = true; // NOTE: this may delete "this"! if (!prev_owner) { @@ -432,7 +434,7 @@ PYAObjectBase::detach () const gsi::ClassBase *cls = cls_decl (); - if (cls && cls->is_managed ()) { + if (! m_destroyed && cls && cls->is_managed ()) { gsi::ObjectBase *gsi_object = cls->gsi_object (m_obj, false); if (gsi_object) { gsi_object->status_changed_event ().remove (&m_listener, &StatusChangedListener::object_status_changed); diff --git a/src/rba/rbaInternal.cc b/src/rba/rbaInternal.cc index 7e0ee648f..e239f8404 100644 --- a/src/rba/rbaInternal.cc +++ b/src/rba/rbaInternal.cc @@ -379,7 +379,7 @@ Proxy::destroy () void Proxy::detach () { - if (m_cls_decl && m_cls_decl->is_managed ()) { + if (! m_destroyed && m_cls_decl && m_cls_decl->is_managed ()) { gsi::ObjectBase *gsi_object = m_cls_decl->gsi_object (m_obj, false); if (gsi_object) { gsi_object->status_changed_event ().remove (this, &Proxy::object_status_changed); @@ -684,6 +684,7 @@ void Proxy::object_status_changed (gsi::ObjectBase::StatusEventType type) { if (type == gsi::ObjectBase::ObjectDestroyed) { + m_destroyed = true; // NOTE: must be set before detach and indicates that the object was destroyed externally. detach (); } else if (type == gsi::ObjectBase::ObjectKeep) { keep_internal ();