Fix loss of valid dbus objects after unregisterObject

Partial revert of 3c6bb0ed8bfc9a2c679f4154585a16e47275ad21 and
57aed703d21c3a360d95fd9f85396d1283d3fdd0.

When registering an object that was previously unregistered but not yet
garbage collected, the activeChildren count on the parent node was not
incremented, which could result in other registered objects disappearing
after a later unregisterObject.

Copying objects in the tree is not free, but it's not expensive enough
or used frequently enough to justify that error-prone logic. It's much
safer to simply remove objects immediately.

Change-Id: I3dc59c2ebd07b237518424fcd8ea7371a22d6d15
Reviewed-by: Robin Burchell <robin+qt@viroteck.net>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

3 files changed, 11 insertions, 40 deletions

diff --git a/src/dbus/qdbusconnection.cpp b/src/dbus/qdbusconnection.cpp
index a29ba4cb0f..320787c265 100644
--- a/src/dbus/qdbusconnection.cpp
+++ b/src/dbus/qdbusconnection.cpp
@@ -804,7 +804,7 @@ bool QDBusConnection::registerObject(const QString &path, QObject *object, Regis
                 return false;
 
             if (options & QDBusConnectionPrivate::VirtualObject) {
-                if (options & SubPath && node->activeChildren)
+                if (options & SubPath && !node->children.isEmpty())
                     return false;
             } else {
                 if ((options & ExportChildObjects && !node->children.isEmpty()))
@@ -842,7 +842,6 @@ bool QDBusConnection::registerObject(const QString &path, QObject *object, Regis
             }
         } else {
             // add entry
-            ++node->activeChildren;
             node = node->children.insert(it, pathComponents.at(i));
         }
 
diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h
index c702de141a..a61fdaf6c3 100644
--- a/src/dbus/qdbusconnection_p.h
+++ b/src/dbus/qdbusconnection_p.h
@@ -140,16 +140,16 @@ public:
     {
         typedef QVector<ObjectTreeNode> DataList;
 
-        inline ObjectTreeNode() : obj(0), flags(0), activeChildren(0) { }
+        inline ObjectTreeNode() : obj(0), flags(0) { }
         inline ObjectTreeNode(const QString &n) // intentionally implicit
-            : name(n), obj(0), flags(0), activeChildren(0) { }
+            : name(n), obj(0), flags(0) { }
         inline ~ObjectTreeNode() { }
         inline bool operator<(const QString &other) const
             { return name < other; }
         inline bool operator<(const QStringRef &other) const
             { return QStringRef(&name) < other; }
         inline bool isActive() const
-        { return obj || activeChildren; }
+        { return obj || !children.isEmpty(); }
 
         QString name;
         union {
@@ -157,7 +157,6 @@ public:
             QDBusVirtualObject *treeNode;
         };
         int flags;
-        int activeChildren;
 
         DataList children;
     };
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index 3f25f02bee..c324449b7d 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -587,46 +587,22 @@ bool QDBusConnectionPrivate::handleMessage(const QDBusMessage &amsg)
     return false;
 }
 
-static void garbageCollectChildren(QDBusConnectionPrivate::ObjectTreeNode &node)
-{
-    int size = node.children.count();
-    if (node.activeChildren == 0) {
-        // easy case
-        node.children.clear();
-    } else if (size > node.activeChildren * 3 || (size > 20 && size * 2 > node.activeChildren * 3)) {
-        // rewrite the vector, keeping only the active children
-        // if the vector is large (> 20 items) and has one third of inactives
-        // or if the vector is small and has two thirds of inactives.
-        QDBusConnectionPrivate::ObjectTreeNode::DataList::Iterator end = node.children.end();
-        QDBusConnectionPrivate::ObjectTreeNode::DataList::Iterator it = node.children.begin();
-        QDBusConnectionPrivate::ObjectTreeNode::DataList::Iterator tgt = it;
-        for ( ; it != end; ++it) {
-            if (it->isActive())
-                *tgt++ = qMove(*it);
-        }
-        ++tgt;
-        node.children.erase(tgt, end);
-    }
-}
-
 static void huntAndDestroy(QObject *needle, QDBusConnectionPrivate::ObjectTreeNode &haystack)
 {
     QDBusConnectionPrivate::ObjectTreeNode::DataList::Iterator it = haystack.children.begin();
-    QDBusConnectionPrivate::ObjectTreeNode::DataList::Iterator end = haystack.children.end();
-    for ( ; it != end; ++it) {
-        if (!it->isActive())
-            continue;
+
+    while (it != haystack.children.end()) {
         huntAndDestroy(needle, *it);
         if (!it->isActive())
-            --haystack.activeChildren;
+            it = haystack.children.erase(it);
+        else
+            it++;
     }
 
     if (needle == haystack.obj) {
         haystack.obj = 0;
         haystack.flags = 0;
     }
-
-    garbageCollectChildren(haystack);
 }
 
 static void huntAndUnregister(const QStringList &pathComponents, int i, QDBusConnection::UnregisterMode mode,
@@ -639,7 +615,6 @@ static void huntAndUnregister(const QStringList &pathComponents, int i, QDBusCon
 
         if (mode == QDBusConnection::UnregisterTree) {
             // clear the sub-tree as well
-            node->activeChildren = 0;
             node->children.clear();  // can't disconnect the objects because we really don't know if they can
                             // be found somewhere else in the path too
         }
@@ -648,14 +623,12 @@ static void huntAndUnregister(const QStringList &pathComponents, int i, QDBusCon
         QDBusConnectionPrivate::ObjectTreeNode::DataList::Iterator end = node->children.end();
         QDBusConnectionPrivate::ObjectTreeNode::DataList::Iterator it =
             std::lower_bound(node->children.begin(), end, pathComponents.at(i));
-        if (it == end || it->name != pathComponents.at(i) || !it->isActive())
+        if (it == end || it->name != pathComponents.at(i))
             return;              // node not found
 
         huntAndUnregister(pathComponents, i + 1, mode, it);
         if (!it->isActive())
-            --node->activeChildren;
-
-        garbageCollectChildren(*node);
+            node->children.erase(it);
     }
 }