Fix a crash when deleting QObject instances with connections in threads

GlobalReceiverV2 connected to the destroyed() signal of the senders to keep track of its lifetime, which caused issues with delayed emission of signals when using threads. To fix this, change GlobalReceiverV2's sender list to use QPointer, which automatically tracks the lifetime of the pointees. Move the deletion of the GlobalReceiverV2 instances into SignalManager, completely, removing the "delete this" pattern used. This allows for removing some hacks for the QObject::receivers() function. Fixes: PYSIDE-2141 Change-Id: I361256f919dab13bfcf20800624b2454308bbc4b Reviewed-by: Cristian Maureira-Fredes <cristian.maureira-fredes@qt.io> Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
author: Friedemann Kleint <Friedemann.Kleint@qt.io> 2022-12-06 17:56:14 +0100
committer: Friedemann Kleint <Friedemann.Kleint@qt.io> 2022-12-08 10:30:35 +0100
commit: 1270a9e82e5bc3bd53a1131698ece60403da1192 (patch)
tree: 5015e154c123078bd5d4524b63f06db6f2f0cd69
parent: f7e2771a8f3a187407822df4564deeb7df1712dc (diff)
6 files changed, 74 insertions, 169 deletions
diff --git a/sources/pyside6/PySide6/QtCore/typesystem_core_common.xml b/sources/pyside6/PySide6/QtCore/typesystem_core_common.xml
index d9a6c8435..21be326df 100644
--- a/sources/pyside6/PySide6/QtCore/typesystem_core_common.xml
+++ b/sources/pyside6/PySide6/QtCore/typesystem_core_common.xml
@@ -1773,10 +1773,6 @@
         </modify-argument>
     </add-function>
 
-   <modify-function signature="receivers(const char*)const">
-     <inject-code class="target" position="beginning" file="../glue/qtcore.cpp" snippet="qobject-receivers"/>
-   </modify-function>
-
    <modify-function signature="destroyed(QObject*)" allow-thread="yes">
      <modify-argument index="1">
        <rename to="object"/>
diff --git a/sources/pyside6/PySide6/glue/qtcore.cpp b/sources/pyside6/PySide6/glue/qtcore.cpp
index 0f4816bbb..ba34a0259 100644
--- a/sources/pyside6/PySide6/glue/qtcore.cpp
+++ b/sources/pyside6/PySide6/glue/qtcore.cpp
@@ -536,15 +536,6 @@ const QString result = qObjectTr(reinterpret_cast<PyTypeObject *>(%PYSELF), %1,
 %PYARG_0 = %CONVERTTOPYTHON[QString](result);
 // @snippet qobject-tr
 
-// @snippet qobject-receivers
-// Avoid return +1 because SignalManager connect to "destroyed()" signal to control object timelife
-int ret = %CPPSELF.%FUNCTION_NAME(%1);
-if (ret > 0 && ((strcmp(%1, SIGNAL(destroyed())) == 0) || (strcmp(%1, SIGNAL(destroyed(QObject*))) == 0)))
-    ret -= PySide::SignalManager::instance().countConnectionsWith(%CPPSELF);
-
-%PYARG_0 = %CONVERTTOPYTHON[int](ret);
-// @snippet qobject-receivers
-
 // @snippet qbytearray-mgetitem
 if (PepIndex_Check(_key)) {
     const Py_ssize_t _i = PyNumber_AsSsize_t(_key, PyExc_IndexError);
diff --git a/sources/pyside6/libpyside/globalreceiverv2.cpp b/sources/pyside6/libpyside/globalreceiverv2.cpp
index 4f72d14ce..1342af24e 100644
--- a/sources/pyside6/libpyside/globalreceiverv2.cpp
+++ b/sources/pyside6/libpyside/globalreceiverv2.cpp
@@ -18,11 +18,6 @@
 
 #define RECEIVER_DESTROYED_SLOT_NAME "__receiverDestroyed__(QObject*)"
 
-namespace
-{
-    static int DESTROY_SIGNAL_ID = 0;
-    static int DESTROY_SLOT_ID = 0;
-}
 
 namespace PySide
 {
@@ -146,7 +141,7 @@ void DynamicSlotDataV2::onCallbackDestroyed(void *data)
     auto self = reinterpret_cast<DynamicSlotDataV2 *>(data);
     self->m_weakRef = nullptr;
     Py_BEGIN_ALLOW_THREADS
-    delete self->m_parent;
+    SignalManager::instance().deleteGobalReceiver(self->m_parent);
     Py_END_ALLOW_THREADS
 }
 
@@ -160,31 +155,17 @@ DynamicSlotDataV2::~DynamicSlotDataV2()
     Py_DECREF(m_callback);
 }
 
-GlobalReceiverV2::GlobalReceiverV2(PyObject *callback, GlobalReceiverV2MapPtr map) :
+GlobalReceiverV2::GlobalReceiverV2(PyObject *callback) :
     QObject(nullptr),
-    m_metaObject("__GlobalReceiver__", &QObject::staticMetaObject),
-    m_sharedMap(std::move(map))
+    m_metaObject("__GlobalReceiver__", &QObject::staticMetaObject)
 {
     m_data = new DynamicSlotDataV2(callback, this);
-    m_metaObject.addSlot(RECEIVER_DESTROYED_SLOT_NAME);
-    m_metaObject.update();
-    m_refs.append(nullptr);
-
-
-    if (DESTROY_SIGNAL_ID == 0)
-        DESTROY_SIGNAL_ID = QObject::staticMetaObject.indexOfSignal("destroyed(QObject*)");
-
-    if (DESTROY_SLOT_ID == 0)
-        DESTROY_SLOT_ID = m_metaObject.indexOfMethod(QMetaMethod::Slot, RECEIVER_DESTROYED_SLOT_NAME);
-
-
 }
 
 GlobalReceiverV2::~GlobalReceiverV2()
 {
     m_refs.clear();
     // Remove itself from map.
-    m_sharedMap->remove(m_data->key());
     // Suppress handling of destroyed() for objects whose last reference is contained inside
     // the callback object that will now be deleted. The reference could be a default argument,
     // a callback local variable, etc.
@@ -204,69 +185,34 @@ int GlobalReceiverV2::addSlot(const char *signature)
 
 void GlobalReceiverV2::incRef(const QObject *link)
 {
-    if (link) {
-        if (!m_refs.contains(link)) {
-            bool connected{};
-            Py_BEGIN_ALLOW_THREADS
-            connected = QMetaObject::connect(link, DESTROY_SIGNAL_ID, this, DESTROY_SLOT_ID);
-            Py_END_ALLOW_THREADS
-            if (connected)
-                m_refs.append(link);
-            else
-                Q_ASSERT(false);
-        } else {
-            m_refs.append(link);
-        }
-    } else {
-        m_refs.append(nullptr);
-    }
+    Q_ASSERT(link);
+    m_refs.append(link);
 }
 
 void GlobalReceiverV2::decRef(const QObject *link)
 {
-    if (m_refs.isEmpty())
-        return;
-
-
+    Q_ASSERT(link);
     m_refs.removeOne(link);
-    if (link) {
-        if (!m_refs.contains(link)) {
-            bool result{};
-            Py_BEGIN_ALLOW_THREADS
-            result = QMetaObject::disconnect(link, DESTROY_SIGNAL_ID, this, DESTROY_SLOT_ID);
-            Py_END_ALLOW_THREADS
-            Q_ASSERT(result);
-            if (!result)
-                return;
-        }
-    }
-
-    if (m_refs.isEmpty())
-        Py_BEGIN_ALLOW_THREADS
-        delete this;
-        Py_END_ALLOW_THREADS
+}
 
+void GlobalReceiverV2::notify()
+{
+    purgeDeletedSenders();
 }
 
-int GlobalReceiverV2::refCount(const QObject *link) const
+static bool isNull(const QPointer<const QObject> &p)
 {
-    if (link)
-        return m_refs.count(link);
+    return p.isNull();
+}
 
-    return m_refs.size();
+void GlobalReceiverV2::purgeDeletedSenders()
+{
+    m_refs.erase(std::remove_if(m_refs.begin(), m_refs.end(), isNull), m_refs.end());
 }
 
-void GlobalReceiverV2::notify()
+bool GlobalReceiverV2::isEmpty() const
 {
-    const QSet<const QObject *> objSet(m_refs.cbegin(), m_refs.cend());
-    Py_BEGIN_ALLOW_THREADS
-    for (const QObject *o : objSet) {
-        if (o) {
-            QMetaObject::disconnect(o, DESTROY_SIGNAL_ID, this, DESTROY_SLOT_ID);
-            QMetaObject::connect(o, DESTROY_SIGNAL_ID, this, DESTROY_SLOT_ID);
-        }
-    }
-    Py_END_ALLOW_THREADS
+    return std::all_of(m_refs.cbegin(), m_refs.cend(), isNull);
 }
 
 GlobalReceiverKey GlobalReceiverV2::key() const
@@ -294,26 +240,15 @@ int GlobalReceiverV2::qt_metacall(QMetaObject::Call call, int id, void **args)
     Q_ASSERT(slot.methodType() == QMetaMethod::Slot);
 
     if (!m_data) {
-        if (id != DESTROY_SLOT_ID) {
-            const QByteArray message = "PySide6 Warning: Skipping callback call "
-                + slot.methodSignature() + " because the callback object is being destructed.";
-            PyErr_WarnEx(PyExc_RuntimeWarning, message.constData(), 0);
-        }
+        const QByteArray message = "PySide6 Warning: Skipping callback call "
+            + slot.methodSignature() + " because the callback object is being destructed.";
+        PyErr_WarnEx(PyExc_RuntimeWarning, message.constData(), 0);
         return -1;
     }
 
-    if (id == DESTROY_SLOT_ID) {
-        if (m_refs.isEmpty())
-            return -1;
-        auto obj = *reinterpret_cast<QObject **>(args[1]);
-        incRef(); //keep the object live (safe ref)
-        m_refs.removeAll(obj); // remove all refs to this object
-        decRef(); //remove the safe ref
-    } else {
-        const bool isShortCuit = std::strchr(slot.methodSignature(), '(') == nullptr;
-        Shiboken::AutoDecRef callback(m_data->callback());
-        SignalManager::callPythonMetaMethod(slot, args, callback, isShortCuit);
-    }
+    const bool isShortCuit = std::strchr(slot.methodSignature(), '(') == nullptr;
+    Shiboken::AutoDecRef callback(m_data->callback());
+    SignalManager::callPythonMetaMethod(slot, args, callback, isShortCuit);
 
     // SignalManager::callPythonMetaMethod might have failed, in that case we have to print the
     // error so it considered "handled".
diff --git a/sources/pyside6/libpyside/globalreceiverv2.h b/sources/pyside6/libpyside/globalreceiverv2.h
index 472c3e94a..7ac966eb5 100644
--- a/sources/pyside6/libpyside/globalreceiverv2.h
+++ b/sources/pyside6/libpyside/globalreceiverv2.h
@@ -10,6 +10,7 @@
 
 #include <QtCore/QByteArray>
 #include <QtCore/QObject>
+#include <QtCore/QPointer>
 #include <QtCore/QMap>
 #include <QtCore/QSharedPointer>
 
@@ -37,9 +38,6 @@ inline bool operator!=(const GlobalReceiverKey &k1, const GlobalReceiverKey &k2)
 
 size_t qHash(const GlobalReceiverKey &k, size_t seed = 0);
 
-using GlobalReceiverV2Map = QHash<GlobalReceiverKey, GlobalReceiverV2 *>;
-using GlobalReceiverV2MapPtr = QSharedPointer<GlobalReceiverV2Map>;
-
 /// A class used to link C++ Signals to non C++ slots (Python callbacks) by
 /// providing fake slots for QObject::connect().
 /// It keeps a Python callback and the list of QObject senders. It is stored
@@ -49,9 +47,7 @@ class GlobalReceiverV2 : public QObject
 public:
     /// Create a GlobalReceiver object that will call 'callback'
     /// @param callback A Python callable object (can be a method or not)
-    /// @param map      A SharedPointer used on Signal manager that contains
-    ///                 all instaces of GlobalReceiver
-    GlobalReceiverV2(PyObject *callback, GlobalReceiverV2MapPtr map);
+    explicit GlobalReceiverV2(PyObject *callback);
 
     ~GlobalReceiverV2() override;
 
@@ -68,20 +64,17 @@ public:
     void notify();
 
     /// Used to increment the reference of the GlobalReceiver object
-    /// @param link This is a optional parameter used to link the ref to
+    /// @param link This is a parameter used to link the ref to
     ///             some QObject life.
-    void incRef(const QObject *link = nullptr);
+    void incRef(const QObject *link);
 
     /// Used to decrement the reference of the GlobalReceiver object.
-    /// @param link This is a optional parameter used to dismiss the link
+    /// @param link This is a parameter used to dismiss the link
     ///             ref to some QObject.
-    void decRef(const QObject *link = nullptr);
+    void decRef(const QObject *link);
 
-    /// Return the count of refs which the GlobalReceiver has
-    /// @param link If any QObject was passed, the function returns the
-    ///             number of references relative to this 'link' object.
-    /// @return The number of references
-    int refCount(const QObject *link) const;
+    /// Returns whether any senders are registered.
+    bool isEmpty() const;
 
     /// Use to retrieve the unique hash of this GlobalReceiver object
     /// @return hash key
@@ -96,10 +89,12 @@ public:
     MetaObjectBuilder &metaObjectBuilder() { return m_metaObject; }
 
 private:
+    void purgeDeletedSenders();
+
     MetaObjectBuilder m_metaObject;
     DynamicSlotDataV2 *m_data;
-    QList<const QObject *> m_refs;
-    GlobalReceiverV2MapPtr m_sharedMap;
+    using QObjectPointer = QPointer<const QObject>;
+    QList<QObjectPointer> m_refs;
 };
 
 }
diff --git a/sources/pyside6/libpyside/signalmanager.cpp b/sources/pyside6/libpyside/signalmanager.cpp
index 9a9d1c19b..1f2fcac56 100644
--- a/sources/pyside6/libpyside/signalmanager.cpp
+++ b/sources/pyside6/libpyside/signalmanager.cpp
@@ -187,28 +187,19 @@ QDataStream &operator>>(QDataStream &in, PyObjectWrapper &myObj)
 
 };
 
+namespace PySide {
+using GlobalReceiverV2Ptr = QSharedPointer<GlobalReceiverV2>;
+using GlobalReceiverV2Map = QHash<PySide::GlobalReceiverKey, GlobalReceiverV2Ptr>;
+}
+
 using namespace PySide;
 
 struct SignalManager::SignalManagerPrivate
 {
-    GlobalReceiverV2MapPtr m_globalReceivers;
-    static SignalManager::QmlMetaCallErrorHandler m_qmlMetaCallErrorHandler;
-
-    SignalManagerPrivate() : m_globalReceivers(new GlobalReceiverV2Map{})
-    {
-    }
+    void deleteGobalReceiver(const QObject *gr);
 
-    ~SignalManagerPrivate()
-    {
-        if (!m_globalReceivers.isNull()) {
-            // Delete receivers by always retrieving the current first element, because deleting a
-            // receiver can indirectly delete another one, and if we use qDeleteAll, that could
-            // cause either a double delete, or iterator invalidation, and thus undefined behavior.
-            while (!m_globalReceivers->isEmpty())
-                delete *m_globalReceivers->cbegin();
-            Q_ASSERT(m_globalReceivers->isEmpty());
-        }
-    }
+    GlobalReceiverV2Map m_globalReceivers;
+    static SignalManager::QmlMetaCallErrorHandler m_qmlMetaCallErrorHandler;
 
     static void handleMetaCallError(QObject *object, int *result);
     static int qtPropertyMetacall(QObject *object, QMetaObject::Call call,
@@ -264,8 +255,7 @@ SignalManager::SignalManager() : m_d(new SignalManagerPrivate)
 
 void SignalManager::clear()
 {
-    delete m_d;
-    m_d = new SignalManagerPrivate();
+    m_d->m_globalReceivers.clear();
 }
 
 SignalManager::~SignalManager()
@@ -286,34 +276,16 @@ void SignalManager::setQmlMetaCallErrorHandler(QmlMetaCallErrorHandler handler)
 
 QObject *SignalManager::globalReceiver(QObject *sender, PyObject *callback)
 {
-    GlobalReceiverV2MapPtr globalReceivers = m_d->m_globalReceivers;
-    GlobalReceiverKey key = GlobalReceiverV2::key(callback);
-    GlobalReceiverV2 *gr = nullptr;
-    auto it = globalReceivers->find(key);
-    if (it == globalReceivers->end()) {
-        gr = new GlobalReceiverV2(callback, globalReceivers);
-        globalReceivers->insert(key, gr);
-        if (sender) {
-            gr->incRef(sender); // create a link reference
-            gr->decRef(); // remove extra reference
-        }
-    } else {
-        gr = it.value();
-        if (sender)
-            gr->incRef(sender);
+    auto &globalReceivers = m_d->m_globalReceivers;
+    const GlobalReceiverKey key = GlobalReceiverV2::key(callback);
+    auto it = globalReceivers.find(key);
+    if (it == globalReceivers.end()) {
+        it = globalReceivers.insert(key, GlobalReceiverV2Ptr(new GlobalReceiverV2(callback)));
     }
+    if (sender)
+        it.value()->incRef(sender); // create a link reference
 
-    return reinterpret_cast<QObject *>(gr);
-}
-
-int SignalManager::countConnectionsWith(const QObject *object)
-{
-    int count = 0;
-    for (GlobalReceiverV2Map::const_iterator it = m_d->m_globalReceivers->cbegin(), end = m_d->m_globalReceivers->cend(); it != end; ++it) {
-        if (it.value()->refCount(object))
-            count++;
-    }
-    return count;
+    return it.value().data();
 }
 
 void SignalManager::notifyGlobalReceiver(QObject *receiver)
@@ -323,13 +295,30 @@ void SignalManager::notifyGlobalReceiver(QObject *receiver)
 
 void SignalManager::releaseGlobalReceiver(const QObject *source, QObject *receiver)
 {
-    auto gr = reinterpret_cast<GlobalReceiverV2 *>(receiver);
+    auto gr = static_cast<GlobalReceiverV2 *>(receiver);
     gr->decRef(source);
+    if (gr->isEmpty())
+        m_d->deleteGobalReceiver(gr);
+}
+
+void SignalManager::deleteGobalReceiver(const QObject *gr)
+{
+    SignalManager::instance().m_d->deleteGobalReceiver(gr);
+}
+
+void SignalManager::SignalManagerPrivate::deleteGobalReceiver(const QObject *gr)
+{
+    for (auto it = m_globalReceivers.begin(), end = m_globalReceivers.end(); it != end; ++it) {
+        if (it.value().data() == gr) {
+            m_globalReceivers.erase(it);
+            break;
+        }
+    }
 }
 
 int SignalManager::globalReceiverSlotIndex(QObject *receiver, const char *signature) const
 {
-    return reinterpret_cast<GlobalReceiverV2 *>(receiver)->addSlot(signature);
+    return static_cast<GlobalReceiverV2 *>(receiver)->addSlot(signature);
 }
 
 bool SignalManager::emitSignal(QObject *source, const char *signal, PyObject *args)
diff --git a/sources/pyside6/libpyside/signalmanager.h b/sources/pyside6/libpyside/signalmanager.h
index 60cf4e75e..abcff24d3 100644
--- a/sources/pyside6/libpyside/signalmanager.h
+++ b/sources/pyside6/libpyside/signalmanager.h
@@ -67,15 +67,14 @@ public:
     // used to discovery metaobject
     static const QMetaObject* retrieveMetaObject(PyObject* self);
 
-    // Used to discovery if SignalManager was connected with object "destroyed()" signal.
-    int countConnectionsWith(const QObject *object);
-
     // Disconnect all signals managed by Globalreceiver
     void clear();
 
     // Utility function to call a python method usign args received in qt_metacall
     static int callPythonMetaMethod(const QMetaMethod& method, void** args, PyObject* obj, bool isShortCuit);
 
+    static void deleteGobalReceiver(const QObject *globalReceiver);
+
 private:
     struct SignalManagerPrivate;
     SignalManagerPrivate* m_d;
author	Friedemann Kleint <Friedemann.Kleint@qt.io>	2022-12-06 17:56:14 +0100
committer	Friedemann Kleint <Friedemann.Kleint@qt.io>	2022-12-08 10:30:35 +0100
commit	1270a9e82e5bc3bd53a1131698ece60403da1192 (patch)
tree	5015e154c123078bd5d4524b63f06db6f2f0cd69
parent	f7e2771a8f3a187407822df4564deeb7df1712dc (diff)