QObject: Fix memory leak in queued_activate

queued_activate adds a reference to the slot object. It also attempts to
deref it again, but that did not work correctly so far. We could end up
with
T1                              |             T2
queued_activate                 |
  checks isSlotObject           |
    adds ref                    |
  locker.unlock()               |
                                | QObject::~QObject
                                |  //In disconnect all senders loop
                                |  sets isSlotObject to false
                                |  derefs slotObj, but not deleted
  checks isSlotObject           |
  (no deref because it's null)  |

To solve this issue and others caused by early returns, we now use a
RAII helper, which always takes care of calling destroyIfLastRef if the
ref count has been incremented.

Change-Id: I9c011cdb8faa5f344d7e70f024fc13f407e39ccf
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

1 files changed, 34 insertions, 12 deletions

diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index fafcd38417..1659573009 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -3648,6 +3648,37 @@ void QMetaObject::connectSlotsByName(QObject *o)
 }
 
 /*!
+     \internal
+     A small RAII helper for QSlotObjectBase.
+     Calls ref on construction and destroyLastRef in its dtor.
+     Allows construction from a nullptr in which case it does nothing.
+ */
+struct SlotObjectGuard {
+    SlotObjectGuard() = default;
+    // move would be fine, but we do not need it currently
+    Q_DISABLE_COPY_MOVE(SlotObjectGuard)
+    explicit SlotObjectGuard(QtPrivate::QSlotObjectBase *slotObject)
+        : m_slotObject(slotObject)
+    {
+        if (m_slotObject)
+            m_slotObject->ref();
+    }
+
+    QtPrivate::QSlotObjectBase const *operator->() const
+    { return m_slotObject; }
+
+    QtPrivate::QSlotObjectBase *operator->()
+    { return m_slotObject; }
+
+    ~SlotObjectGuard() {
+        if (m_slotObject)
+            m_slotObject->destroyIfLastRef();
+    }
+private:
+    QtPrivate::QSlotObjectBase *m_slotObject = nullptr;
+};
+
+/*!
     \internal
 
     \a signal must be in the signal index range (see QObjectPrivate::signalIndex()).
@@ -3678,8 +3709,8 @@ static void queued_activate(QObject *sender, int signal, QObjectPrivate::Connect
         // the connection has been disconnected before we got the lock
         return;
     }
-    if (c->isSlotObject)
-        c->slotObj->ref();
+
+    SlotObjectGuard slotObjectGuard { c->isSlotObject ? c->slotObj : nullptr };
     locker.unlock();
 
     QMetaCallEvent *ev = c->isSlotObject ?
@@ -3706,8 +3737,6 @@ static void queued_activate(QObject *sender, int signal, QObjectPrivate::Connect
     }
 
     locker.relock();
-    if (c->isSlotObject)
-        c->slotObj->destroyIfLastRef();
     if (!c->isSingleShot && !c->receiver.loadRelaxed()) {
         // the connection has been disconnected while we were unlocked
         locker.unlock();
@@ -3835,14 +3864,7 @@ void doActivate(QObject *sender, int signal_index, void **argv)
             QObjectPrivate::Sender senderData(receiverInSameThread ? receiver : nullptr, sender, signal_index);
 
             if (c->isSlotObject) {
-                c->slotObj->ref();
-
-                struct Deleter {
-                    void operator()(QtPrivate::QSlotObjectBase *slot) const {
-                        if (slot) slot->destroyIfLastRef();
-                    }
-                };
-                const std::unique_ptr<QtPrivate::QSlotObjectBase, Deleter> obj{c->slotObj};
+                SlotObjectGuard obj{c->slotObj};
 
                 {
                     Q_TRACE_SCOPE(QMetaObject_activate_slot_functor, obj.get());