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

chromeos/dbus/cras_audio_client.cc

@@ -197,7 +197,8 @@ class CrasAudioClientImpl : public CrasAudioClient {
         << "Failed to connect to cras signal:" << signal_name;
   }
 
-  void NameOwnerChangedReceived(dbus::Signal* signal) {
+  void NameOwnerChangedReceived(const std::string& old_owner,
+                                const std::string& new_owner) {
     FOR_EACH_OBSERVER(Observer, observers_, AudioClientRestarted());
   }
 

chromeos/dbus/power_manager_client.cc

@@ -338,7 +338,8 @@ class PowerManagerClientImpl : public PowerManagerClient {
         dbus::ObjectProxy::EmptyResponseCallback());
   }
 
-  void NameOwnerChangedReceived(dbus::Signal* signal) {
+  void NameOwnerChangedReceived(const std::string& old_owner,
+                                const std::string& new_owner) {
     VLOG(1) << "Power manager restarted";
     RegisterSuspendDelay();
     SetIsProjecting(last_is_projecting_);

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<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 @@ 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

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<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 @@ 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<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 @@ 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<std::string> match_rules_;
 

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();