D-Bus: ObjectProxy remove function for Bus object.

This implements a remove function for an ObjectProxy owned by a bus
object. Although the Bus object removes all the objects at shutdown,
this functions permits to reduce the memory usage for objects no
longer needed and reduce the number of dbus matching rules used in
the bus connection.

BUG=chromium:170182
TEST=BusTest.RemoveObjectProxy (run out/Debug/dbus_unittests)


Review URL: https://chromiumcodereview.appspot.com/12022004

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

3 files changed, 142 insertions, 16 deletions

diff --git a/dbus/bus.cc b/dbus/bus.cc
index 30b6cc3..c7584fd 100644
--- a/dbus/bus.cc
+++ b/dbus/bus.cc
@@ -235,6 +235,46 @@ ObjectProxy* Bus::GetObjectProxyWithOptions(const std::string& service_name,
   return object_proxy.get();
 }
 
+bool Bus::RemoveObjectProxy(const std::string& service_name,
+                            const ObjectPath& object_path,
+                            const base::Closure& callback) {
+  return RemoveObjectProxyWithOptions(service_name, object_path,
+                                      ObjectProxy::DEFAULT_OPTIONS,
+                                      callback);
+}
+
+bool Bus::RemoveObjectProxyWithOptions(const std::string& service_name,
+                                       const dbus::ObjectPath& object_path,
+                                       int options,
+                                       const base::Closure& callback) {
+  AssertOnOriginThread();
+
+  // Check if we have the requested object proxy.
+  const ObjectProxyTable::key_type key(service_name + object_path.value(),
+                                       options);
+  ObjectProxyTable::iterator iter = object_proxy_table_.find(key);
+  if (iter != object_proxy_table_.end()) {
+    // Object is present. Remove it now and Detach in the DBus thread.
+    PostTaskToDBusThread(FROM_HERE, base::Bind(
+        &Bus::RemoveObjectProxyInternal,
+        this, iter->second, callback));
+
+    object_proxy_table_.erase(iter);
+    return true;
+  }
+  return false;
+}
+
+void Bus::RemoveObjectProxyInternal(
+    scoped_refptr<dbus::ObjectProxy> object_proxy,
+    const base::Closure& callback) {
+  AssertOnDBusThread();
+
+  object_proxy.get()->Detach();
+
+  PostTaskToOriginThread(FROM_HERE, callback);
+}
+
 ExportedObject* Bus::GetExportedObject(const ObjectPath& object_path) {
   AssertOnOriginThread();
 
@@ -410,21 +450,11 @@ void Bus::RequestOwnershipInternal(const std::string& service_name,
     success = RequestOwnershipAndBlock(service_name);
 
   PostTaskToOriginThread(FROM_HERE,
-                         base::Bind(&Bus::OnOwnership,
-                                    this,
-                                    on_ownership_callback,
+                         base::Bind(on_ownership_callback,
                                     service_name,
                                     success));
 }
 
-void Bus::OnOwnership(OnOwnershipCallback on_ownership_callback,
-                      const std::string& service_name,
-                      bool success) {
-  AssertOnOriginThread();
-
-  on_ownership_callback.Run(service_name, success);
-}
-
 bool Bus::RequestOwnershipAndBlock(const std::string& service_name) {
   DCHECK(connection_);
   // dbus_bus_request_name() is a blocking call.
diff --git a/dbus/bus.h b/dbus/bus.h
index 75de9a3..0545258 100644
--- a/dbus/bus.h
+++ b/dbus/bus.h
@@ -201,6 +201,8 @@ class CHROME_DBUS_EXPORT Bus : public base::RefCountedThreadSafe<Bus> {
   // - the requested service name.
   // - whether ownership has been obtained or not.
   typedef base::Callback<void (const std::string&, bool)> OnOwnershipCallback;
+  // TODO(satorux): Remove the service name parameter as the caller of
+  // RequestOwnership() knows the service name.
 
   // Gets the object proxy for the given service name and the object path.
   // The caller must not delete the returned object.
@@ -230,6 +232,42 @@ class CHROME_DBUS_EXPORT Bus : public base::RefCountedThreadSafe<Bus> {
       const ObjectPath& object_path,
       int options);
 
+  // Removes the previously created object proxy for the given service
+  // name and the object path and releases its memory.
+  //
+  // If and object proxy for the given service name and object was
+  // created with GetObjectProxy, this function removes it from the
+  // bus object and detaches the ObjectProxy, invalidating any pointer
+  // previously acquired for it with GetObjectProxy. A subsequent call
+  // to GetObjectProxy will return a new object.
+  //
+  // All the object proxies are detached from remote objects at the
+  // shutdown time of the bus, but they can be detached early to reduce
+  // memory footprint and used match rules for the bus connection.
+  //
+  // |service_name| looks like "org.freedesktop.NetworkManager", and
+  // |object_path| looks like "/org/freedesktop/NetworkManager/Devices/0".
+  // |callback| is called when the object proxy is successfully removed and
+  // detached.
+  //
+  // The function returns true when there is an object proxy matching the
+  // |service_name| and |object_path| to remove, and calls |callback| when it
+  // is removed. Otherwise, it returns false and the |callback| function is
+  // never called. The |callback| argument must not be null.
+  //
+  // Must be called in the origin thread.
+  virtual bool RemoveObjectProxy(const std::string& service_name,
+                                 const ObjectPath& object_path,
+                                 const base::Closure& callback);
+
+  // Same as above, but also takes a bitfield of ObjectProxy::Options.
+  // See object_proxy.h for available options.
+  virtual bool RemoveObjectProxyWithOptions(
+      const std::string& service_name,
+      const ObjectPath& object_path,
+      int options,
+      const base::Closure& callback);
+
   // Gets the exported object for the given object path.
   // The caller must not delete the returned object.
   //
@@ -449,6 +487,10 @@ class CHROME_DBUS_EXPORT Bus : public base::RefCountedThreadSafe<Bus> {
  private:
   friend class base::RefCountedThreadSafe<Bus>;
 
+  // Helper function used for RemoveObjectProxy().
+  void RemoveObjectProxyInternal(scoped_refptr<dbus::ObjectProxy> object_proxy,
+                                 const base::Closure& callback);
+
   // Helper function used for UnregisterExportedObject().
   void UnregisterExportedObjectInternal(
       scoped_refptr<dbus::ExportedObject> exported_object);
@@ -460,11 +502,6 @@ class CHROME_DBUS_EXPORT Bus : public base::RefCountedThreadSafe<Bus> {
   void RequestOwnershipInternal(const std::string& service_name,
                                 OnOwnershipCallback on_ownership_callback);
 
-  // Called when the ownership request is completed.
-  void OnOwnership(OnOwnershipCallback on_ownership_callback,
-                   const std::string& service_name,
-                   bool success);
-
   // Processes the all incoming data to the connection, if any.
   //
   // BLOCKING CALL.
diff --git a/dbus/bus_unittest.cc b/dbus/bus_unittest.cc
index bc155e7..0f57db1 100644
--- a/dbus/bus_unittest.cc
+++ b/dbus/bus_unittest.cc
@@ -84,6 +84,65 @@ TEST(BusTest, GetObjectProxyIgnoreUnknownService) {
   bus->ShutdownAndBlock();
 }
 
+TEST(BusTest, RemoveObjectProxy) {
+  // Setup the current thread's MessageLoop.
+  MessageLoop message_loop;
+
+  // Start the D-Bus thread.
+  base::Thread::Options thread_options;
+  thread_options.message_loop_type = MessageLoop::TYPE_IO;
+  base::Thread dbus_thread("D-Bus thread");
+  dbus_thread.StartWithOptions(thread_options);
+
+  // Create the bus.
+  dbus::Bus::Options options;
+  options.dbus_thread_message_loop_proxy = dbus_thread.message_loop_proxy();
+  scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
+  ASSERT_FALSE(bus->shutdown_completed());
+
+  // Try to remove a non existant object proxy should return false.
+  ASSERT_FALSE(
+      bus->RemoveObjectProxy("org.chromium.TestService",
+                             dbus::ObjectPath("/org/chromium/TestObject"),
+                             base::Bind(&base::DoNothing)));
+
+  dbus::ObjectProxy* object_proxy1 =
+      bus->GetObjectProxy("org.chromium.TestService",
+                          dbus::ObjectPath("/org/chromium/TestObject"));
+  ASSERT_TRUE(object_proxy1);
+
+  // Increment the reference count to the object proxy to avoid destroying it
+  // while removing the object.
+  object_proxy1->AddRef();
+
+  // Remove the object from the bus. This will invalidate any other usage of
+  // object_proxy1 other than destroy it. We keep this object for a comparison
+  // at a later time.
+  ASSERT_TRUE(
+      bus->RemoveObjectProxy("org.chromium.TestService",
+                             dbus::ObjectPath("/org/chromium/TestObject"),
+                             base::Bind(&base::DoNothing)));
+
+  // This should return a different object because the first object was removed
+  // from the bus, but not deleted from memory.
+  dbus::ObjectProxy* object_proxy2 =
+      bus->GetObjectProxy("org.chromium.TestService",
+                          dbus::ObjectPath("/org/chromium/TestObject"));
+  ASSERT_TRUE(object_proxy2);
+
+  // Compare the new object with the first object. The first object still exists
+  // thanks to the increased reference.
+  EXPECT_NE(object_proxy1, object_proxy2);
+
+  // Release object_proxy1.
+  object_proxy1->Release();
+
+  // Shut down synchronously.
+  bus->ShutdownOnDBusThreadAndBlock();
+  EXPECT_TRUE(bus->shutdown_completed());
+  dbus_thread.Stop();
+}
+
 TEST(BusTest, GetExportedObject) {
   dbus::Bus::Options options;
   scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);