From 859ef056331a94fb0b1e4b41f596ff78539dfd8b Mon Sep 17 00:00:00 2001 From: Ievgenii Meshcheriakov Date: Tue, 2 May 2023 15:25:51 +0200 Subject: [PATCH] QDBusConnectionPrivate: Fix handling of queued messages Handle any queued messages before attempting to dispatch any newly received messages. This ensures that messages are processed in the order they were sent. Add a regression test for this bug using code adapted from the bug report by Pascal Weisser. Because of the nature of the bug, this new test does not always fail even when compiled with affected versions of Qt though. Fixes: QTBUG-105457 Pick-to: 6.2 6.5 Change-Id: I2725f3450ad537d63d6660e21645ac2c578e1768 Reviewed-by: Thiago Macieira --- src/dbus/qdbusintegrator.cpp | 2 +- tests/auto/dbus/CMakeLists.txt | 1 + .../CMakeLists.txt | 13 +++ .../tst_qdbusconnection_signalorder.cpp | 103 ++++++++++++++++++ 4 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 tests/auto/dbus/qdbusconnection_signalorder/CMakeLists.txt create mode 100644 tests/auto/dbus/qdbusconnection_signalorder/tst_qdbusconnection_signalorder.cpp diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp index 519b8d1fa4..5f95465b30 100644 --- a/src/dbus/qdbusintegrator.cpp +++ b/src/dbus/qdbusintegrator.cpp @@ -1173,7 +1173,6 @@ void QDBusConnectionPrivate::timerEvent(QTimerEvent *e) void QDBusConnectionPrivate::doDispatch() { if (mode == ClientMode || mode == PeerMode) { - while (q_dbus_connection_dispatch(connection) == DBUS_DISPATCH_DATA_REMAINS) ; if (dispatchEnabled && !pendingMessages.isEmpty()) { // dispatch previously queued messages PendingMessageList::Iterator it = pendingMessages.begin(); @@ -1184,6 +1183,7 @@ void QDBusConnectionPrivate::doDispatch() } pendingMessages.clear(); } + while (q_dbus_connection_dispatch(connection) == DBUS_DISPATCH_DATA_REMAINS) ; } } diff --git a/tests/auto/dbus/CMakeLists.txt b/tests/auto/dbus/CMakeLists.txt index 6ca0eb050e..d67877a0b3 100644 --- a/tests/auto/dbus/CMakeLists.txt +++ b/tests/auto/dbus/CMakeLists.txt @@ -6,6 +6,7 @@ add_subdirectory(qdbusconnection) add_subdirectory(qdbusconnection_no_app) add_subdirectory(qdbusconnection_no_bus) add_subdirectory(qdbusconnection_no_libdbus) +add_subdirectory(qdbusconnection_signalorder) add_subdirectory(qdbusconnection_spyhook) add_subdirectory(qdbuscontext) add_subdirectory(qdbuslocalcalls) diff --git a/tests/auto/dbus/qdbusconnection_signalorder/CMakeLists.txt b/tests/auto/dbus/qdbusconnection_signalorder/CMakeLists.txt new file mode 100644 index 0000000000..32dda2f3f3 --- /dev/null +++ b/tests/auto/dbus/qdbusconnection_signalorder/CMakeLists.txt @@ -0,0 +1,13 @@ +# Copyright (C) 2023 The Qt Company Ltd. +# SPDX-License-Identifier: BSD-3-Clause + +##################################################################### +## tst_qdbusconnection_signalorder Test: +##################################################################### + +qt_internal_add_test(tst_qdbusconnection_signalorder + SOURCES + tst_qdbusconnection_signalorder.cpp + LIBRARIES + Qt::DBus +) diff --git a/tests/auto/dbus/qdbusconnection_signalorder/tst_qdbusconnection_signalorder.cpp b/tests/auto/dbus/qdbusconnection_signalorder/tst_qdbusconnection_signalorder.cpp new file mode 100644 index 0000000000..dd21ecec5f --- /dev/null +++ b/tests/auto/dbus/qdbusconnection_signalorder/tst_qdbusconnection_signalorder.cpp @@ -0,0 +1,103 @@ +// Copyright (C) 2023 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0 + +#include +#include +#include +#include +#include +#include + +using namespace Qt::StringLiterals; + +static constexpr int MAX_TEST_DURATION_MS = 10000; +static constexpr int NUM_MESSAGES = 20; + +class SignalReceiver : public QObject +{ + Q_OBJECT +public: + explicit SignalReceiver(QDBusConnection &connection, const QString &serviceName); + + int nextValue() const { return m_nextValue; } + bool inOrder() const { return m_inOrder; } + +Q_SIGNALS: + void testSignal(int); + void done(); + +private Q_SLOTS: + void testSlot(int number); + +private: + int m_nextValue = 0; + bool m_inOrder = true; +}; + +SignalReceiver::SignalReceiver(QDBusConnection &connection, const QString &serviceName) +{ + connection.connect(serviceName, "/", {}, "testSignal", this, SLOT(testSlot(int))); +} + +void SignalReceiver::testSlot(int number) +{ + if (m_nextValue != number) { + m_inOrder = false; + qWarning("Message out of sequence, expected: %d, received: %d", m_nextValue, number); + } + + m_nextValue++; + + if (m_nextValue == NUM_MESSAGES) { + Q_EMIT done(); + } +} + +class tst_QDBusConnection_SignalOrder : public QObject +{ + Q_OBJECT +private Q_SLOTS: + void signalOrder(); +}; + +// This is a regression test for QTBUG-105457. The bug is a race condition, +// so it cannot be reliably triggered at each test execution. +void tst_QDBusConnection_SignalOrder::signalOrder() +{ + int argc = 1; + static char appName[] = "tst_qdbusconnection_signalorder"; + char *argv[] = { appName, 0 }; + QCoreApplication app(argc, argv); + + const QString serviceName = + u"org.qtproject.tst_dbusconnection_signalorder_%1"_s.arg(app.applicationPid()); + + auto connection = QDBusConnection::sessionBus(); + + QVERIFY(connection.isConnected()); + QVERIFY(connection.registerService(serviceName)); + + // Limit the test execution time in case if something goes wrong inside + // the signal receiver. + QTimer::singleShot(MAX_TEST_DURATION_MS, &app, &QCoreApplication::quit); + + SignalReceiver signalReceiver(connection, serviceName); + connect(&signalReceiver, &SignalReceiver::done, &app, &QCoreApplication::quit); + + QVERIFY(connection.registerObject("/", &signalReceiver, QDBusConnection::ExportAllSlots)); + + for (int i = 0; i < NUM_MESSAGES; i++) { + auto testSignal = QDBusMessage::createSignal("/", serviceName, "testSignal"); + testSignal << i; + QVERIFY(connection.send(testSignal)); + } + + app.exec(); + + QVERIFY(signalReceiver.inOrder()); + QCOMPARE(signalReceiver.nextValue(), NUM_MESSAGES); +} + +QTEST_APPLESS_MAIN(tst_QDBusConnection_SignalOrder) + +#include "tst_qdbusconnection_signalorder.moc"