diff options
author | haruki@chromium.org <haruki@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-11-14 11:02:59 +0000 |
---|---|---|
committer | haruki@chromium.org <haruki@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-11-14 11:02:59 +0000 |
commit | 6d36c0cadfc825db5336a04c1c0778ebe2652a92 (patch) | |
tree | 451eecc45061e3bc58c02deb20032a37d49c733f /dbus | |
parent | 8a2e513493bd5b0d2eb8e068975efc2e91fb5250 (diff) | |
download | chromium_src-6d36c0cadfc825db5336a04c1c0778ebe2652a92.zip chromium_src-6d36c0cadfc825db5336a04c1c0778ebe2652a92.tar.gz chromium_src-6d36c0cadfc825db5336a04c1c0778ebe2652a92.tar.bz2 |
Make SignalSenderVerificationTest more robust
Add more assertions and a callback to check the result of RequestOwnership.
Original test can hang when test_service2_ tries to acquire the ownership before D-Bus recognizes test_service_'s disconnection.
In that situation, test_service2_ cannot own the name and cannot send a message.
BUG=158689
TEST=unittests
Review URL: https://chromiumcodereview.appspot.com/11358111
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@167649 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'dbus')
-rw-r--r-- | dbus/object_proxy.cc | 23 | ||||
-rw-r--r-- | dbus/object_proxy.h | 10 | ||||
-rw-r--r-- | dbus/signal_sender_verification_unittest.cc | 105 | ||||
-rw-r--r-- | dbus/test_service.cc | 29 | ||||
-rw-r--r-- | dbus/test_service.h | 19 |
5 files changed, 153 insertions, 33 deletions
diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc index c8f00d7..5e9bf5b 100644 --- a/dbus/object_proxy.cc +++ b/dbus/object_proxy.cc @@ -408,6 +408,12 @@ void ObjectProxy::OnConnected(OnConnectedCallback on_connected_callback, on_connected_callback.Run(interface_name, signal_name, success); } +void ObjectProxy::SetNameOwnerChangedCallback(SignalCallback callback) { + bus_->AssertOnOriginThread(); + + name_owner_changed_callback_ = callback; +} + DBusHandlerResult ObjectProxy::HandleMessage( DBusConnection* connection, DBusMessage* raw_message) { @@ -430,7 +436,7 @@ DBusHandlerResult ObjectProxy::HandleMessage( if (path.value() == kDbusSystemObjectPath && signal->GetMember() == "NameOwnerChanged") { // Handle NameOwnerChanged separately - return HandleNameOwnerChanged(signal.get()); + return HandleNameOwnerChanged(signal.Pass()); } return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } @@ -619,7 +625,8 @@ void ObjectProxy::UpdateNameOwnerAndBlock() { service_name_owner_.clear(); } -DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(Signal* signal) { +DBusHandlerResult ObjectProxy::HandleNameOwnerChanged( + scoped_ptr<Signal> signal) { DCHECK(signal); bus_->AssertOnDBusThread(); @@ -627,13 +634,23 @@ DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(Signal* signal) { if (signal->GetMember() == "NameOwnerChanged" && signal->GetInterface() == "org.freedesktop.DBus" && signal->GetSender() == "org.freedesktop.DBus") { - MessageReader reader(signal); + MessageReader reader(signal.get()); std::string name, old_owner, new_owner; if (reader.PopString(&name) && reader.PopString(&old_owner) && reader.PopString(&new_owner) && name == service_name_) { service_name_owner_ = new_owner; + if (!name_owner_changed_callback_.is_null()) { + const base::TimeTicks start_time = base::TimeTicks::Now(); + Signal* released_signal = signal.release(); + bus_->PostTaskToOriginThread(FROM_HERE, + base::Bind(&ObjectProxy::RunMethod, + this, + start_time, + name_owner_changed_callback_, + released_signal)); + } return DBUS_HANDLER_RESULT_HANDLED; } } diff --git a/dbus/object_proxy.h b/dbus/object_proxy.h index bec0894..801c577 100644 --- a/dbus/object_proxy.h +++ b/dbus/object_proxy.h @@ -142,6 +142,11 @@ class CHROME_DBUS_EXPORT ObjectProxy SignalCallback signal_callback, OnConnectedCallback on_connected_callback); + // Sets a callback for "NameOwnerChanged" signal. The callback is called on + // the origin thread when D-Bus system sends "NameOwnerChanged" for the name + // represented by |service_name_|. + virtual void SetNameOwnerChangedCallback(SignalCallback callback); + // Detaches from the remote object. The Bus object will take care of // detaching so you don't have to do this manually. // @@ -254,7 +259,7 @@ class CHROME_DBUS_EXPORT ObjectProxy void UpdateNameOwnerAndBlock(); // Handles NameOwnerChanged signal from D-Bus's special message bus. - DBusHandlerResult HandleNameOwnerChanged(dbus::Signal* signal); + DBusHandlerResult HandleNameOwnerChanged(scoped_ptr<dbus::Signal> signal); scoped_refptr<Bus> bus_; std::string service_name_; @@ -268,6 +273,9 @@ class CHROME_DBUS_EXPORT ObjectProxy typedef std::map<std::string, SignalCallback> MethodTable; MethodTable method_table_; + // The callback called when NameOwnerChanged signal is received. + SignalCallback name_owner_changed_callback_; + std::set<std::string> match_rules_; const bool ignore_service_unknown_errors_; diff --git a/dbus/signal_sender_verification_unittest.cc b/dbus/signal_sender_verification_unittest.cc index 91db93f..6e59ea4 100644 --- a/dbus/signal_sender_verification_unittest.cc +++ b/dbus/signal_sender_verification_unittest.cc @@ -20,7 +20,9 @@ // The test for sender verification in ObjectProxy. class SignalSenderVerificationTest : public testing::Test { public: - SignalSenderVerificationTest() { + SignalSenderVerificationTest() + : on_name_owner_changed_called_(false), + on_ownership_called_(false) { } virtual void SetUp() { @@ -35,21 +37,6 @@ class SignalSenderVerificationTest : public testing::Test { thread_options.message_loop_type = MessageLoop::TYPE_IO; ASSERT_TRUE(dbus_thread_->StartWithOptions(thread_options)); - // Start the test service, using the D-Bus thread. - dbus::TestService::Options options; - options.dbus_thread_message_loop_proxy = dbus_thread_->message_loop_proxy(); - test_service_.reset(new dbus::TestService(options)); - ASSERT_TRUE(test_service_->StartService()); - ASSERT_TRUE(test_service_->WaitUntilServiceIsStarted()); - ASSERT_TRUE(test_service_->HasDBusThread()); - - // Same setup for the second TestService. This service should not have the - // ownership of the name at this point. - test_service2_.reset(new dbus::TestService(options)); - ASSERT_TRUE(test_service2_->StartService()); - ASSERT_TRUE(test_service2_->WaitUntilServiceIsStarted()); - ASSERT_TRUE(test_service2_->HasDBusThread()); - // Create the client, using the D-Bus thread. dbus::Bus::Options bus_options; bus_options.bus_type = dbus::Bus::SESSION; @@ -62,6 +49,10 @@ class SignalSenderVerificationTest : public testing::Test { dbus::ObjectPath("/org/chromium/TestObject")); ASSERT_TRUE(bus_->HasDBusThread()); + object_proxy_->SetNameOwnerChangedCallback( + base::Bind(&SignalSenderVerificationTest::OnNameOwnerChanged, + base::Unretained(this))); + // Connect to the "Test" signal of "org.chromium.TestInterface" from // the remote object. object_proxy_->ConnectToSignal( @@ -73,6 +64,29 @@ class SignalSenderVerificationTest : public testing::Test { base::Unretained(this))); // Wait until the object proxy is connected to the signal. message_loop_.Run(); + + // Start the test service, using the D-Bus thread. + dbus::TestService::Options options; + options.dbus_thread_message_loop_proxy = dbus_thread_->message_loop_proxy(); + test_service_.reset(new dbus::TestService(options)); + ASSERT_TRUE(test_service_->StartService()); + ASSERT_TRUE(test_service_->WaitUntilServiceIsStarted()); + ASSERT_TRUE(test_service_->HasDBusThread()); + ASSERT_TRUE(test_service_->has_ownership()); + + // Same setup for the second TestService. This service should not have the + // ownership of the name at this point. + test_service2_.reset(new dbus::TestService(options)); + ASSERT_TRUE(test_service2_->StartService()); + ASSERT_TRUE(test_service2_->WaitUntilServiceIsStarted()); + ASSERT_TRUE(test_service2_->HasDBusThread()); + ASSERT_FALSE(test_service2_->has_ownership()); + + // The name should be owned and known at this point. + if (!on_name_owner_changed_called_) + message_loop_.Run(); + ASSERT_FALSE(latest_name_owner_.empty()); + } virtual void TearDown() { @@ -91,6 +105,31 @@ class SignalSenderVerificationTest : public testing::Test { test_service2_->Stop(); } + void OnOwnership(bool expected, bool success) { + ASSERT_EQ(expected, success); + // PostTask to quit the MessageLoop as this is called from D-Bus thread. + message_loop_.PostTask( + FROM_HERE, + base::Bind(&SignalSenderVerificationTest::OnOwnershipInternal, + base::Unretained(this))); + } + + void OnOwnershipInternal() { + on_ownership_called_ = true; + message_loop_.Quit(); + } + + void OnNameOwnerChanged(dbus::Signal* signal) { + dbus::MessageReader reader(signal); + std::string name, old_owner, new_owner; + ASSERT_TRUE(reader.PopString(&name)); + ASSERT_TRUE(reader.PopString(&old_owner)); + ASSERT_TRUE(reader.PopString(&new_owner)); + latest_name_owner_ = new_owner; + on_name_owner_changed_called_ = true; + message_loop_.Quit(); + } + protected: // Called when the "Test" signal is received, in the main thread. @@ -123,6 +162,13 @@ class SignalSenderVerificationTest : public testing::Test { scoped_ptr<dbus::TestService> test_service2_; // Text message from "Test" signal. std::string test_signal_string_; + + // The known latest name owner of TestService. Updated in OnNameOwnerChanged. + std::string latest_name_owner_; + + // Boolean flags to record callback calls. + bool on_name_owner_changed_called_; + bool on_ownership_called_; }; TEST_F(SignalSenderVerificationTest, TestSignalAccepted) { @@ -157,7 +203,7 @@ TEST_F(SignalSenderVerificationTest, TestSignalRejected) { EXPECT_EQ(samples1->TotalCount() + 1, samples2->TotalCount()); } -TEST_F(SignalSenderVerificationTest, DISABLED_TestOwnerChanged) { +TEST_F(SignalSenderVerificationTest, TestOwnerChanged) { const char kMessage[] = "hello, world"; // Send the test signal from the exported object. @@ -167,9 +213,30 @@ TEST_F(SignalSenderVerificationTest, DISABLED_TestOwnerChanged) { WaitForTestSignal(); ASSERT_EQ(kMessage, test_signal_string_); - // Release and aquire the name ownership. + // 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(); - test_service2_->RequestOwnership(); + // 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; + 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_); + + // latest_name_owner_ becomes non empty as the new owner appears. + ASSERT_FALSE(latest_name_owner_.empty()); // Now the second service owns the name. const char kNewMessage[] = "hello, new world"; diff --git a/dbus/test_service.cc b/dbus/test_service.cc index 87c6e01..0d99d57 100644 --- a/dbus/test_service.cc +++ b/dbus/test_service.cc @@ -13,6 +13,13 @@ #include "dbus/object_path.h" #include "dbus/property.h" +namespace { + +void EmptyCallback(bool /* success */) { +} + +} // namespace + namespace dbus { // Echo, SlowEcho, AsyncEcho, BrokenMethod, GetAll, Get, Set. @@ -95,7 +102,8 @@ void TestService::SendTestSignalFromRootInternal(const std::string& message) { bus_->RequestOwnership("org.chromium.TestService", base::Bind(&TestService::OnOwnership, - base::Unretained(this))); + base::Unretained(this), + base::Bind(&EmptyCallback))); // Use "/" just like dbus-send does. ExportedObject* root_object = @@ -103,22 +111,28 @@ void TestService::SendTestSignalFromRootInternal(const std::string& message) { root_object->SendSignal(&signal); } -void TestService::RequestOwnership() { +void TestService::RequestOwnership(base::Callback<void(bool)> callback) { message_loop()->PostTask( FROM_HERE, base::Bind(&TestService::RequestOwnershipInternal, - base::Unretained(this))); + base::Unretained(this), + callback)); } -void TestService::RequestOwnershipInternal() { +void TestService::RequestOwnershipInternal( + base::Callback<void(bool)> callback) { bus_->RequestOwnership("org.chromium.TestService", base::Bind(&TestService::OnOwnership, - base::Unretained(this))); + base::Unretained(this), + callback)); } -void TestService::OnOwnership(const std::string& service_name, +void TestService::OnOwnership(base::Callback<void(bool)> callback, + const std::string& service_name, bool success) { + has_ownership_ = success; LOG_IF(ERROR, !success) << "Failed to own: " << service_name; + callback.Run(success); } void TestService::OnExported(const std::string& interface_name, @@ -146,7 +160,8 @@ void TestService::Run(MessageLoop* message_loop) { bus_->RequestOwnership("org.chromium.TestService", base::Bind(&TestService::OnOwnership, - base::Unretained(this))); + base::Unretained(this), + base::Bind(&EmptyCallback))); exported_object_ = bus_->GetExportedObject( dbus::ObjectPath("/org/chromium/TestObject")); diff --git a/dbus/test_service.h b/dbus/test_service.h index 17f27ca..30f8216 100644 --- a/dbus/test_service.h +++ b/dbus/test_service.h @@ -66,7 +66,12 @@ class TestService : public base::Thread { void SendTestSignalFromRoot(const std::string& message); // Request the ownership of a well-known name "TestService". - void RequestOwnership(); + // |callback| will be called with the result when an ownership request is + // completed. + void RequestOwnership(base::Callback<void(bool)> callback); + + // Returns whether this instance has the name ownership or not. + bool has_ownership() const { return has_ownership_; } private: // Helper function for SendTestSignal(). @@ -79,7 +84,12 @@ class TestService : public base::Thread { void ShutdownAndBlockInternal(); // Called when an ownership request is completed. - void OnOwnership(const std::string& service_name, + // |callback| is the callback to be called with the result. |service_name| is + // the requested well-known bus name. |callback| and |service_name| are bound + // when the service requests the ownership. |success| is the result of the + // completed request, and is propagated to |callback|. + void OnOwnership(base::Callback<void(bool)> callback, + const std::string& service_name, bool success); // Called when a method is exported. @@ -131,13 +141,16 @@ class TestService : public base::Thread { void SendPropertyChangedSignalInternal(const std::string& name); // Helper function for RequestOwnership(). - void RequestOwnershipInternal(); + void RequestOwnershipInternal(base::Callback<void(bool)> callback); scoped_refptr<base::MessageLoopProxy> dbus_thread_message_loop_proxy_; base::WaitableEvent on_all_methods_exported_; // The number of methods actually exported. int num_exported_methods_; + // True iff this instance has successfully acquired the name ownership. + bool has_ownership_; + scoped_refptr<Bus> bus_; ExportedObject* exported_object_; }; |