Add Bus::ShutdownOnDBusThreadAndBlock() and remove bus::Shutdown()

    
This function is intended to use at the the very end of the browser shutdown,
where it it makes more sense to shut down the bus synchronously, than trying
to make it asynchronous.

Remove Bus::Shutdown() as we are unlikely to need it for the production code.

BUG=chromium:90036
TEST=dbus_unittests


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

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

8 files changed, 92 insertions, 70 deletions

diff --git a/dbus/bus.cc b/dbus/bus.cc
index 1b766acd..3651332 100644
--- a/dbus/bus.cc
+++ b/dbus/bus.cc
@@ -14,6 +14,7 @@
 #include "base/stl_util.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/time.h"
 #include "dbus/exported_object.h"
 #include "dbus/object_proxy.h"
 #include "dbus/scoped_dbus_error.h"
@@ -178,11 +179,13 @@ Bus::Bus(const Options& options)
     : bus_type_(options.bus_type),
       connection_type_(options.connection_type),
       dbus_thread_(options.dbus_thread),
+      on_shutdown_(false /* manual_reset */, false /* initially_signaled */),
       connection_(NULL),
       origin_loop_(MessageLoop::current()),
       origin_thread_id_(base::PlatformThread::CurrentId()),
       dbus_thread_id_(base::kInvalidThreadId),
       async_operations_set_up_(false),
+      shutdown_completed_(false),
       num_pending_watches_(0),
       num_pending_timeouts_(0) {
   if (dbus_thread_) {
@@ -299,21 +302,31 @@ void Bus::ShutdownAndBlock() {
   }
 
   // Private connection should be closed.
-  if (connection_ && connection_type_ == PRIVATE) {
-    dbus_connection_close(connection_);
+  if (connection_) {
+    if (connection_type_ == PRIVATE)
+      dbus_connection_close(connection_);
+    // dbus_connection_close() won't unref.
+    dbus_connection_unref(connection_);
   }
-  // dbus_connection_close() won't unref.
-  dbus_connection_unref(connection_);
 
   connection_ = NULL;
+  shutdown_completed_ = true;
 }
 
-void Bus::Shutdown(OnShutdownCallback callback) {
+void Bus::ShutdownOnDBusThreadAndBlock() {
   AssertOnOriginThread();
+  DCHECK(dbus_thread_);
 
-  PostTaskToDBusThread(FROM_HERE, base::Bind(&Bus::ShutdownInternal,
-                                             this,
-                                             callback));
+  PostTaskToDBusThread(FROM_HERE, base::Bind(
+      &Bus::ShutdownOnDBusThreadAndBlockInternal,
+      this));
+
+  // Wait until the shutdown is complete on the D-Bus thread.
+  // The shutdown should not hang, but set timeout just in case.
+  const int kTimeoutSecs = 3;
+  const base::TimeDelta timeout(base::TimeDelta::FromSeconds(kTimeoutSecs));
+  const bool signaled = on_shutdown_.TimedWait(timeout);
+  LOG_IF(ERROR, !signaled) << "Failed to shutdown the bus";
 }
 
 bool Bus::RequestOwnership(const std::string& service_name) {
@@ -535,11 +548,11 @@ void Bus::UnregisterObjectPath(const std::string& object_path) {
   registered_object_paths_.erase(object_path);
 }
 
-void Bus::ShutdownInternal(OnShutdownCallback callback) {
+void Bus::ShutdownOnDBusThreadAndBlockInternal() {
   AssertOnDBusThread();
 
   ShutdownAndBlock();
-  PostTaskToOriginThread(FROM_HERE, callback);
+  on_shutdown_.Signal();
 }
 
 void Bus::ProcessAllIncomingDataIfAny() {
diff --git a/dbus/bus.h b/dbus/bus.h
index 792eb83..5be6ddc 100644
--- a/dbus/bus.h
+++ b/dbus/bus.h
@@ -14,6 +14,7 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
 #include "base/tracked_objects.h"
 
@@ -61,9 +62,9 @@ class ObjectProxy;
 //
 // SHUTDOWN
 //
-// The Bus object must be shut down manually by Shutdown() or
-// ShutdownAndBlock(). We require the manual shutdown as we should not
-// issue blocking calls in the destructor.
+// The Bus object must be shut down manually by ShutdownAndBlock() and
+// friends. We require the manual shutdown to make the operation explicit
+// rather than doing it silently in the destructor.
 //
 // EXAMPLE USAGE:
 //
@@ -119,8 +120,8 @@ class ObjectProxy;
 // WHY IS THIS A REF COUNTED OBJECT?
 //
 // Bus is a ref counted object, to ensure that |this| of the object is
-// alive when callbacks referencing |this| are called. However, after
-// Shutdown() is called, |connection_| can be NULL. Hence, callbacks should
+// alive when callbacks referencing |this| are called. However, after the
+// bus is shut down, |connection_| can be NULL. Hence, callbacks should
 // not rely on that |connection_| is alive.
 class Bus : public base::RefCountedThreadSafe<Bus> {
  public:
@@ -163,9 +164,6 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
     base::Thread* dbus_thread;  // NULL by default.
   };
 
-  // Called when shutdown is done. Used for Shutdown().
-  typedef base::Callback<void ()> OnShutdownCallback;
-
   // Creates a Bus object. The actual connection will be established when
   // Connect() is called.
   explicit Bus(const Options& options);
@@ -219,11 +217,17 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   // BLOCKING CALL.
   virtual void ShutdownAndBlock();
 
-  // Shuts down the bus in the D-Bus thread. |callback| will be called in
-  // the origin thread.
+  // Similar to ShutdownAndBlock(), but this function is used to
+  // synchronously shut down the bus that uses the D-Bus thread. This
+  // function is intended to be used at the very end of the browser
+  // shutdown, where it makes more sense to shut down the bus
+  // synchronously, than trying to make it asynchronous.
   //
-  // Must be called in the origin thread.
-  virtual void Shutdown(OnShutdownCallback callback);
+  // BLOCKING CALL, but must be called in the origin thread.
+  virtual void ShutdownOnDBusThreadAndBlock();
+
+  // Returns true if the shutdown has been completed.
+  bool shutdown_completed() { return shutdown_completed_; }
 
   //
   // The public functions below are not intended to be used in client
@@ -381,8 +385,8 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
  private:
   friend class base::RefCountedThreadSafe<Bus>;
 
-  // Helper function used for Shutdown().
-  void ShutdownInternal(OnShutdownCallback callback);
+  // Helper function used for ShutdownOnDBusThreadAndBlock().
+  void ShutdownOnDBusThreadAndBlockInternal();
 
   // Processes the all incoming data to the connection, if any.
   //
@@ -427,6 +431,7 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   const BusType bus_type_;
   const ConnectionType connection_type_;
   base::Thread* dbus_thread_;
+  base::WaitableEvent on_shutdown_;
   DBusConnection* connection_;
 
   MessageLoop* origin_loop_;
@@ -455,6 +460,7 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   ExportedObjectTable exported_object_table_;
 
   bool async_operations_set_up_;
+  bool shutdown_completed_;
 
   // Counters to make sure that OnAddWatch()/OnRemoveWatch() and
   // OnAddTimeout()/OnRemoveTimeou() are balanced.
diff --git a/dbus/bus_unittest.cc b/dbus/bus_unittest.cc
index 5cd198d..7d477e2 100644
--- a/dbus/bus_unittest.cc
+++ b/dbus/bus_unittest.cc
@@ -4,7 +4,10 @@
 
 #include "dbus/bus.h"
 
+#include "base/bind.h"
+#include "base/message_loop.h"
 #include "base/memory/ref_counted.h"
+#include "base/threading/thread.h"
 #include "dbus/exported_object.h"
 #include "dbus/object_proxy.h"
 
@@ -57,3 +60,32 @@ TEST(BusTest, GetExportedObject) {
   ASSERT_TRUE(object_proxy3);
   EXPECT_NE(object_proxy1, object_proxy3);
 }
+
+TEST(BusTest, ShutdownAndBlock) {
+  dbus::Bus::Options options;
+  scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
+  ASSERT_FALSE(bus->shutdown_completed());
+
+  // Shut down synchronously.
+  bus->ShutdownAndBlock();
+  EXPECT_TRUE(bus->shutdown_completed());
+}
+
+TEST(BusTest, ShutdownAndBlockWithDBusThread) {
+  // 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 = &dbus_thread;
+  scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
+  ASSERT_FALSE(bus->shutdown_completed());
+
+  // Shut down synchronously.
+  bus->ShutdownOnDBusThreadAndBlock();
+  EXPECT_TRUE(bus->shutdown_completed());
+  dbus_thread.Stop();
+}
diff --git a/dbus/end_to_end_async_unittest.cc b/dbus/end_to_end_async_unittest.cc
index 41b06b6..cea37a8 100644
--- a/dbus/end_to_end_async_unittest.cc
+++ b/dbus/end_to_end_async_unittest.cc
@@ -66,15 +66,10 @@ class EndToEndAsyncTest : public testing::Test {
   }
 
   virtual void TearDown() {
-    bus_->Shutdown(base::Bind(&EndToEndAsyncTest::OnShutdown,
-                              base::Unretained(this)));
-    // Wait until the bus is shutdown. OnShutdown() will be called in
-    // message_loop_.
-    message_loop_.Run();
+    bus_->ShutdownOnDBusThreadAndBlock();
 
     // Shut down the service.
-    test_service_->Shutdown();
-    ASSERT_TRUE(test_service_->WaitUntilServiceIsShutdown());
+    test_service_->ShutdownAndBlock();
 
     // Reset to the default.
     base::ThreadRestrictions::SetIOAllowed(true);
@@ -117,11 +112,6 @@ class EndToEndAsyncTest : public testing::Test {
     message_loop_.Quit();
   };
 
-  // Called when the shutdown is complete.
-  void OnShutdown() {
-    message_loop_.Quit();
-  }
-
   // Called when the "Test" signal is received, in the main thread.
   // Copy the string payload to |test_signal_string_|.
   void OnTestSignal(dbus::Signal* signal) {
diff --git a/dbus/end_to_end_sync_unittest.cc b/dbus/end_to_end_sync_unittest.cc
index 8968985..c338809 100644
--- a/dbus/end_to_end_sync_unittest.cc
+++ b/dbus/end_to_end_sync_unittest.cc
@@ -37,8 +37,7 @@ class EndToEndSyncTest : public testing::Test {
   }
 
   virtual void TearDown() {
-    test_service_->Shutdown();
-    ASSERT_TRUE(test_service_->WaitUntilServiceIsShutdown());
+    test_service_->ShutdownAndBlock();
     test_service_->Stop();
     client_bus_->ShutdownAndBlock();
   }
diff --git a/dbus/mock_bus.h b/dbus/mock_bus.h
index f1c5b4e..7b949cb 100644
--- a/dbus/mock_bus.h
+++ b/dbus/mock_bus.h
@@ -25,7 +25,7 @@ class MockBus : public Bus {
       const std::string& service_name,
       const std::string& object_path));
   MOCK_METHOD0(ShutdownAndBlock, void());
-  MOCK_METHOD1(Shutdown, void(OnShutdownCallback callback));
+  MOCK_METHOD0(ShutdownOnDBusThreadAndBlock, void());
   MOCK_METHOD0(Connect, bool());
   MOCK_METHOD1(RequestOwnership, bool(const std::string& service_name));
   MOCK_METHOD1(ReleaseOwnership, bool(const std::string& service_name));
diff --git a/dbus/test_service.cc b/dbus/test_service.cc
index acab607..1b8b9bf 100644
--- a/dbus/test_service.cc
+++ b/dbus/test_service.cc
@@ -26,7 +26,6 @@ TestService::Options::~Options() {
 TestService::TestService(const Options& options)
     : base::Thread("TestService"),
       dbus_thread_(options.dbus_thread),
-      on_shutdown_(false /* manual_reset */, false /* initially_signaled */),
       on_all_methods_exported_(false, false),
       num_exported_methods_(0) {
 }
@@ -48,24 +47,24 @@ bool TestService::WaitUntilServiceIsStarted() {
   return on_all_methods_exported_.TimedWait(timeout);
 }
 
-void TestService::Shutdown() {
+void TestService::ShutdownAndBlock() {
   message_loop()->PostTask(
       FROM_HERE,
-      base::Bind(&TestService::ShutdownInternal,
+      base::Bind(&TestService::ShutdownAndBlockInternal,
                  base::Unretained(this)));
 }
 
-bool TestService::WaitUntilServiceIsShutdown() {
-  const base::TimeDelta timeout(
-      base::TimeDelta::FromMilliseconds(
-          TestTimeouts::action_max_timeout_ms()));
-  return on_shutdown_.TimedWait(timeout);
-}
-
 bool TestService::HasDBusThread() {
   return bus_->HasDBusThread();
 }
 
+void TestService::ShutdownAndBlockInternal() {
+  if (HasDBusThread())
+    bus_->ShutdownOnDBusThreadAndBlock();
+  else
+    bus_->ShutdownAndBlock();
+}
+
 void TestService::SendTestSignal(const std::string& message) {
   message_loop()->PostTask(
       FROM_HERE,
@@ -81,11 +80,6 @@ void TestService::SendTestSignalInternal(const std::string& message) {
   exported_object_->SendSignal(&signal);
 }
 
-void TestService::ShutdownInternal() {
-  bus_->Shutdown(base::Bind(&TestService::OnShutdown,
-                            base::Unretained(this)));
-}
-
 void TestService::OnExported(const std::string& interface_name,
                              const std::string& method_name,
                              bool success) {
@@ -102,10 +96,6 @@ void TestService::OnExported(const std::string& interface_name,
     on_all_methods_exported_.Signal();
 }
 
-void TestService::OnShutdown() {
-  on_shutdown_.Signal();
-}
-
 void TestService::Run(MessageLoop* message_loop) {
   Bus::Options bus_options;
   bus_options.bus_type = Bus::SESSION;
diff --git a/dbus/test_service.h b/dbus/test_service.h
index 0ed8462..348aa73 100644
--- a/dbus/test_service.h
+++ b/dbus/test_service.h
@@ -49,12 +49,8 @@ class TestService : public base::Thread {
   // Returns true on success.
   bool WaitUntilServiceIsStarted() WARN_UNUSED_RESULT;
 
-  // Shuts down the service.
-  void Shutdown();
-
-  // Waits until the service is shut down.
-  // Returns true on success.
-  bool WaitUntilServiceIsShutdown() WARN_UNUSED_RESULT;
+  // Shuts down the service and blocks until it's done.
+  void ShutdownAndBlock();
 
   // Returns true if the bus has the D-Bus thread.
   bool HasDBusThread();
@@ -66,17 +62,14 @@ class TestService : public base::Thread {
   // Helper function for SendTestSignal().
   void SendTestSignalInternal(const std::string& message);
 
-  // Helper function for Shutdown().
-  void ShutdownInternal();
+  // Helper function for ShutdownAndBlock().
+  void ShutdownAndBlockInternal();
 
   // Called when a method is exported.
   void OnExported(const std::string& interface_name,
                   const std::string& method_name,
                   bool success);
 
-  // Called when the bus is shut down.
-  void OnShutdown();
-
   // base::Thread override.
   virtual void Run(MessageLoop* message_loop);
 
@@ -95,7 +88,6 @@ class TestService : public base::Thread {
   Response* BrokenMethod(MethodCall* method_call);
 
   base::Thread* dbus_thread_;
-  base::WaitableEvent on_shutdown_;
   base::WaitableEvent on_all_methods_exported_;
   // The number of methods actually exported.
   int num_exported_methods_;