Skip to content

Commit

Permalink
Make SignalSenderVerificationTest more robust
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
haruki@chromium.org committed Nov 14, 2012
1 parent 8a2e513 commit 6d36c0c
Show file tree
Hide file tree
Showing 5 changed files with 153 additions and 33 deletions.
23 changes: 20 additions & 3 deletions dbus/object_proxy.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down Expand Up @@ -619,21 +625,32 @@ void ObjectProxy::UpdateNameOwnerAndBlock() {
service_name_owner_.clear();
}

DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(Signal* signal) {
DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(
scoped_ptr<Signal> signal) {
DCHECK(signal);
bus_->AssertOnDBusThread();

// Confirm the validity of the NameOwnerChanged 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;
}
}
Expand Down
10 changes: 9 additions & 1 deletion dbus/object_proxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down Expand Up @@ -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_;
Expand All @@ -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_;
Expand Down
105 changes: 86 additions & 19 deletions dbus/signal_sender_verification_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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;
Expand All @@ -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(
Expand All @@ -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() {
Expand All @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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";
Expand Down
29 changes: 22 additions & 7 deletions dbus/test_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -95,30 +102,37 @@ 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 =
bus_->GetExportedObject(dbus::ObjectPath("/"));
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,
Expand Down Expand Up @@ -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"));
Expand Down
19 changes: 16 additions & 3 deletions dbus/test_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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().
Expand All @@ -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.
Expand Down Expand Up @@ -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_;
};
Expand Down

0 comments on commit 6d36c0c

Please sign in to comment.