dbus: Add comments about the right way to expose methods

Along the way, fix the order in the test service used in unit tests.

BUG=332120
TEST=dbus_unittests pass as before

Review URL: https://codereview.chromium.org/125673003

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

4 files changed, 27 insertions, 12 deletions

diff --git a/dbus/bus.h b/dbus/bus.h
index 12621d3..647b1b7 100644
--- a/dbus/bus.h
+++ b/dbus/bus.h
@@ -421,6 +421,10 @@ class CHROME_DBUS_EXPORT Bus : public base::RefCountedThreadSafe<Bus> {
   // Requests the ownership of the given service name.
   // Returns true on success, or the the service name is already obtained.
   //
+  // Note that it's important to expose methods before requesting a service
+  // name with this method.  See also ExportedObject::ExportMethodAndBlock()
+  // for details.
+  //
   // BLOCKING CALL.
   virtual bool RequestOwnershipAndBlock(const std::string& service_name,
                                         ServiceOwnershipOptions options);
diff --git a/dbus/exported_object.h b/dbus/exported_object.h
index eae2767..5f74dc2 100644
--- a/dbus/exported_object.h
+++ b/dbus/exported_object.h
@@ -66,6 +66,14 @@ class CHROME_DBUS_EXPORT ExportedObject
   // |method_callback| can safely reference objects in the origin thread
   // (i.e. UI thread in most cases).
   //
+  // IMPORTANT NOTE: You should export all methods before requesting a
+  // service name by Bus::RequestOwnership/AndBlock(). If you do it in the
+  // wrong order (i.e. request a service name then export methods), there
+  // will be a short time period where your service is unable to respond to
+  // method calls because these methods aren't yet exposed. This race is a
+  // real problem as clients may start calling methods of your service as
+  // soon as you acquire a service name, by watching the name owner change.
+  //
   // BLOCKING CALL.
   virtual bool ExportMethodAndBlock(const std::string& interface_name,
                                     const std::string& method_name,
diff --git a/dbus/test_service.cc b/dbus/test_service.cc
index 5f59501..21a885d 100644
--- a/dbus/test_service.cc
+++ b/dbus/test_service.cc
@@ -38,7 +38,7 @@ TestService::TestService(const Options& options)
     : base::Thread("TestService"),
       request_ownership_options_(options.request_ownership_options),
       dbus_task_runner_(options.dbus_task_runner),
-      on_all_methods_exported_(false, false),
+      on_name_obtained_(false, false),
       num_exported_methods_(0) {
 }
 
@@ -54,8 +54,8 @@ bool TestService::StartService() {
 
 bool TestService::WaitUntilServiceIsStarted() {
   const base::TimeDelta timeout(TestTimeouts::action_max_timeout());
-  // Wait until all methods are exported.
-  return on_all_methods_exported_.TimedWait(timeout);
+  // Wait until the ownership of the service name is obtained.
+  return on_name_obtained_.TimedWait(timeout);
 }
 
 void TestService::ShutdownAndBlock() {
@@ -138,6 +138,8 @@ void TestService::OnOwnership(base::Callback<void(bool)> callback,
   has_ownership_ = success;
   LOG_IF(ERROR, !success) << "Failed to own: " << service_name;
   callback.Run(success);
+
+  on_name_obtained_.Signal();
 }
 
 void TestService::OnExported(const std::string& interface_name,
@@ -152,8 +154,15 @@ void TestService::OnExported(const std::string& interface_name,
   }
 
   ++num_exported_methods_;
-  if (num_exported_methods_ == kNumMethodsToExport)
-    on_all_methods_exported_.Signal();
+  if (num_exported_methods_ == kNumMethodsToExport) {
+    // As documented in exported_object.h, the service name should be
+    // requested after all methods are exposed.
+    bus_->RequestOwnership("org.chromium.TestService",
+                           request_ownership_options_,
+                           base::Bind(&TestService::OnOwnership,
+                                      base::Unretained(this),
+                                      base::Bind(&EmptyCallback)));
+  }
 }
 
 void TestService::Run(base::MessageLoop* message_loop) {
@@ -163,12 +172,6 @@ void TestService::Run(base::MessageLoop* message_loop) {
   bus_options.dbus_task_runner = dbus_task_runner_;
   bus_ = new Bus(bus_options);
 
-  bus_->RequestOwnership("org.chromium.TestService",
-                         request_ownership_options_,
-                         base::Bind(&TestService::OnOwnership,
-                                    base::Unretained(this),
-                                    base::Bind(&EmptyCallback)));
-
   exported_object_ = bus_->GetExportedObject(
       ObjectPath("/org/chromium/TestObject"));
 
diff --git a/dbus/test_service.h b/dbus/test_service.h
index 7ddaf21..197b8d2 100644
--- a/dbus/test_service.h
+++ b/dbus/test_service.h
@@ -170,7 +170,7 @@ class TestService : public base::Thread {
   Bus::ServiceOwnershipOptions request_ownership_options_;
 
   scoped_refptr<base::SequencedTaskRunner> dbus_task_runner_;
-  base::WaitableEvent on_all_methods_exported_;
+  base::WaitableEvent on_name_obtained_;
   // The number of methods actually exported.
   int num_exported_methods_;