QTimer: fix new-style slot invocation for receiver in another thread

For single-shot timers, at least. QSingleShotTimer had either an
optimization or the only way to make new-style slot invocations by
storing the QSlotObject pointer and calling it directly. Instead of
doing that, let's just actually connect and let QObject handle the
actual delivery.

[ChangeLog][QtCore][QTimer] Fixed a bug that caused slots connected to
single-slot timers using the new-style connection mechanism to be
delivered in the wrong thread.

Fixes: QTBUG-112162
Pick-to: 5.15 6.2 6.5
Change-Id: Idd5e1bb52be047d7b4fffffd174eadb227ab65ee
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
Reviewed-by: Ahmad Samir <a.samirh78@gmail.com>
This commit is contained in:
Thiago Macieira 2023-03-21 21:38:11 -10:00
parent f7167ae82f
commit 4d90c4e74a
2 changed files with 21 additions and 23 deletions

View File

@ -7,10 +7,11 @@
#include "qabstracteventdispatcher.h" #include "qabstracteventdispatcher.h"
#include "qcoreapplication.h" #include "qcoreapplication.h"
#include "qobject_p.h"
#include "qthread.h"
#include "qcoreapplication_p.h" #include "qcoreapplication_p.h"
#include "qmetaobject_p.h"
#include "qobject_p.h"
#include "qproperty_p.h" #include "qproperty_p.h"
#include "qthread.h"
QT_BEGIN_NAMESPACE QT_BEGIN_NAMESPACE
@ -249,9 +250,6 @@ class QSingleShotTimer : public QObject
{ {
Q_OBJECT Q_OBJECT
int timerId; int timerId;
bool hasValidReceiver;
QPointer<const QObject> receiver;
QtPrivate::QSlotObjectBase *slotObj;
public: public:
~QSingleShotTimer(); ~QSingleShotTimer();
QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, const char * m); QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, const char * m);
@ -264,16 +262,23 @@ protected:
}; };
QSingleShotTimer::QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, const char *member) QSingleShotTimer::QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, const char *member)
: QObject(QAbstractEventDispatcher::instance()), hasValidReceiver(true), slotObj(nullptr) : QObject(QAbstractEventDispatcher::instance())
{ {
timerId = startTimer(std::chrono::milliseconds{msec}, timerType); timerId = startTimer(std::chrono::milliseconds{msec}, timerType);
connect(this, SIGNAL(timeout()), r, member); connect(this, SIGNAL(timeout()), r, member);
} }
QSingleShotTimer::QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, QtPrivate::QSlotObjectBase *slotObj) QSingleShotTimer::QSingleShotTimer(int msec, Qt::TimerType timerType, const QObject *r, QtPrivate::QSlotObjectBase *slotObj)
: QObject(QAbstractEventDispatcher::instance()), hasValidReceiver(r), receiver(r), slotObj(slotObj) : QObject(QAbstractEventDispatcher::instance())
{ {
timerId = startTimer(std::chrono::milliseconds{msec}, timerType); timerId = startTimer(std::chrono::milliseconds{msec}, timerType);
int signal_index = QMetaObjectPrivate::signalOffset(&staticMetaObject);
Q_ASSERT(QMetaObjectPrivate::signal(&staticMetaObject, signal_index).name() == "timeout");
QObjectPrivate::connectImpl(this, signal_index, r ? r : this, nullptr, slotObj,
Qt::AutoConnection, nullptr, &staticMetaObject);
// ### Why is this here? Why doesn't the case above need it?
if (r && thread() != r->thread()) { if (r && thread() != r->thread()) {
// Avoid leaking the QSingleShotTimer instance in case the application exits before the timer fires // Avoid leaking the QSingleShotTimer instance in case the application exits before the timer fires
connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QObject::deleteLater); connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QObject::deleteLater);
@ -286,8 +291,6 @@ QSingleShotTimer::~QSingleShotTimer()
{ {
if (timerId > 0) if (timerId > 0)
killTimer(timerId); killTimer(timerId);
if (slotObj)
slotObj->destroyIfLastRef();
} }
void QSingleShotTimer::timerEvent(QTimerEvent *) void QSingleShotTimer::timerEvent(QTimerEvent *)
@ -298,17 +301,7 @@ void QSingleShotTimer::timerEvent(QTimerEvent *)
killTimer(timerId); killTimer(timerId);
timerId = -1; timerId = -1;
if (slotObj) { emit timeout();
// If the receiver was destroyed, skip this part
if (Q_LIKELY(!receiver.isNull() || !hasValidReceiver)) {
// We allocate only the return type - we previously checked the function had
// no arguments.
void *args[1] = { nullptr };
slotObj->call(const_cast<QObject*>(receiver.data()), args);
}
} else {
emit timeout();
}
// we would like to use delete later here, but it feels like a // we would like to use delete later here, but it feels like a
// waste to post a new event to handle this event, so we just unset the flag // waste to post a new event to handle this event, so we just unset the flag

View File

@ -1006,18 +1006,21 @@ void tst_QTimer::postedEventsShouldNotStarveTimers()
} }
struct DummyFunctor { struct DummyFunctor {
void operator()() {} static QThread *callThread;
void operator()() { callThread = QThread::currentThread(); }
}; };
QThread *DummyFunctor::callThread = nullptr;
void tst_QTimer::crossThreadSingleShotToFunctor() void tst_QTimer::crossThreadSingleShotToFunctor()
{ {
// We're testing for crashes here, so the test simply running to // We're also testing for crashes here, so the test simply running to
// completion is considered a success // completion is part of the success
QThread t; QThread t;
t.start(); t.start();
QObject* o = new QObject(); QObject* o = new QObject();
o->moveToThread(&t); o->moveToThread(&t);
DummyFunctor::callThread = nullptr;
for (int i = 0; i < 10000; i++) { for (int i = 0; i < 10000; i++) {
QTimer::singleShot(0, o, DummyFunctor()); QTimer::singleShot(0, o, DummyFunctor());
@ -1026,6 +1029,8 @@ void tst_QTimer::crossThreadSingleShotToFunctor()
t.quit(); t.quit();
t.wait(); t.wait();
delete o; delete o;
QCOMPARE(DummyFunctor::callThread, &t);
} }
void tst_QTimer::callOnTimeout() void tst_QTimer::callOnTimeout()