dbus: remove service name from ExportedObject

Well-known names in D-Bus are merely aliases to unique connection ids
maintained by the bus, they have no purpose in qualifying object paths
or interfaces and it's perfectly legimiate for a client to make
requests to the unique connection id (e.g. in response to a signal,
which does not reference the well-known name of the origin connection).

Remove the service_name member from dbus::ExportedObject, from its
constructor and from dbus::Bus::GetExportedObject and require code to
call dbus::Bus::RequestOwnership if a well-known name is desired. This
requires making that function callable from the origin thread with
a callback for the return value.

BUG=chromium-os:27101
TEST=dbus_unittests

Change-Id: Ib91de8b68ad9c3b432e224a2c715f0c2ca1af463


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

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

chrome/browser/chromeos/dbus/cros_dbus_service.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/dbus/cros_dbus_service.h"
 
+#include "base/bind.h"
 #include "base/stl_util.h"
 #include "base/threading/platform_thread.h"
 #include "chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h"
@@ -39,8 +40,11 @@ class CrosDBusServiceImpl : public CrosDBusService {
     if (service_started_)
       return;
 
+    bus_->RequestOwnership(kLibCrosServiceName,
+                           base::Bind(&CrosDBusServiceImpl::OnOwnership,
+                                      base::Unretained(this)));
+
     exported_object_ = bus_->GetExportedObject(
-        kLibCrosServiceName,
         dbus::ObjectPath(kLibCrosServicePath));
 
     for (size_t i = 0; i < service_providers_.size(); ++i)
@@ -63,6 +67,12 @@ class CrosDBusServiceImpl : public CrosDBusService {
     return base::PlatformThread::CurrentId() == origin_thread_id_;
   }
 
+  // Called when an ownership request is completed.
+  void OnOwnership(const std::string& service_name,
+                   bool success) {
+    LOG_IF(ERROR, !success) << "Failed to own: " << service_name;
+  }
+
   bool service_started_;
   base::PlatformThreadId origin_thread_id_;
   dbus::Bus* bus_;

chrome/browser/chromeos/dbus/cros_dbus_service_unittest.cc

@@ -48,13 +48,12 @@ class CrosDBusServiceTest : public testing::Test {
     // org.chromium.CrosDBusService.
     mock_exported_object_ =
         new dbus::MockExportedObject(mock_bus_.get(),
-                                     kLibCrosServiceName,
                                      dbus::ObjectPath(kLibCrosServicePath));
 
     // |mock_bus_|'s GetExportedObject() will return mock_exported_object_|
     // for the given service name and the object path.
     EXPECT_CALL(*mock_bus_, GetExportedObject(
-        kLibCrosServiceName, dbus::ObjectPath(kLibCrosServicePath)))
+        dbus::ObjectPath(kLibCrosServicePath)))
         .WillOnce(Return(mock_exported_object_.get()));
 
     // Create a mock proxy resolution service.

chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc

@@ -80,7 +80,6 @@ class ProxyResolutionServiceProviderTest : public testing::Test {
     // org.chromium.CrosDBusService.
     mock_exported_object_ =
         new dbus::MockExportedObject(mock_bus_.get(),
-                                     kLibCrosServiceName,
                                      dbus::ObjectPath(kLibCrosServicePath));
 
     // |mock_exported_object_|'s ExportMethod() will use

dbus/bus.cc

@@ -233,20 +233,18 @@ ObjectProxy* Bus::GetObjectProxyWithOptions(const std::string& service_name,
   return object_proxy.get();
 }
 
-ExportedObject* Bus::GetExportedObject(const std::string& service_name,
-                                       const ObjectPath& object_path) {
+ExportedObject* Bus::GetExportedObject(const ObjectPath& object_path) {
   AssertOnOriginThread();
 
   // Check if we already have the requested exported object.
-  const std::string key = service_name + object_path.value();
-  ExportedObjectTable::iterator iter = exported_object_table_.find(key);
+  ExportedObjectTable::iterator iter = exported_object_table_.find(object_path);
   if (iter != exported_object_table_.end()) {
     return iter->second;
   }
 
   scoped_refptr<ExportedObject> exported_object =
-      new ExportedObject(this, service_name, object_path);
-  exported_object_table_[key] = exported_object;
+      new ExportedObject(this, object_path);
+  exported_object_table_[object_path] = exported_object;
 
   return exported_object.get();
 }
@@ -339,7 +337,40 @@ void Bus::ShutdownOnDBusThreadAndBlock() {
   LOG_IF(ERROR, !signaled) << "Failed to shutdown the bus";
 }
 
-bool Bus::RequestOwnership(const std::string& service_name) {
+void Bus::RequestOwnership(const std::string& service_name,
+                           OnOwnershipCallback on_ownership_callback) {
+  AssertOnOriginThread();
+
+  PostTaskToDBusThread(FROM_HERE, base::Bind(
+      &Bus::RequestOwnershipInternal,
+      this, service_name, on_ownership_callback));
+}
+
+void Bus::RequestOwnershipInternal(const std::string& service_name,
+                                   OnOwnershipCallback on_ownership_callback) {
+  AssertOnDBusThread();
+
+  bool success = Connect();
+  if (success)
+    success = RequestOwnershipAndBlock(service_name);
+
+  PostTaskToOriginThread(FROM_HERE,
+                         base::Bind(&Bus::OnOwnership,
+                                    this,
+                                    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.
   AssertOnDBusThread();

dbus/bus.h

@@ -176,6 +176,12 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   // Connect() is called.
   explicit Bus(const Options& options);
 
+  // Called when an ownership request is complete.
+  // Parameters:
+  // - the requested service name.
+  // - whether ownership has been obtained or not.
+  typedef base::Callback<void (const std::string&, bool)> OnOwnershipCallback;
+
   // Gets the object proxy for the given service name and the object path.
   // The caller must not delete the returned object.
   //
@@ -204,12 +210,11 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
       const ObjectPath& object_path,
       int options);
 
-  // Gets the exported object for the given service name and the object
-  // path. The caller must not delete the returned object.
+  // Gets the exported object for the given object 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 exported object for the given 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
@@ -219,8 +224,7 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   // send signal from them.
   //
   // Must be called in the origin thread.
-  virtual ExportedObject* GetExportedObject(const std::string& service_name,
-                                            const ObjectPath& object_path);
+  virtual ExportedObject* GetExportedObject(const ObjectPath& object_path);
 
   // Shuts down the bus and blocks until it's done. More specifically, this
   // function does the following:
@@ -255,11 +259,21 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   // BLOCKING CALL.
   virtual bool Connect();
 
+  // Requests the ownership of the service name given by |service_name|.
+  // See also RequestOwnershipAndBlock().
+  //
+  // |on_ownership_callback| is called when the service name is obtained
+  // or failed to be obtained, in the origin thread.
+  //
+  // Must be called in the origin thread.
+  virtual void RequestOwnership(const std::string& service_name,
+                                OnOwnershipCallback on_ownership_callback);
+
   // Requests the ownership of the given service name.
   // Returns true on success, or the the service name is already obtained.
   //
   // BLOCKING CALL.
-  virtual bool RequestOwnership(const std::string& service_name);
+  virtual bool RequestOwnershipAndBlock(const std::string& service_name);
 
   // Releases the ownership of the given service name.
   // Returns true on success.
@@ -409,6 +423,15 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   // Helper function used for ShutdownOnDBusThreadAndBlock().
   void ShutdownOnDBusThreadAndBlockInternal();
 
+  // Helper function used for RequestOwnership().
+  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.
@@ -478,7 +501,7 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   // 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,
+  typedef std::map<const dbus::ObjectPath,
                    scoped_refptr<dbus::ExportedObject> > ExportedObjectTable;
   ExportedObjectTable exported_object_table_;
 

dbus/bus_unittest.cc

@@ -89,21 +89,18 @@ TEST(BusTest, GetExportedObject) {
   scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
 
   dbus::ExportedObject* object_proxy1 =
-      bus->GetExportedObject("org.chromium.TestService",
-                             dbus::ObjectPath("/org/chromium/TestObject"));
+      bus->GetExportedObject(dbus::ObjectPath("/org/chromium/TestObject"));
   ASSERT_TRUE(object_proxy1);
 
   // This should return the same object.
   dbus::ExportedObject* object_proxy2 =
-      bus->GetExportedObject("org.chromium.TestService",
-                             dbus::ObjectPath("/org/chromium/TestObject"));
+      bus->GetExportedObject(dbus::ObjectPath("/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",
           dbus::ObjectPath("/org/chromium/DifferentTestObject"));
   ASSERT_TRUE(object_proxy3);
   EXPECT_NE(object_proxy1, object_proxy3);

dbus/exported_object.cc

@@ -35,10 +35,8 @@ std::string GetAbsoluteMethodName(
 }  // namespace
 
 ExportedObject::ExportedObject(Bus* bus,
-                               const std::string& service_name,
                                const ObjectPath& object_path)
     : bus_(bus),
-      service_name_(service_name),
       object_path_(object_path),
       object_is_registered_(false) {
 }
@@ -65,8 +63,6 @@ bool ExportedObject::ExportMethodAndBlock(
     return false;
   if (!bus_->SetUpAsyncOperations())
     return false;
-  if (!bus_->RequestOwnership(service_name_))
-    return false;
   if (!Register())
     return false;
 

dbus/exported_object.h

@@ -35,9 +35,7 @@ class ExportedObject : public base::RefCountedThreadSafe<ExportedObject> {
  public:
   // Client code should use Bus::GetExportedObject() instead of this
   // constructor.
-  ExportedObject(Bus* bus,
-                 const std::string& service_name,
-                 const ObjectPath& object_path);
+  ExportedObject(Bus* bus, const ObjectPath& object_path);
 
   // Called to send a response from an exported method. Response* is the
   // response message. Callers should pass a NULL Response* in the event
@@ -157,7 +155,6 @@ class ExportedObject : public base::RefCountedThreadSafe<ExportedObject> {
                                   void* user_data);
 
   scoped_refptr<Bus> bus_;
-  std::string service_name_;
   ObjectPath object_path_;
   bool object_is_registered_;
 

dbus/mock_bus.h

@@ -26,13 +26,15 @@ class MockBus : public Bus {
                ObjectProxy*(const std::string& service_name,
                             const ObjectPath& object_path,
                             int options));
-  MOCK_METHOD2(GetExportedObject, ExportedObject*(
-      const std::string& service_name,
+  MOCK_METHOD1(GetExportedObject, ExportedObject*(
       const ObjectPath& object_path));
   MOCK_METHOD0(ShutdownAndBlock, void());
   MOCK_METHOD0(ShutdownOnDBusThreadAndBlock, void());
   MOCK_METHOD0(Connect, bool());
-  MOCK_METHOD1(RequestOwnership, bool(const std::string& service_name));
+  MOCK_METHOD2(RequestOwnership, void(
+      const std::string& service_name,
+      OnOwnershipCallback on_ownership_callback));
+  MOCK_METHOD1(RequestOwnershipAndBlock, bool(const std::string& service_name));
   MOCK_METHOD1(ReleaseOwnership, bool(const std::string& service_name));
   MOCK_METHOD0(SetUpAsyncOperations, bool());
   MOCK_METHOD3(SendWithReplyAndBlock, DBusMessage*(DBusMessage* request,

dbus/mock_exported_object.cc

@@ -7,9 +7,8 @@
 namespace dbus {
 
 MockExportedObject::MockExportedObject(Bus* bus,
-                                       const std::string& service_name,
                                        const ObjectPath& object_path)
-    : ExportedObject(bus, service_name, object_path) {
+    : ExportedObject(bus, object_path) {
 }
 
 MockExportedObject::~MockExportedObject() {

dbus/mock_exported_object.h

@@ -18,7 +18,6 @@ namespace dbus {
 class MockExportedObject : public ExportedObject {
  public:
   MockExportedObject(Bus* bus,
-                     const std::string& service_name,
                      const ObjectPath& object_path);
   virtual ~MockExportedObject();
 

dbus/test_service.cc

@@ -95,13 +95,21 @@ void TestService::SendTestSignalFromRootInternal(const std::string& message) {
   dbus::MessageWriter writer(&signal);
   writer.AppendString(message);
 
+  bus_->RequestOwnership("org.chromium.TestService",
+                         base::Bind(&TestService::OnOwnership,
+                                    base::Unretained(this)));
+
   // Use "/" just like dbus-send does.
   ExportedObject* root_object =
-      bus_->GetExportedObject("org.chromium.TestService",
-                              dbus::ObjectPath("/"));
+      bus_->GetExportedObject(dbus::ObjectPath("/"));
   root_object->SendSignal(&signal);
 }
 
+void TestService::OnOwnership(const std::string& service_name,
+                              bool success) {
+  LOG_IF(ERROR, !success) << "Failed to own: " << service_name;
+}
+
 void TestService::OnExported(const std::string& interface_name,
                              const std::string& method_name,
                              bool success) {
@@ -125,8 +133,11 @@ void TestService::Run(MessageLoop* message_loop) {
   bus_options.dbus_thread_message_loop_proxy = dbus_thread_message_loop_proxy_;
   bus_ = new Bus(bus_options);
 
+  bus_->RequestOwnership("org.chromium.TestService",
+                         base::Bind(&TestService::OnOwnership,
+                                    base::Unretained(this)));
+
   exported_object_ = bus_->GetExportedObject(
-      "org.chromium.TestService",
       dbus::ObjectPath("/org/chromium/TestObject"));
 
   int num_methods = 0;

dbus/test_service.h

@@ -76,6 +76,10 @@ class TestService : public base::Thread {
   // Helper function for ShutdownAndBlock().
   void ShutdownAndBlockInternal();
 
+  // Called when an ownership request is completed.
+  void OnOwnership(const std::string& service_name,
+                   bool success);
+
   // Called when a method is exported.
   void OnExported(const std::string& interface_name,
                   const std::string& method_name,