Fix a race condition related to service acquisition.

The explanation is in the testcase and in the task.

The reentrancy caused some deadlocks, that's why handleMessage() stops
processing if the refcount has dropped down to zero. Should also save
some CPU cycles at the application shutdown time.

Task-number: QTBUG-15651
Reviewed-by: Trust Me

3 files changed, 108 insertions, 9 deletions

diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h
index 1bd00da..67145b8 100644
--- a/src/dbus/qdbusconnection_p.h
+++ b/src/dbus/qdbusconnection_p.h
@@ -203,6 +203,8 @@ public:
     void disconnectRelay(const QString &service,
                          const QString &path, const QString &interface,
                          QDBusAbstractInterface *receiver, const char *signal);
+    void registerService(const QString &serviceName);
+    void unregisterService(const QString &serviceName);
 
     bool handleMessage(const QDBusMessage &msg);
     void waitForFinished(QDBusPendingCallPrivate *pcall);
@@ -247,9 +249,11 @@ public slots:
     void socketWrite(int);
     void objectDestroyed(QObject *o);
     void relaySignal(QObject *obj, const QMetaObject *, int signalId, const QVariantList &args);
-    void _q_serviceOwnerChanged(const QString &name, const QString &oldOwner, const QString &newOwner);
-    void registerService(const QString &serviceName);
-    void unregisterService(const QString &serviceName);
+
+private slots:
+    void serviceOwnerChangedNoLock(const QString &name, const QString &oldOwner, const QString &newOwner);
+    void registerServiceNoLock(const QString &serviceName);
+    void unregisterServiceNoLock(const QString &serviceName);
 
 signals:
     void serviceOwnerChanged(const QString &name, const QString &oldOwner, const QString &newOwner);
@@ -303,6 +307,9 @@ public:
                             QObject *receiver, const char *signal, int minMIdx,
                             bool buildSignature);
     static DBusHandlerResult messageFilter(DBusConnection *, DBusMessage *, void *);
+    static bool checkReplyForDelivery(QDBusConnectionPrivate *target, QObject *object,
+                                      int idx, const QList<int> &metaTypes,
+                                      const QDBusMessage &msg);
     static QDBusCallDeliveryEvent *prepareReply(QDBusConnectionPrivate *target, QObject *object,
                                                 int idx, const QList<int> &metaTypes,
                                                 const QDBusMessage &msg);
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index a5d8ada..1842e5a 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -552,6 +552,9 @@ bool QDBusConnectionPrivate::handleMessage(const QDBusMessage &amsg)
         (*(*list)[i])(amsg);
     }
 
+    if (!ref)
+        return false;
+
     switch (amsg.type()) {
     case QDBusMessage::SignalMessage:
         handleSignal(amsg);
@@ -713,6 +716,8 @@ static int findSlot(const QMetaObject *mo, const QByteArray &name, int flags,
     return -1;
 }
 
+static QDBusCallDeliveryEvent * const DIRECT_DELIVERY = (QDBusCallDeliveryEvent *)1;
+
 QDBusCallDeliveryEvent* QDBusConnectionPrivate::prepareReply(QDBusConnectionPrivate *target,
                                                              QObject *object, int idx,
                                                              const QList<int> &metaTypes,
@@ -736,6 +741,8 @@ QDBusCallDeliveryEvent* QDBusConnectionPrivate::prepareReply(QDBusConnectionPriv
 
     // we can deliver
     // prepare for the call
+    if (target == object)
+        return DIRECT_DELIVERY;
     return new QDBusCallDeliveryEvent(QDBusConnection(target), idx, target, msg, metaTypes);
 }
 
@@ -750,6 +757,12 @@ void QDBusConnectionPrivate::activateSignal(const QDBusConnectionPrivate::Signal
     // Slots can optionally have one final parameter that is a QDBusMessage
     // Slots receive read-only copies of the message (i.e., pass by value or by const-ref)
     QDBusCallDeliveryEvent *call = prepareReply(this, hook.obj, hook.midx, hook.params, msg);
+    if (call == DIRECT_DELIVERY) {
+        // short-circuit delivery
+        Q_ASSERT(this == hook.obj);
+        deliverCall(this, 0, msg, hook.params, hook.midx);
+        return;
+    }
     if (call)
         postEventToThread(ActivateSignalAction, hook.obj, call);
 }
@@ -1207,11 +1220,11 @@ void QDBusConnectionPrivate::relaySignal(QObject *obj, const QMetaObject *mo, in
     q_dbus_message_unref(msg);
 }
 
-void QDBusConnectionPrivate::_q_serviceOwnerChanged(const QString &name,
-                                                    const QString &oldOwner, const QString &newOwner)
+void QDBusConnectionPrivate::serviceOwnerChangedNoLock(const QString &name,
+                                                       const QString &oldOwner, const QString &newOwner)
 {
     Q_UNUSED(oldOwner);
-    QDBusWriteLocker locker(UpdateSignalHookOwnerAction, this);
+//    QDBusWriteLocker locker(UpdateSignalHookOwnerAction, this);
     WatchedServicesHash::Iterator it = watchedServices.find(name);
     if (it == watchedServices.end())
         return;
@@ -1686,11 +1699,11 @@ void QDBusConnectionPrivate::setConnection(DBusConnection *dbc, const QDBusError
     hook.obj = this;
     hook.params << QMetaType::Void << QVariant::String; // both functions take a QString as parameter and return void
 
-    hook.midx = staticMetaObject.indexOfSlot("registerService(QString)");
+    hook.midx = staticMetaObject.indexOfSlot("registerServiceNoLock(QString)");
     Q_ASSERT(hook.midx != -1);
     signalHooks.insert(QLatin1String("NameAcquired:" DBUS_INTERFACE_DBUS), hook);
 
-    hook.midx = staticMetaObject.indexOfSlot("unregisterService(QString)");
+    hook.midx = staticMetaObject.indexOfSlot("unregisterServiceNoLock(QString)");
     Q_ASSERT(hook.midx != -1);
     signalHooks.insert(QLatin1String("NameLost:" DBUS_INTERFACE_DBUS), hook);
 
@@ -2081,7 +2094,7 @@ void QDBusConnectionPrivate::connectSignal(const QString &key, const SignalHook
                     // we need to watch for this service changing
                     connectSignal(dbusServiceString(), QString(), dbusInterfaceString(),
                                   QLatin1String("NameOwnerChanged"), QStringList() << hook.service, QString(),
-                                  this, SLOT(_q_serviceOwnerChanged(QString,QString,QString)));
+                                  this, SLOT(serviceOwnerChangedNoLock(QString,QString,QString)));
                     data.owner = getNameOwnerNoCache(hook.service);
                     qDBusDebug() << this << "Watching service" << hook.service << "for owner changes (current owner:"
                             << data.owner << ")";
@@ -2342,12 +2355,22 @@ QDBusConnectionPrivate::findMetaObject(const QString &service, const QString &pa
 void QDBusConnectionPrivate::registerService(const QString &serviceName)
 {
     QDBusWriteLocker locker(RegisterServiceAction, this);
+    registerServiceNoLock(serviceName);
+}
+
+void QDBusConnectionPrivate::registerServiceNoLock(const QString &serviceName)
+{
     serviceNames.append(serviceName);
 }
 
 void QDBusConnectionPrivate::unregisterService(const QString &serviceName)
 {
     QDBusWriteLocker locker(UnregisterServiceAction, this);
+    unregisterServiceNoLock(serviceName);
+}
+
+void QDBusConnectionPrivate::unregisterServiceNoLock(const QString &serviceName)
+{
     serviceNames.removeAll(serviceName);
 }
 
diff --git a/tests/auto/qdbusconnection/tst_qdbusconnection.cpp b/tests/auto/qdbusconnection/tst_qdbusconnection.cpp
index 599abbd..4494d6f 100644
--- a/tests/auto/qdbusconnection/tst_qdbusconnection.cpp
+++ b/tests/auto/qdbusconnection/tst_qdbusconnection.cpp
@@ -106,6 +106,8 @@ private slots:
     void slotsWithLessParameters();
     void nestedCallWithCallback();
 
+    void serviceRegistrationRaceCondition();
+
 public:
     QString serviceName() const { return "com.trolltech.Qt.Autotests.QDBusConnection"; }
     bool callMethod(const QDBusConnection &conn, const QString &path);
@@ -647,6 +649,73 @@ void tst_QDBusConnection::nestedCallWithCallback()
     QCOMPARE(signalsReceived, 1);
 }
 
+class RaceConditionSignalWaiter : public QObject
+{
+    Q_OBJECT
+public:
+    int count;
+    RaceConditionSignalWaiter() : count (0) {}
+    virtual ~RaceConditionSignalWaiter() {}
+
+public slots:
+    void countUp() { ++count; emit done(); }
+signals:
+    void done();
+};
+
+void tst_QDBusConnection::serviceRegistrationRaceCondition()
+{
+    // There was a race condition in the updating of list of name owners in
+    // QtDBus. When the user connects to a signal coming from a given
+    // service, we must listen for NameOwnerChanged signals relevant to that
+    // name and update when the owner changes. However, it's possible that we
+    // receive in one chunk from the server both the NameOwnerChanged signal
+    // about the service and the signal we're interested in. Since QtDBus
+    // posts events in order to handle the incoming signals, the update
+    // happens too late.
+
+    const QString connectionName = "testConnectionName";
+    const QString serviceName = "org.example.SecondaryName";
+
+    QDBusConnection session = QDBusConnection::sessionBus();
+    QVERIFY(!session.interface()->isServiceRegistered(serviceName));
+
+    // connect to the signal:
+    RaceConditionSignalWaiter recv;
+    session.connect(serviceName, "/", "com.trolltech.TestCase", "oneSignal", &recv, SLOT(countUp()));
+
+    // create a secondary connection and register a name
+    QDBusConnection connection = QDBusConnection::connectToBus(QDBusConnection::SessionBus, connectionName);
+    QDBusConnection::disconnectFromBus(connectionName); // disconnection happens when "connection" goes out of scope
+    QVERIFY(connection.isConnected());
+    QVERIFY(connection.registerService(serviceName));
+
+    // send a signal
+    QDBusMessage msg = QDBusMessage::createSignal("/", "com.trolltech.TestCase", "oneSignal");
+    connection.send(msg);
+
+    // make a blocking call just to be sure that the buffer was flushed
+    msg = QDBusMessage::createMethodCall("org.freedesktop.DBus", "/org/freedesktop/DBus", "org.freedesktop.DBus",
+                                         "NameHasOwner");
+    msg << connectionName;
+    connection.call(msg); // ignore result
+
+    // Now here's the race condition (more info on task QTBUG-15651):
+    // the bus has most likely queued three signals for us to work on:
+    // 1) NameOwnerChanged for the connection we created above
+    // 2) NameOwnerChanged for the service we registered above
+    // 3) The "oneSignal" signal we sent
+    //
+    // We'll most likely receive all three in one go from the server. We must
+    // update the owner of serviceName before we start processing the
+    // "oneSignal" signal.
+
+    QTestEventLoop::instance().connect(&recv, SIGNAL(done()), SLOT(exitLoop()));
+    QTestEventLoop::instance().enterLoop(1);
+    QVERIFY(!QTestEventLoop::instance().timeout());
+    QCOMPARE(recv.count, 1);
+}
+
 QString MyObject::path;
 QTEST_MAIN(tst_QDBusConnection)