From db817802ed529f7064928426ef9ba8f0d427dc2b Mon Sep 17 00:00:00 2001 From: "hashimoto@chromium.org" Date: Fri, 27 Sep 2013 07:12:03 +0000 Subject: dbus: Stop accessing ObjectProxy::name_owner_changed_callback_ on the D-Bus thread Accessing the callback on the D-Bus thread while changing its value may result in a race condition. Change the type of SetNameOwnerChangedCallback's argument from Signal to strings to stop worrying about on which thread the Signal gets released. BUG=298747 TEST=dbus_unittests R=satorux@chromium.org Review URL: https://codereview.chromium.org/24673006 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@225675 0039d316-1c4b-4281-b951-d872f2087c98 --- dbus/object_proxy.cc | 37 ++++++++++++++--------------- dbus/object_proxy.h | 13 ++++++++-- dbus/signal_sender_verification_unittest.cc | 9 +++---- 3 files changed, 32 insertions(+), 27 deletions(-) (limited to 'dbus') diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc index 5ff0b86..f2c4ebd 100644 --- a/dbus/object_proxy.cc +++ b/dbus/object_proxy.cc @@ -185,6 +185,13 @@ void ObjectProxy::ConnectToSignal(const std::string& interface_name, signal_name)); } +void ObjectProxy::SetNameOwnerChangedCallback( + NameOwnerChangedCallback callback) { + bus_->AssertOnOriginThread(); + + name_owner_changed_callback_ = callback; +} + void ObjectProxy::Detach() { bus_->AssertOnDBusThread(); @@ -407,12 +414,6 @@ bool ObjectProxy::ConnectToSignalInternal(const std::string& interface_name, return success; } -void ObjectProxy::SetNameOwnerChangedCallback(SignalCallback callback) { - bus_->AssertOnOriginThread(); - - name_owner_changed_callback_ = callback; -} - DBusHandlerResult ObjectProxy::HandleMessage( DBusConnection* connection, DBusMessage* raw_message) { @@ -620,19 +621,10 @@ DBusHandlerResult ObjectProxy::HandleNameOwnerChanged( 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(); - std::vector callbacks; - callbacks.push_back(name_owner_changed_callback_); - bus_->GetOriginTaskRunner()->PostTask( - FROM_HERE, - base::Bind(&ObjectProxy::RunMethod, - this, - start_time, - callbacks, - released_signal)); - } + bus_->GetOriginTaskRunner()->PostTask( + FROM_HERE, + base::Bind(&ObjectProxy::RunNameOwnerChangedCallback, + this, old_owner, new_owner)); } } @@ -641,4 +633,11 @@ DBusHandlerResult ObjectProxy::HandleNameOwnerChanged( return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } +void ObjectProxy::RunNameOwnerChangedCallback(const std::string& old_owner, + const std::string& new_owner) { + bus_->AssertOnOriginThread(); + if (!name_owner_changed_callback_.is_null()) + name_owner_changed_callback_.Run(old_owner, new_owner); +} + } // namespace dbus diff --git a/dbus/object_proxy.h b/dbus/object_proxy.h index 79e15d14..e618d5d 100644 --- a/dbus/object_proxy.h +++ b/dbus/object_proxy.h @@ -72,6 +72,11 @@ class CHROME_DBUS_EXPORT ObjectProxy // Called when a signal is received. Signal* is the incoming signal. typedef base::Callback SignalCallback; + // Called when NameOwnerChanged signal is received. + typedef base::Callback NameOwnerChangedCallback; + // Called when the object proxy is connected to the signal. // Parameters: // - the interface name. @@ -145,7 +150,7 @@ class CHROME_DBUS_EXPORT ObjectProxy // 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); + virtual void SetNameOwnerChangedCallback(NameOwnerChangedCallback callback); // Detaches from the remote object. The Bus object will take care of // detaching so you don't have to do this manually. @@ -253,6 +258,10 @@ class CHROME_DBUS_EXPORT ObjectProxy // Handles NameOwnerChanged signal from D-Bus's special message bus. DBusHandlerResult HandleNameOwnerChanged(scoped_ptr signal); + // Runs |name_owner_changed_callback_|. + void RunNameOwnerChangedCallback(const std::string& old_owner, + const std::string& new_owner); + scoped_refptr bus_; std::string service_name_; ObjectPath object_path_; @@ -266,7 +275,7 @@ class CHROME_DBUS_EXPORT ObjectProxy MethodTable method_table_; // The callback called when NameOwnerChanged signal is received. - SignalCallback name_owner_changed_callback_; + NameOwnerChangedCallback name_owner_changed_callback_; std::set match_rules_; diff --git a/dbus/signal_sender_verification_unittest.cc b/dbus/signal_sender_verification_unittest.cc index cbf4b5f..1d8f6e1 100644 --- a/dbus/signal_sender_verification_unittest.cc +++ b/dbus/signal_sender_verification_unittest.cc @@ -120,12 +120,9 @@ class SignalSenderVerificationTest : public testing::Test { message_loop_.Quit(); } - void OnNameOwnerChanged(bool* called_flag, Signal* signal) { - 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)); + void OnNameOwnerChanged(bool* called_flag, + const std::string& old_owner, + const std::string& new_owner) { latest_name_owner_ = new_owner; *called_flag = true; message_loop_.Quit(); -- cgit v1.1