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
smime.p7s
Description: S/MIME cryptographic signature
-- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
