Change the QtDBus watched service list to a map to its owner.

Instead of simply counting how many times we're watching a service,
also map a service to its owner. This allows us to provide a faster,
cached GetNameOwner too.

Technically, this could mean that a service that is started in-between
event loops could remain invalid for a longer period of time. I'm not
sure this is an issue -- will need some testing in KDE.

Task-number: QTBUG-5979
Reviewed-By: TrustMe

2 files changed, 48 insertions, 17 deletions

diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h
index d6f7598..5cffdce 100644
--- a/src/dbus/qdbusconnection_p.h
+++ b/src/dbus/qdbusconnection_p.h
@@ -155,7 +155,13 @@ public:
     typedef QMultiHash<QString, SignalHook> SignalHookHash;
     typedef QHash<QString, QDBusMetaObject* > MetaObjectHash;
     typedef QHash<QByteArray, int> MatchRefCountHash;
-    typedef QHash<QString, int> WatchedServicesHash;
+
+    struct WatchedServiceData {
+        WatchedServiceData() : refcount(0) {}
+        QString owner;
+        int refcount;
+    };
+    typedef QHash<QString, WatchedServiceData> WatchedServicesHash;
 
 public:
     // public methods are entry points from other objects
@@ -223,6 +229,8 @@ private:
 
     bool isServiceRegisteredByThread(const QString &serviceName) const;
 
+    QString getNameOwnerNoCache(const QString &service);
+
 protected:
     void customEvent(QEvent *e);
     void timerEvent(QTimerEvent *e);
@@ -271,7 +279,7 @@ public:
     QDBusError lastError;
 
     QStringList serviceNames;
-    WatchedServicesHash watchedServiceNames;
+    WatchedServicesHash watchedServices;
     SignalHookHash signalHooks;
     MatchRefCountHash matchRefCounts;
     ObjectTreeNode rootNode;
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index 278da3f..0c436f0 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -947,7 +947,6 @@ QDBusConnectionPrivate::QDBusConnectionPrivate(QObject *p)
     QDBusMetaTypeId::init();
 
     rootNode.flags = 0;
-    watchedServiceNames[QLatin1String(DBUS_SERVICE_DBUS)] = 1;
 }
 
 QDBusConnectionPrivate::~QDBusConnectionPrivate()
@@ -1179,11 +1178,15 @@ void QDBusConnectionPrivate::_q_serviceOwnerChanged(const QString &name,
 {
     Q_UNUSED(oldOwner);
     QDBusWriteLocker locker(UpdateSignalHookOwnerAction, this);
-    QMutableHashIterator<QString, SignalHook> it(signalHooks);
-    it.toFront();
-    while (it.hasNext())
-        if (it.next().value().service == name)
-            it.value().owner = newOwner;
+    WatchedServicesHash::Iterator it = watchedServices.find(name);
+    if (it == watchedServices.end())
+        return;
+    if (oldOwner != it->owner)
+        qWarning("QDBusConnection: name '%s' had owner '%s' but we thought it was '%s'",
+                 qPrintable(name), qPrintable(oldOwner), qPrintable(it->owner));
+
+    qDBusDebug() << this << "Updating name" << name << "from" << oldOwner << "to" << newOwner;
+    it->owner = newOwner;
 }
 
 int QDBusConnectionPrivate::findSlot(QObject* obj, const QByteArray &normalizedName,
@@ -1652,6 +1655,9 @@ void QDBusConnectionPrivate::setConnection(DBusConnection *dbc, const QDBusError
     }
 
     QString busService = QLatin1String(DBUS_SERVICE_DBUS);
+    WatchedServicesHash::mapped_type &bus = watchedServices[busService];
+    bus.refcount = 1;
+    bus.owner = getNameOwnerNoCache(busService);
     connectSignal(busService, QString(), QString(), QString(), QLatin1String("NameAcquired"), QStringList(), QString(),
                   this, SLOT(registerService(QString)));
     connectSignal(busService, QString(), QString(), QString(), QLatin1String("NameLost"), QStringList(), QString(),
@@ -2035,16 +2041,19 @@ void QDBusConnectionPrivate::connectSignal(const QString &key, const SignalHook
             // Successfully connected the signal
             // Do we need to watch for this name?
             if (shouldWatchService(hook.service)) {
-                WatchedServicesHash::Iterator it = watchedServiceNames.find(hook.service);
-                if (it != watchedServiceNames.end()) {
+                WatchedServicesHash::mapped_type &data = watchedServices[hook.service];
+                if (data.refcount) {
                     // already watching
-                    ++it.value();
+                    ++data.refcount;
                 } else {
                     // we need to watch for this service changing
                     QString dbusServerService = QLatin1String(DBUS_SERVICE_DBUS);
                     connectSignal(dbusServerService, dbusServerService, QString(), QLatin1String(DBUS_INTERFACE_DBUS),
                                   QLatin1String("NameOwnerChanged"), QStringList() << hook.service, QString(),
                                   this, SLOT(_q_serviceOwnerChanged(QString,QString,QString)));
+                    data.owner = getNameOwnerNoCache(hook.service);
+                    qDBusDebug() << this << "Watching service" << hook.service << "for owner changes (current owner:"
+                            << data.owner << ")";
                 }
             }
         }
@@ -2093,16 +2102,16 @@ QDBusConnectionPrivate::disconnectSignal(SignalHookHash::Iterator &it)
 {
     const SignalHook &hook = it.value();
 
-    WatchedServicesHash::Iterator sit = watchedServiceNames.find(hook.service);
-    if (sit != watchedServiceNames.end()) {
-        if (sit.value() == 1) {
-            watchedServiceNames.erase(sit);
+    WatchedServicesHash::Iterator sit = watchedServices.find(hook.service);
+    if (sit != watchedServices.end()) {
+        if (sit.value().refcount == 1) {
+            watchedServices.erase(sit);
             QString dbusServerService = QLatin1String(DBUS_SERVICE_DBUS);
             disconnectSignal(dbusServerService, QString(), QLatin1String(DBUS_INTERFACE_DBUS),
                           QLatin1String("NameOwnerChanged"), QStringList() << hook.service, QString(),
                           this, SLOT(_q_serviceOwnerChanged(QString,QString,QString)));
         } else {
-            --sit.value();
+            --sit.value().refcount;
         }
     }
 
@@ -2225,9 +2234,23 @@ QString QDBusConnectionPrivate::getNameOwner(const QString& serviceName)
 {
     if (QDBusUtil::isValidUniqueConnectionName(serviceName))
         return serviceName;
-    if (!connection || !QDBusUtil::isValidBusName(serviceName))
+    if (!connection)
         return QString();
 
+    {
+        // acquire a read lock for the cache
+        QReadLocker locker(&lock);
+        WatchedServicesHash::ConstIterator it = watchedServices.constFind(serviceName);
+        if (it != watchedServices.constEnd())
+            return it->owner;
+    }
+
+    // not cached
+    return getNameOwnerNoCache(serviceName);
+}
+
+QString QDBusConnectionPrivate::getNameOwnerNoCache(const QString &serviceName)
+{
     QDBusMessage msg = QDBusMessage::createMethodCall(QLatin1String(DBUS_SERVICE_DBUS),
             QLatin1String(DBUS_PATH_DBUS), QLatin1String(DBUS_INTERFACE_DBUS),
             QLatin1String("GetNameOwner"));