Reuse existing object proxies and exported objects, if these exist.

The Bus object shouldn't return new objects if the bus object already owns
the requested object proxies, or the exported objects.

BUG=90036
TEST=dbus_unittests

Review URL: http://codereview.chromium.org/7702001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@97898 0039d316-1c4b-4281-b951-d872f2087c98

dbus/bus.cc

@@ -212,22 +212,36 @@ ObjectProxy* Bus::GetObjectProxy(const std::string& service_name,
                                  const std::string& object_path) {
   AssertOnOriginThread();
 
+  // Check if we already have the requested object proxy.
+  const std::string key = service_name + object_path;
+  ObjectProxyTable::iterator iter = object_proxy_table_.find(key);
+  if (iter != object_proxy_table_.end()) {
+    return iter->second;
+  }
+
   scoped_refptr<ObjectProxy> object_proxy =
       new ObjectProxy(this, service_name, object_path);
-  object_proxies_.push_back(object_proxy);
+  object_proxy_table_[key] = object_proxy;
 
-  return object_proxy;
+  return object_proxy.get();
 }
 
 ExportedObject* Bus::GetExportedObject(const std::string& service_name,
                                        const std::string& object_path) {
   AssertOnOriginThread();
 
+  // Check if we already have the requested exported object.
+  const std::string key = service_name + object_path;
+  ExportedObjectTable::iterator iter = exported_object_table_.find(key);
+  if (iter != exported_object_table_.end()) {
+    return iter->second;
+  }
+
   scoped_refptr<ExportedObject> exported_object =
       new ExportedObject(this, service_name, object_path);
-  exported_objects_.push_back(exported_object);
+  exported_object_table_[key] = exported_object;
 
-  return exported_object;
+  return exported_object.get();
 }
 
 bool Bus::Connect() {
@@ -260,8 +274,9 @@ void Bus::ShutdownAndBlock() {
   AssertOnDBusThread();
 
   // Unregister the exported objects.
-  for (size_t i = 0; i < exported_objects_.size(); ++i) {
-    exported_objects_[i]->Unregister();
+  for (ExportedObjectTable::iterator iter = exported_object_table_.begin();
+       iter != exported_object_table_.end(); ++iter) {
+    iter->second->Unregister();
   }
 
   // Release all service names.
@@ -278,8 +293,9 @@ void Bus::ShutdownAndBlock() {
   }
 
   // Detach from the remote objects.
-  for (size_t i = 0; i < object_proxies_.size(); ++i) {
-    object_proxies_[i]->Detach();
+  for (ObjectProxyTable::iterator iter = object_proxy_table_.begin();
+       iter != object_proxy_table_.end(); ++iter) {
+    iter->second->Detach();
   }
 
   // Private connection should be closed.

dbus/bus.h

@@ -7,6 +7,7 @@
 #define DBUS_BUS_H_
 #pragma once
 
+#include <map>
 #include <set>
 #include <string>
 #include <dbus/dbus.h>
@@ -55,8 +56,8 @@ class ObjectProxy;
 // Note that it's hard to tell if a libdbus function is actually blocking
 // or not (ex. dbus_bus_request_name() internally calls
 // dbus_connection_send_with_reply_and_block(), which is a blocking
-// call). To err on the side, we consider all libdbus functions that deal
-// with the connection to dbus-damoen to be blocking.
+// call). To err on the safe side, we consider all libdbus functions that
+// deal with the connection to dbus-damoen to be blocking.
 //
 // EXAMPLE USAGE:
 //
@@ -164,8 +165,15 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   explicit Bus(const Options& options);
 
   // Gets the object proxy for the given service name and the object path.
-  // The caller must not delete the returned object. The bus will own the
-  // object. Never returns NULL.
+  // The caller must not delete the returned object.
+  //
+  // Returns an existing object proxy if the bus object already owns the
+  // object proxy for the given service name and the object path.
+  // Never returns NULL.
+  //
+  // The bus will own all object proxies created by the bus, to ensure
+  // that the object proxies are detached from remote objects at the
+  // shutdown time of the bus.
   //
   // The object proxy is used to call methods of remote objects, and
   // receive signals from them.
@@ -178,8 +186,15 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
                                       const std::string& object_path);
 
   // Gets the exported object for the given service name and the object
-  // path. The caller must not delete the returned object. The bus will
-  // own the object. Never returns NULL.
+  // path. The caller must not delete the returned object.
+  //
+  // Returns an existing exported object if the bus object already owns
+  // the exported object for the given service name and the object path.
+  // Never returns NULL.
+  //
+  // The bus will own all exported objects created by the bus, to ensure
+  // that the exported objects are unregistered at the shutdown time of
+  // the bus.
   //
   // The exported object is used to export methods of local objects, and
   // send signal from them.
@@ -416,8 +431,19 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   std::set<std::string> registered_object_paths_;
   std::set<DBusHandleMessageFunction> filter_functions_added_;
 
-  std::vector<scoped_refptr<dbus::ObjectProxy> > object_proxies_;
-  std::vector<scoped_refptr<dbus::ExportedObject> > exported_objects_;
+  // ObjectProxyTable is used to hold the object proxies created by the
+  // bus object. Key is a concatenated string of service name + object path,
+  // like "org.chromium.TestService/org/chromium/TestObject".
+  typedef std::map<std::string,
+                   scoped_refptr<dbus::ObjectProxy> > ObjectProxyTable;
+  ObjectProxyTable object_proxy_table_;
+
+  // ExportedObjectTable is used to hold the exported objects created by
+  // the bus object. Key is a concatenated string of service name +
+  // object path, like "org.chromium.TestService/org/chromium/TestObject".
+  typedef std::map<std::string,
+                   scoped_refptr<dbus::ExportedObject> > ExportedObjectTable;
+  ExportedObjectTable exported_object_table_;
 
   bool async_operations_are_set_up_;
 

dbus/bus_unittest.cc

@@ -0,0 +1,59 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "dbus/bus.h"
+
+#include "base/memory/ref_counted.h"
+#include "dbus/exported_object.h"
+#include "dbus/object_proxy.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+TEST(BusTest, GetObjectProxy) {
+  dbus::Bus::Options options;
+  scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
+
+  dbus::ObjectProxy* object_proxy1 =
+      bus->GetObjectProxy("org.chromium.TestService",
+                          "/org/chromium/TestObject");
+  ASSERT_TRUE(object_proxy1);
+
+  // This should return the same object.
+  dbus::ObjectProxy* object_proxy2 =
+      bus->GetObjectProxy("org.chromium.TestService",
+                          "/org/chromium/TestObject");
+  ASSERT_TRUE(object_proxy2);
+  EXPECT_EQ(object_proxy1, object_proxy2);
+
+  // This should not.
+  dbus::ObjectProxy* object_proxy3 =
+      bus->GetObjectProxy("org.chromium.TestService",
+                          "/org/chromium/DifferentTestObject");
+  ASSERT_TRUE(object_proxy3);
+  EXPECT_NE(object_proxy1, object_proxy3);
+}
+
+TEST(BusTest, GetExportedObject) {
+  dbus::Bus::Options options;
+  scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
+
+  dbus::ExportedObject* object_proxy1 =
+      bus->GetExportedObject("org.chromium.TestService",
+                             "/org/chromium/TestObject");
+  ASSERT_TRUE(object_proxy1);
+
+  // This should return the same object.
+  dbus::ExportedObject* object_proxy2 =
+      bus->GetExportedObject("org.chromium.TestService",
+                             "/org/chromium/TestObject");
+  ASSERT_TRUE(object_proxy2);
+  EXPECT_EQ(object_proxy1, object_proxy2);
+
+  // This should not.
+  dbus::ExportedObject* object_proxy3 =
+      bus->GetExportedObject("org.chromium.TestService",
+                             "/org/chromium/DifferentTestObject");
+  ASSERT_TRUE(object_proxy3);
+  EXPECT_NE(object_proxy1, object_proxy3);
+}

dbus/dbus.gyp

@@ -37,6 +37,7 @@
       ],
       'sources': [
         '../base/test/run_all_unittests.cc',
+        'bus_unittest.cc',
         'message_unittest.cc',
         'end_to_end_async_unittest.cc',
         'end_to_end_sync_unittest.cc',