[Development] Not delivering signals to slots in class sub-objects already destroyed

[Giuseppe D'Angelo]

Hi,

Il 03/09/25 07:18, Thiago Macieira ha scritto:
> Re: https://codereview.qt-project.org/c/qt/qtbase/+/672087
> 
> TL;DR: I've got a solution to make the new-style QObject::connect() match the
> behaviour of the old-style string-based one, which due to the nature of the
> virtual table dispatching, would not deliver signals to slots in class sub-
> objects already destroyed. But it isn't perfect. So I'd like a discussion:

My take:

> a) should this functionality be provided at all?

Yes, but AFAICT it's impossible to provide it in a way which is correct 
and complete (like the string-based used to be), unless we force our 
hand in some place.


> b) should it be provided by default for PMFs? [*]

Yes, but I think we should clarify what PMF actually means here...


> c) if it's provided for PMFs, should they be allowed to opt out?
> d) should it be provided by default for callbacks other than PMFs?

... because if affects the answer here:

A) if you're connecting to a non-static member function of a QObject 
subclass, then the correct thing to do is to disconnect/not to activate 
the slot if receiver's type is no longer the one declaring that 
non-static member function.

I have no idea how to achieve this reliably without RTTI: you can 
connect to an arbitrary PMF of a QObject subclass that doesn't use 
Q_OBJECT and therefore whose "distance" from QObject is a wrong metric:

struct A : public QObject
{
     Q_OBJECT
     ~A() { emit aSignal(); }
signals:
     void aSignal();
};

struct B : public A
{
     B() { connect(this, &B::aSignal, this, &B::aSlot); }
     void aSlot();
};

~A() will still call the slot, won't it? But it mustn't.



To complicate things further, the type of the receiver isn't the one 
that actually matters:


struct A : public QObject
{
     Q_OBJECT
     ~A() { emit aSignal(); }
     void aSlot();
signals:
     void aSignal();
};

struct B : public A
{
     B() { connect(this, &B::aSignal, this, &B::aSlot); }
};

Now A::aSlot() must be called instead.


We could bring back the "rule" that you should only connect to PMFs of 
classes that do have Q_OBJECT, or at least enforce it as best practice, 
say with some Clazy warnings (which already warns for QObject subclasses 
without Q_OBJECT in general). With this rule in place, would we be able 
to go back to the behavior of string-based connections?



B) if you're connecting to something else than a PMF, and you're passing 
a context object, then the check applies to the context. Not passing a 
context object is _definitely_ bad practice because of lifetime and 
threading issues; right now there's an opt-in 
(QT_NO_CONTEXTLESS_CONNECT, also set by QT_ENABLE_STRICT_MODE_UP_TO) but 
we should consider raising this to a deprecation level.

(More on this below)



> e) if there's a flag to opt in or out, what do we call the Qt::XxxConnection?

Is there a valid use-case for even wanting an opt-out mechanism? 
String-based connects don't have an opt-out one.


> 
> Longer version:
> I found a neat solution to implement a check for the dynamic class type that
> does not involve RTTI, and will restore the behaviour that we used to have
> with the string-based connections. The solution is to count how many meta
> object superClass() we have until we reach the QObject's, and store this count
> in the QObjectPrivate::Connection object. I've also done that without
> increasing that object's size, because there are members unused when
> connecting to a QSlotObjectBase.


> 
> There are two drawbacks:
> 1) it adds overhead to the activation of the slots, because it must count meta
> objects for every Connection prior to activation. As a singly-linked list,
> this is O(n), and though N is usually very small (99-percentile less than 10),
> it may cross library boundaries and may thus run into cache misses. The same
> operation is required on connect() itself, but the overhead there is
> relatively smaller compared to the rest of the work. I have not measured it
> and quite frankly I don't know what a good benchmark would be, if the issue is
> going to be cache misses. But I am wary of just adding content to
> QObject::doActivate(), which should be FAST.

Yeah, this implementation doesn't sound optimal.

If we force our hand elsewhere and require RTTI instead, would we able 
to provide a faster one, by checking the type_infos of the associated 
objects? (This would require using non-portable code, surely, but it's 
not that such a thing should scare us away.)


> 
> 2) I think it's safe to apply this by default when the receiver is a PMF and
> the class of the PMF has destroyed (implemented on the commit after the above,
> 672088), but what about when the receiver is a static or non-member function,
> or lambda or functor? The lifetime of the receiver's current dynamic class is
> not tightly bound with the callback. An example of this is
> Q_APPLICATION_STATIC:
> 
>   QObject::connect(app, &QObject::destroyed, app, reset, Qt::DirectConnection);

Here I think we may need a separate connect() overload, because the 3rd 
argument is pointless since one is calling a static function.

Why a different overload rather than just using the existing overload of 
connect()? Because the existing overload has shown that it's terribly 
error-prone. The hope would be that a connectToStatic() would help avoid 
such issues.

(Alternatively, the 3-arg connect should distinguish between connecting 
to function objects in general, and connecting to pointers to (static) 
functions; disallow the former, and allow the latter.)



> The dynamic class of the receiver stopped being QCoreApplication by the time
> ~QObject emitted destroyed(). Should the signal be delivered to reset()?
> Obviously the code above expects it to do so. In fact, that's the expectation
> for every use of &QObject::destroyed, because otherwise nothing would ever
> run.
> 
> But how about
> 
>      QObject::connect(notifier, &QWinEventNotifier::activated, this,
>          [this, registerForNotification] {
>              emit valueChanged();
>              registerForNotification();
>          });
>       QObject::connect(menuAction, &QAction::changed, q, [this] {
>          if (!tornPopup.isNull())
>              tornPopup->updateWindowTitle();
>      });
> QObject::connect(reply, &QNetworkReply::errorOccurred, query, [this, reply] {
>       m_networkErrors << reply->errorString();
>   });
> 
> Those are lambdas that, by capturing [this], are effectively member functions
> in disguise, but there's no way we can tell that. Invoking those callbacks
> after the members used in bodies of the lambdas have been destroyed is UB.

But that's the reason for passing the 3rd argument, isn't it? To make 
sure that deleting `this` will disconnect. (At least, that's the idea; 
at the moment it's not foolproof, thus this thread...)


> Even if the callback is a static or non-member function, it could still find
> the receiver via some global variable. And I don't know if we'd want a
> different behaviour depending on whether the callback is a lambda or a function
> pointer.
> 

I'd say that this is _way_ less likely to happen than 99.99% of the 
trouble we've been having with connects to PMFs(). What people do _in_ 
the slot is a separate issue than the fact that we should or not should 
activate them.



> Even the distinction between PMFs and others could be a pitfall. Take the first
> example from QWinEventNotifier and let's say the original code was
> 
>      QObject::connect(notifier, &QWinEventNotifier::activated,
>                       this, &QWinRegistryKey::valueChanged);
> 
> That code would have had the benefit of the protection. But then after some
> bug-fixing or additional functionality, it became the lambda above, which does
> not have the protection. That could lead to a subtle new bug[*] that may
> escape detection during unit-testing and get found only by users.
> 
> [*] Right now, we don't have the protection against calling a PMF in a
> destroyed sub-object, but we have an *enforcement* in debug mode, so if the
> delivery could have hit the PMF before the change, it should have been seen
> and fixed. Since we have the enforcement, I think that if we provide the
> functionality, it should be active for PMFs by default.

That's a separate can of worms, and also, once more, why people should 
always use the 4-arg connect (and avoid connecting to complicated lambdas).


My 2 cents,
-- 
Giuseppe D'Angelo | giuseppe.dangelo at kdab.com | Senior Software Engineer
KDAB (France) S.A.S., a KDAB Group company
Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com
KDAB - Trusted Software Excellence
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4850 bytes
Desc: Firma crittografica S/MIME
URL: <http://lists.qt-project.org/pipermail/development/attachments/20250917/8b958a83/attachment.bin>