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

[Lars Knoll]

Hi Thiago and Fabian,

> On 3 Sep 2025, at 09:24, Fabian Kosmale via Development <development at qt-project.org> wrote:
> 
> Hey Thiago,
> 
> first of all, thanks for looking into this. Give that this is a pitfall when porting to new style connection,
> and we had issues with this in Qt (and IIRC Creator) itself, I definitely think that we should investigate it.
> I had discussed a different, less generic idea (introduce indexOfSlot and use the same connect code as
> with string based connect) in private with Mårten and Marc, and the difference between lambdas and
> PMFs was also a point of contention. One possible way out there might be to support passing an
> explicit target meta-object + a clazy check which warns about capturing lambdas without that explicit
> meta-object.
> 
> Regarding performance: I'm wary about the overhead, but we can reduce it. Lars proposed back in the
> day to track all ancestor meta-objects, but that ran into opposition because he required that every
> QObject had a Q_OBJECT macro. However, that's not actually necessary; after all, a QObject without
> the macro doesn't have it's own meta-object. We also know that moc must be able to see all parent classes,
> as you can't have an incomplete base class. So I would expect that moc can track the depth[1], and we can store
> it directly inside the meta-object, making the check O(1) for code compiled against a new enough code.

I was thinking about a slightly different approach, namely storing store the meta object pointer that the object has when creating the connection in the connection object, you can with one pointer comparison determine whether the object is still fully constructed. That should cover most of the signal emissions with one pointer comparison. If object->metaObject != connection->metaObject, you can take the slower path that Thiago proposes and iterate over the meta object chain.

The problem with that approach is that it wouldn't work well for connections created in the constructor of a parent class.

So I think your approach is better, one complication could be classes in the hierarchy without a Q_OBJECT macro (that are later gaining one), but moc should see those as well, and can count them into the depth.

Cheers,
Lars

> 
> Regards,
> Fabian
> 
> [1] There's some complication with deriving from (potentially specialized) template instantiations, which is hard
> to properly support in moc. C++26 reflection solves this, and as we need to support meta-objects coming from
> older Qt versions, we'd have the loop as a fallback available in any case.
> 
> ________________________________________
> Von: Development <development-bounces at qt-project.org> im Auftrag von Thiago Macieira <thiago.macieira at intel.com>
> Gesendet: Mittwoch, 3. September 2025 07:18
> An: development at qt-project.org
> Betreff: [Development] Not delivering signals to slots in class sub-objects already destroyed
> 
> 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:
> 
> a) should this functionality be provided at all?
> b) should it be provided by default for PMFs? [*]
> 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?
> e) if there's a flag to opt in or out, what do we call the Qt::XxxConnection?
> 
> 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.
> 
> 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);
> 
> 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.
> 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.
> 
> 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.
> 
> --
> Thiago Macieira - thiago.macieira (AT) intel.com
>  Principal Engineer - Intel Platform Engineering Group
> -- 
> Development mailing list
> Development at qt-project.org
> https://lists.qt-project.org/listinfo/development