Implement the blocking QtDBus call in terms of the non-blocking one

This simplifies the code a little by having a single code path. More
importantly, we no longer need to call the evil function
dbus_connection_send_with_reply_and_block. That function acquires a lock
on the socket transport inside libdbus-1, which means all threads need
to wait until the one call gets unblocked before they can continue.

To do that, this commit reimplements the QDBus::Block part of
QDBusConnectionPrivate::sendWithReply by reusing the existing call to
sendWithReplyAsync() and then doing a blocking-wait with
QDBusPendingCallPrivate::waitForFinished().

By using (Q)DBusPendingCall and the threaded connection approach (next
commit), now we never block on the socket. That also means the code to
call dbus_pending_call_block() is no longer necessary and the
waitForFinished() function itself can be considerably simplified.

As a side-effect of no longer blocking, a number of pre-existing race
conditions that used to be hidden showed up.

Note: this commit deadlocks without the threading (next commits).

Task-number: QTBUG-43585
Change-Id: Ic5d393bfd36e48a193fcffff13b73754954a3f7d
Reviewed-by: Albert Astals Cid <aacid@kde.org>
Reviewed-by: Alex Blasche <alexander.blasche@theqtcompany.com>

6 files changed, 23 insertions, 86 deletions

diff --git a/src/dbus/qdbus_symbols_p.h b/src/dbus/qdbus_symbols_p.h
index 32b76ee5bd..1991a462e9 100644
--- a/src/dbus/qdbus_symbols_p.h
+++ b/src/dbus/qdbus_symbols_p.h
@@ -1,6 +1,7 @@
 /****************************************************************************
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2015 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -205,11 +206,6 @@ DEFINEFUNC(dbus_bool_t        , dbus_connection_send_with_reply, (DBusConnection
                                                                   DBusPendingCall           **pending_return,
                                                                   int                         timeout_milliseconds),
            (connection, message, pending_return, timeout_milliseconds), return)
-DEFINEFUNC(DBusMessage *      , dbus_connection_send_with_reply_and_block, (DBusConnection             *connection,
-                                                                            DBusMessage                *message,
-                                                                            int                         timeout_milliseconds,
-                                                                            DBusError                  *error),
-           (connection, message, timeout_milliseconds, error), return)
 DEFINEFUNC(void               , dbus_connection_set_exit_on_disconnect, (DBusConnection             *connection,
                                                                          dbus_bool_t                 exit_on_disconnect),
            (connection, exit_on_disconnect), )
diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h
index 048e255e88..2155bd634c 100644
--- a/src/dbus/qdbusconnection_p.h
+++ b/src/dbus/qdbusconnection_p.h
@@ -223,7 +223,6 @@ public:
     void unregisterService(const QString &serviceName);
 
     bool handleMessage(const QDBusMessage &msg);
-    void waitForFinished(QDBusPendingCallPrivate *pcall);
 
     QDBusMetaObject *findMetaObject(const QString &service, const QString &path,
                                     const QString &interface, QDBusError &error);
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index 08a8877ad5..4ae6a7f351 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -1790,34 +1790,6 @@ static void qDBusResultReceived(DBusPendingCall *pending, void *user_data)
 }
 }
 
-void QDBusConnectionPrivate::waitForFinished(QDBusPendingCallPrivate *pcall)
-{
-    Q_ASSERT(pcall->pending);
-    //Q_ASSERT(pcall->mutex.isLocked()); // there's no such function
-
-    if (pcall->waitingForFinished) {
-        // another thread is already waiting
-        pcall->waitForFinishedCondition.wait(&pcall->mutex);
-    } else {
-        pcall->waitingForFinished = true;
-        pcall->mutex.unlock();
-
-        {
-            QDBusDispatchLocker locker(PendingCallBlockAction, this);
-            q_dbus_pending_call_block(pcall->pending);
-            // QDBusConnectionPrivate::processFinishedCall() is called automatically
-        }
-        pcall->mutex.lock();
-
-        if (pcall->pending) {
-            q_dbus_pending_call_unref(pcall->pending);
-            pcall->pending = 0;
-        }
-
-        pcall->waitForFinishedCondition.wakeAll();
-    }
-}
-
 void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call)
 {
     QDBusConnectionPrivate *connection = const_cast<QDBusConnectionPrivate *>(call->connection);
@@ -1833,7 +1805,7 @@ void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call)
         msg = QDBusMessagePrivate::fromDBusMessage(reply, connection->capabilities);
         q_dbus_message_unref(reply);
     }
-    qDBusDebug() << connection << "got message reply (async):" << msg;
+    qDBusDebug() << connection << "got message reply:" << msg;
 
     // Check if the reply has the expected signature
     call->checkReceivedSignature();
@@ -1855,7 +1827,8 @@ void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call)
             qDBusDebug() << "Deliver failed!";
     }
 
-    if (call->pending && !call->waitingForFinished) {
+    if (call->pending) {
+        call->waitForFinishedCondition.wakeAll();
         q_dbus_pending_call_unref(call->pending);
         call->pending = 0;
     }
@@ -2008,43 +1981,12 @@ QDBusMessage QDBusConnectionPrivate::sendWithReply(const QDBusMessage &message,
         // special case for synchronous local calls
         return sendWithReplyLocal(message);
 
-    if (!QCoreApplication::instance() || sendMode == QDBus::Block) {
-        QDBusError err;
-        DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message, capabilities, &err);
-        if (!msg) {
-            qWarning("QDBusConnection: error: could not send message to service \"%s\" path \"%s\" interface \"%s\" member \"%s\": %s",
-                     qPrintable(message.service()), qPrintable(message.path()),
-                     qPrintable(message.interface()), qPrintable(message.member()),
-                     qPrintable(err.message()));
-            lastError = err;
-            return QDBusMessage::createError(err);
-        }
-
-        qDBusDebug() << this << "sending message (blocking):" << message;
-        QDBusErrorInternal error;
-        DBusMessage *reply;
-        {
-            QDBusDispatchLocker locker(SendWithReplyAndBlockAction, this);
-            reply = q_dbus_connection_send_with_reply_and_block(connection, msg, timeout, error);
-        }
-
-        q_dbus_message_unref(msg);
-
-        if (!!error) {
-            lastError = err = error;
-            return QDBusMessage::createError(err);
-        }
-
-        QDBusMessage amsg = QDBusMessagePrivate::fromDBusMessage(reply, capabilities);
-        q_dbus_message_unref(reply);
-        qDBusDebug() << this << "got message reply (blocking):" << amsg;
-
-        return amsg;
-    } else { // use the event loop
-        QDBusPendingCallPrivate *pcall = sendWithReplyAsync(message, 0, 0, 0, timeout);
-        Q_ASSERT(pcall);
+    QDBusPendingCallPrivate *pcall = sendWithReplyAsync(message, 0, 0, 0, timeout);
+    Q_ASSERT(pcall);
 
-        if (pcall->replyMessage.type() == QDBusMessage::InvalidMessage) {
+    if (pcall->replyMessage.type() == QDBusMessage::InvalidMessage) {
+        // need to wait for the reply
+        if (sendMode == QDBus::BlockWithGui) {
             pcall->watcherHelper = new QDBusPendingCallWatcherHelper;
             QEventLoop loop;
             loop.connect(pcall->watcherHelper, SIGNAL(reply(QDBusMessage)), SLOT(quit()));
@@ -2052,18 +1994,17 @@ QDBusMessage QDBusConnectionPrivate::sendWithReply(const QDBusMessage &message,
 
             // enter the event loop and wait for a reply
             loop.exec(QEventLoop::ExcludeUserInputEvents | QEventLoop::WaitForMoreEvents);
+        } else {
+            pcall->waitForFinished();
         }
+    }
 
-        QDBusMessage reply = pcall->replyMessage;
-        lastError = QDBusError(reply);      // set or clear error
-
-        bool r = pcall->ref.deref();
-        Q_ASSERT(!r);
-        Q_UNUSED(r);
+    QDBusMessage reply = pcall->replyMessage;
+    lastError = QDBusError(reply);      // set or clear error
 
+    if (!pcall->ref.deref())
         delete pcall;
-        return reply;
-    }
+    return reply;
 }
 
 QDBusMessage QDBusConnectionPrivate::sendWithReplyLocal(const QDBusMessage &message)
@@ -2163,7 +2104,7 @@ QDBusPendingCallPrivate *QDBusConnectionPrivate::sendWithReplyAsync(const QDBusM
         return pcall;
     }
 
-    qDBusDebug() << this << "sending message (async):" << message;
+    qDBusDebug() << this << "sending message:" << message;
     DBusPendingCall *pending = 0;
 
     QDBusDispatchLocker locker(SendWithReplyAsyncAction, this);
diff --git a/src/dbus/qdbuspendingcall.cpp b/src/dbus/qdbuspendingcall.cpp
index 09eff81107..ad5632be5a 100644
--- a/src/dbus/qdbuspendingcall.cpp
+++ b/src/dbus/qdbuspendingcall.cpp
@@ -1,6 +1,7 @@
 /****************************************************************************
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2015 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -231,7 +232,7 @@ void QDBusPendingCallPrivate::waitForFinished()
     if (replyMessage.type() != QDBusMessage::InvalidMessage)
         return;                 // already finished
 
-    connection->waitForFinished(this);
+    waitForFinishedCondition.wait(&mutex);
 }
 
 /*!
diff --git a/src/dbus/qdbuspendingcall_p.h b/src/dbus/qdbuspendingcall_p.h
index ac39487fee..dcf733679c 100644
--- a/src/dbus/qdbuspendingcall_p.h
+++ b/src/dbus/qdbuspendingcall_p.h
@@ -1,6 +1,7 @@
 /****************************************************************************
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2015 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -89,11 +90,10 @@ public:
     DBusPendingCall *pending;
     QString expectedReplySignature;
     int expectedReplyCount;
-    bool waitingForFinished;
     // }
 
     QDBusPendingCallPrivate(const QDBusMessage &sent, QDBusConnectionPrivate *connection)
-        : sentMessage(sent), connection(connection), watcherHelper(0), pending(0), waitingForFinished(false)
+        : sentMessage(sent), connection(connection), watcherHelper(0), pending(0)
     { }
     ~QDBusPendingCallPrivate();
     bool setReplyCallback(QObject *target, const char *member);
diff --git a/src/dbus/qdbusthreaddebug_p.h b/src/dbus/qdbusthreaddebug_p.h
index a8e126cf73..60b6acd38d 100644
--- a/src/dbus/qdbusthreaddebug_p.h
+++ b/src/dbus/qdbusthreaddebug_p.h
@@ -1,6 +1,7 @@
 /****************************************************************************
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2015 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -87,8 +88,7 @@ enum ThreadAction {
     ActivateSignalAction = 27,
     PendingCallBlockAction = 28,
     SendMessageAction = 29,
-    SendWithReplyAndBlockAction = 30,
-    HuntAndEmitAction = 31,
+    HuntAndEmitAction = 30,
 
     AddTimeoutAction = 50,
     RealAddTimeoutAction = 51,