Allow Chromium's DBus service ownership to be stealable

We've seen some cases in tests where a Chromium process winds up in a
temporarily unkillable state, causing the dbus-daemon to believe that
it still actively owns org.chromium.LibCrosService.  This makes
attempts to restart the UI fail, as the browser dies when it cannot
take ownership of this service name. The reason it can't is because
Chromium currently doesn't allow other processes to steal ownership --
and the unkillable process is holding onto the token.

This can be remedied by providing certain options when ownership of
the service name is taken, options that allow other processes to seize
ownership if they so choose. The ramifications of this are discussed
further in the bug.

BUG=chromium:261381
TEST=new unit test in dbus_unittest
TEST=run the following as chronos on a device: "gdbus call --system --dest org.freedesktop.DBus --object-path /org/freedesktop/DBus --method org.freedesktop.DBus.RequestName org.chromium.LibCrosService 7"
TEST=This should return (uint32 1,)

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

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

7 files changed, 115 insertions, 11 deletions

diff --git a/dbus/bus.cc b/dbus/bus.cc
index 2be649e..ec2417e 100644
--- a/dbus/bus.cc
+++ b/dbus/bus.cc
@@ -501,21 +501,23 @@ void Bus::ShutdownOnDBusThreadAndBlock() {
 }
 
 void Bus::RequestOwnership(const std::string& service_name,
+                           ServiceOwnershipOptions options,
                            OnOwnershipCallback on_ownership_callback) {
   AssertOnOriginThread();
 
   PostTaskToDBusThread(FROM_HERE, base::Bind(
       &Bus::RequestOwnershipInternal,
-      this, service_name, on_ownership_callback));
+      this, service_name, options, on_ownership_callback));
 }
 
 void Bus::RequestOwnershipInternal(const std::string& service_name,
+                                   ServiceOwnershipOptions options,
                                    OnOwnershipCallback on_ownership_callback) {
   AssertOnDBusThread();
 
   bool success = Connect();
   if (success)
-    success = RequestOwnershipAndBlock(service_name);
+    success = RequestOwnershipAndBlock(service_name, options);
 
   PostTaskToOriginThread(FROM_HERE,
                          base::Bind(on_ownership_callback,
@@ -523,7 +525,8 @@ void Bus::RequestOwnershipInternal(const std::string& service_name,
                                     success));
 }
 
-bool Bus::RequestOwnershipAndBlock(const std::string& service_name) {
+bool Bus::RequestOwnershipAndBlock(const std::string& service_name,
+                                   ServiceOwnershipOptions options) {
   DCHECK(connection_);
   // dbus_bus_request_name() is a blocking call.
   AssertOnDBusThread();
@@ -536,7 +539,7 @@ bool Bus::RequestOwnershipAndBlock(const std::string& service_name) {
   ScopedDBusError error;
   const int result = dbus_bus_request_name(connection_,
                                            service_name.c_str(),
-                                           DBUS_NAME_FLAG_DO_NOT_QUEUE,
+                                           options,
                                            error.get());
   if (result != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) {
     LOG(ERROR) << "Failed to get the ownership of " << service_name << ": "
@@ -568,7 +571,8 @@ bool Bus::ReleaseOwnership(const std::string& service_name) {
     return true;
   } else {
     LOG(ERROR) << "Failed to release the ownership of " << service_name << ": "
-               << (error.is_set() ? error.message() : "");
+               << (error.is_set() ? error.message() : "")
+               << ", result code: " << result;
     return false;
   }
 }
diff --git a/dbus/bus.h b/dbus/bus.h
index 220ed78..2e6db70 100644
--- a/dbus/bus.h
+++ b/dbus/bus.h
@@ -168,6 +168,22 @@ class CHROME_DBUS_EXPORT Bus : public base::RefCountedThreadSafe<Bus> {
     SUPPRESS_ERRORS,
   };
 
+  // Specifies service ownership options.
+  //
+  // REQUIRE_PRIMARY indicates that you require primary ownership of the
+  // service name.
+  //
+  // ALLOW_REPLACEMENT indicates that you'll allow another connection to
+  // steal ownership of this service name from you.
+  //
+  // REQUIRE_PRIMARY_ALLOW_REPLACEMENT does the obvious.
+  enum ServiceOwnershipOptions {
+    REQUIRE_PRIMARY = (DBUS_NAME_FLAG_DO_NOT_QUEUE |
+                       DBUS_NAME_FLAG_REPLACE_EXISTING),
+    REQUIRE_PRIMARY_ALLOW_REPLACEMENT = (REQUIRE_PRIMARY |
+                                         DBUS_NAME_FLAG_ALLOW_REPLACEMENT),
+  };
+
   // Options used to create a Bus object.
   struct CHROME_DBUS_EXPORT Options {
     Options();
@@ -398,13 +414,15 @@ class CHROME_DBUS_EXPORT Bus : public base::RefCountedThreadSafe<Bus> {
   //
   // Must be called in the origin thread.
   virtual void RequestOwnership(const std::string& service_name,
+                                ServiceOwnershipOptions options,
                                 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 RequestOwnershipAndBlock(const std::string& service_name);
+  virtual bool RequestOwnershipAndBlock(const std::string& service_name,
+                                        ServiceOwnershipOptions options);
 
   // Releases the ownership of the given service name.
   // Returns true on success.
@@ -608,6 +626,7 @@ class CHROME_DBUS_EXPORT Bus : public base::RefCountedThreadSafe<Bus> {
 
   // Helper function used for RequestOwnership().
   void RequestOwnershipInternal(const std::string& service_name,
+                                ServiceOwnershipOptions options,
                                 OnOwnershipCallback on_ownership_callback);
 
   // Helper function used for GetServiceOwner().
diff --git a/dbus/bus_unittest.cc b/dbus/bus_unittest.cc
index 664ceb5..d695569 100644
--- a/dbus/bus_unittest.cc
+++ b/dbus/bus_unittest.cc
@@ -372,7 +372,8 @@ TEST(BusTest, ListenForServiceOwnerChange) {
   EXPECT_EQ(0, num_of_owner_changes1);
 
   // Make an ownership change.
-  ASSERT_TRUE(bus->RequestOwnershipAndBlock("org.chromium.TestService"));
+  ASSERT_TRUE(bus->RequestOwnershipAndBlock("org.chromium.TestService",
+                                            Bus::REQUIRE_PRIMARY));
   run_loop_state.Run(1);
 
   {
diff --git a/dbus/mock_bus.h b/dbus/mock_bus.h
index 26113d4..c3991f5 100644
--- a/dbus/mock_bus.h
+++ b/dbus/mock_bus.h
@@ -31,10 +31,12 @@ class MockBus : public Bus {
   MOCK_METHOD0(ShutdownAndBlock, void());
   MOCK_METHOD0(ShutdownOnDBusThreadAndBlock, void());
   MOCK_METHOD0(Connect, bool());
-  MOCK_METHOD2(RequestOwnership, void(
+  MOCK_METHOD3(RequestOwnership, void(
       const std::string& service_name,
+      ServiceOwnershipOptions options,
       OnOwnershipCallback on_ownership_callback));
-  MOCK_METHOD1(RequestOwnershipAndBlock, bool(const std::string& service_name));
+  MOCK_METHOD2(RequestOwnershipAndBlock, bool(const std::string& service_name,
+                                              ServiceOwnershipOptions options));
   MOCK_METHOD1(ReleaseOwnership, bool(const std::string& service_name));
   MOCK_METHOD0(SetUpAsyncOperations, bool());
   MOCK_METHOD3(SendWithReplyAndBlock, DBusMessage*(DBusMessage* request,
diff --git a/dbus/signal_sender_verification_unittest.cc b/dbus/signal_sender_verification_unittest.cc
index df2acd2..cbf4b5f 100644
--- a/dbus/signal_sender_verification_unittest.cc
+++ b/dbus/signal_sender_verification_unittest.cc
@@ -154,6 +154,14 @@ class SignalSenderVerificationTest : public testing::Test {
     message_loop_.Run();
   }
 
+  // Stopping a thread is considered an IO operation, so we need to fiddle with
+  // thread restrictions before and after calling Stop() on a TestService.
+  void SafeServiceStop(TestService* test_service) {
+    base::ThreadRestrictions::SetIOAllowed(true);
+    test_service->Stop();
+    base::ThreadRestrictions::SetIOAllowed(false);
+  }
+
   base::MessageLoop message_loop_;
   scoped_ptr<base::Thread> dbus_thread_;
   scoped_refptr<Bus> bus_;
@@ -224,6 +232,7 @@ TEST_F(SignalSenderVerificationTest, TestOwnerChanged) {
 
   // Reset the flag as NameOwnerChanged is already received in setup.
   on_name_owner_changed_called_ = false;
+  on_ownership_called_ = false;
   test_service2_->RequestOwnership(
       base::Bind(&SignalSenderVerificationTest::OnOwnership,
                  base::Unretained(this), true));
@@ -246,6 +255,64 @@ TEST_F(SignalSenderVerificationTest, TestOwnerChanged) {
   ASSERT_EQ(kNewMessage, test_signal_string_);
 }
 
+TEST_F(SignalSenderVerificationTest, TestOwnerStealing) {
+  // Release and acquire the name ownership.
+  // latest_name_owner_ should be non empty as |test_service_| owns the name.
+  ASSERT_FALSE(latest_name_owner_.empty());
+  test_service_->ShutdownAndBlock();
+  // OnNameOwnerChanged will PostTask to quit the message loop.
+  message_loop_.Run();
+  // latest_name_owner_ should be empty as the owner is gone.
+  ASSERT_TRUE(latest_name_owner_.empty());
+  // Reset the flag as NameOwnerChanged is already received in setup.
+  on_name_owner_changed_called_ = false;
+
+  // Start a test service that allows theft, using the D-Bus thread.
+  TestService::Options options;
+  options.dbus_task_runner = dbus_thread_->message_loop_proxy();
+  options.request_ownership_options = Bus::REQUIRE_PRIMARY_ALLOW_REPLACEMENT;
+  TestService stealable_test_service(options);
+  ASSERT_TRUE(stealable_test_service.StartService());
+  ASSERT_TRUE(stealable_test_service.WaitUntilServiceIsStarted());
+  ASSERT_TRUE(stealable_test_service.HasDBusThread());
+  ASSERT_TRUE(stealable_test_service.has_ownership());
+
+  // OnNameOwnerChanged will PostTask to quit the message loop.
+  message_loop_.Run();
+
+  // Send a signal to check that the service is correctly owned.
+  const char kMessage[] = "hello, world";
+
+  // Send the test signal from the exported object.
+  stealable_test_service.SendTestSignal(kMessage);
+  // Receive the signal with the object proxy. The signal is handled in
+  // SignalSenderVerificationTest::OnTestSignal() in the main thread.
+  WaitForTestSignal();
+  ASSERT_EQ(kMessage, test_signal_string_);
+
+  // Reset the flag as NameOwnerChanged was called above.
+  on_name_owner_changed_called_ = false;
+  test_service2_->RequestOwnership(
+      base::Bind(&SignalSenderVerificationTest::OnOwnership,
+                 base::Unretained(this), true));
+  // Both of OnNameOwnerChanged() and OnOwnership() should quit the MessageLoop,
+  // but there's no expected order of those 2 event.
+  message_loop_.Run();
+  if (!on_name_owner_changed_called_ || !on_ownership_called_)
+    message_loop_.Run();
+  ASSERT_TRUE(on_name_owner_changed_called_);
+  ASSERT_TRUE(on_ownership_called_);
+
+  // Now the second service owns the name.
+  const char kNewMessage[] = "hello, new world";
+
+  test_service2_->SendTestSignal(kNewMessage);
+  WaitForTestSignal();
+  ASSERT_EQ(kNewMessage, test_signal_string_);
+
+  SafeServiceStop(&stealable_test_service);
+}
+
 // Fails on Linux ChromiumOS Tests
 TEST_F(SignalSenderVerificationTest, DISABLED_TestMultipleObjects) {
   const char kMessage[] = "hello, world";
diff --git a/dbus/test_service.cc b/dbus/test_service.cc
index ae67bf7..5f59501 100644
--- a/dbus/test_service.cc
+++ b/dbus/test_service.cc
@@ -27,7 +27,8 @@ namespace dbus {
 // GetManagedObjects.
 const int TestService::kNumMethodsToExport = 9;
 
-TestService::Options::Options() {
+TestService::Options::Options()
+    : request_ownership_options(Bus::REQUIRE_PRIMARY) {
 }
 
 TestService::Options::~Options() {
@@ -35,6 +36,7 @@ TestService::Options::~Options() {
 
 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),
       num_exported_methods_(0) {
@@ -103,6 +105,7 @@ void TestService::SendTestSignalFromRootInternal(const std::string& message) {
   writer.AppendString(message);
 
   bus_->RequestOwnership("org.chromium.TestService",
+                         request_ownership_options_,
                          base::Bind(&TestService::OnOwnership,
                                     base::Unretained(this),
                                     base::Bind(&EmptyCallback)));
@@ -123,6 +126,7 @@ void TestService::RequestOwnership(base::Callback<void(bool)> callback) {
 void TestService::RequestOwnershipInternal(
     base::Callback<void(bool)> callback) {
   bus_->RequestOwnership("org.chromium.TestService",
+                         request_ownership_options_,
                          base::Bind(&TestService::OnOwnership,
                                     base::Unretained(this),
                                     callback));
@@ -160,6 +164,7 @@ void TestService::Run(base::MessageLoop* message_loop) {
   bus_ = new Bus(bus_options);
 
   bus_->RequestOwnership("org.chromium.TestService",
+                         request_ownership_options_,
                          base::Bind(&TestService::OnOwnership,
                                     base::Unretained(this),
                                     base::Bind(&EmptyCallback)));
diff --git a/dbus/test_service.h b/dbus/test_service.h
index 765a630..7ddaf21 100644
--- a/dbus/test_service.h
+++ b/dbus/test_service.h
@@ -9,6 +9,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/threading/thread.h"
 #include "base/synchronization/waitable_event.h"
+#include "dbus/bus.h"
 #include "dbus/exported_object.h"
 
 namespace base {
@@ -17,7 +18,6 @@ class SequencedTaskRunner;
 
 namespace dbus {
 
-class Bus;
 class MethodCall;
 class MessageWriter;
 class Response;
@@ -37,6 +37,9 @@ class TestService : public base::Thread {
 
     // NULL by default (i.e. don't use the D-Bus thread).
     scoped_refptr<base::SequencedTaskRunner> dbus_task_runner;
+
+    // Flags governing parameters of service ownership request.
+    Bus::ServiceOwnershipOptions request_ownership_options;
   };
 
   // The number of methods we'll export.
@@ -163,6 +166,9 @@ class TestService : public base::Thread {
   // Helper function for RequestOwnership().
   void RequestOwnershipInternal(base::Callback<void(bool)> callback);
 
+  // Options to use when requesting service ownership.
+  Bus::ServiceOwnershipOptions request_ownership_options_;
+
   scoped_refptr<base::SequencedTaskRunner> dbus_task_runner_;
   base::WaitableEvent on_all_methods_exported_;
   // The number of methods actually exported.