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
CrOS-Libchrome-Original-Commit: db817802ed529f7064928426ef9ba8f0d427dc2b
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 @@
signal_name));
}
+void ObjectProxy::SetNameOwnerChangedCallback(
+ NameOwnerChangedCallback callback) {
+ bus_->AssertOnOriginThread();
+
+ name_owner_changed_callback_ = callback;
+}
+
void ObjectProxy::Detach() {
bus_->AssertOnDBusThread();
@@ -407,12 +414,6 @@
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 @@
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<SignalCallback> 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 @@
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 79e15d1..e618d5d 100644
--- a/dbus/object_proxy.h
+++ b/dbus/object_proxy.h
@@ -72,6 +72,11 @@
// Called when a signal is received. Signal* is the incoming signal.
typedef base::Callback<void (Signal*)> SignalCallback;
+ // Called when NameOwnerChanged signal is received.
+ typedef base::Callback<void(
+ const std::string& old_owner,
+ const std::string& new_owner)> NameOwnerChangedCallback;
+
// Called when the object proxy is connected to the signal.
// Parameters:
// - the interface name.
@@ -145,7 +150,7 @@
// 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 @@
// Handles NameOwnerChanged signal from D-Bus's special message bus.
DBusHandlerResult HandleNameOwnerChanged(scoped_ptr<dbus::Signal> signal);
+ // Runs |name_owner_changed_callback_|.
+ void RunNameOwnerChangedCallback(const std::string& old_owner,
+ const std::string& new_owner);
+
scoped_refptr<Bus> bus_;
std::string service_name_;
ObjectPath object_path_;
@@ -266,7 +275,7 @@
MethodTable method_table_;
// The callback called when NameOwnerChanged signal is received.
- SignalCallback name_owner_changed_callback_;
+ NameOwnerChangedCallback name_owner_changed_callback_;
std::set<std::string> 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 @@
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();