dbus: don't fail when reconnecting object signals

Since dbus::ObjectProxy is silently cached, with no way to invalidate,
it's possible that individual instances of objects will come and go
using the same underlying object proxy.  i.e. dbus::PropertySet

These will need to change the signal callbacks to be bound to their
own instance, so the current behaviour of failing in this case with
a log message is pessimal.

Change dbus::ObjectProxy to overwrite the existing signal callbacks
with the new ones on repeated calls, rather than preserve the first.

BUG=chromium-os:28064
TEST=unit test included, and we receive property notifications on devices after connection now

Change-Id: Ic4ae092163a364c53bdfcf88f4ce8f74b110b5cb

R=satorux@chromium.org


Review URL: http://codereview.chromium.org/9808001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128100 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 61 insertions, 8 deletions

diff --git a/dbus/end_to_end_async_unittest.cc b/dbus/end_to_end_async_unittest.cc
index 600c83a..9be24e0 100644
--- a/dbus/end_to_end_async_unittest.cc
+++ b/dbus/end_to_end_async_unittest.cc
@@ -343,3 +343,59 @@ TEST_F(EndToEndAsyncTest, TestSignalFromRoot) {
   // Verify the string WAS received by the root proxy.
   ASSERT_EQ(kMessage, root_test_signal_string_);
 }
+
+class SignalReplacementTest : public EndToEndAsyncTest {
+ public:
+  SignalReplacementTest() {
+  }
+
+  virtual void SetUp() {
+    // Set up base class.
+    EndToEndAsyncTest::SetUp();
+
+    // Reconnect the root object proxy's signal handler to a new handler
+    // so that we can verify that a second call to ConnectSignal() delivers
+    // to our new handler and not the old.
+    object_proxy_->ConnectToSignal(
+        "org.chromium.TestInterface",
+        "Test",
+        base::Bind(&SignalReplacementTest::OnReplacementTestSignal,
+                   base::Unretained(this)),
+        base::Bind(&SignalReplacementTest::OnReplacementConnected,
+                   base::Unretained(this)));
+    // Wait until the object proxy is connected to the signal.
+    message_loop_.Run();
+  }
+
+ protected:
+  // Called when the "Test" signal is received, in the main thread.
+  // Copy the string payload to |replacement_test_signal_string_|.
+  void OnReplacementTestSignal(dbus::Signal* signal) {
+    dbus::MessageReader reader(signal);
+    ASSERT_TRUE(reader.PopString(&replacement_test_signal_string_));
+    message_loop_.Quit();
+  }
+
+  // Called when connected to the signal.
+  void OnReplacementConnected(const std::string& interface_name,
+                              const std::string& signal_name,
+                              bool success) {
+    ASSERT_TRUE(success);
+    message_loop_.Quit();
+  }
+
+  // Text message from "Test" signal delivered to replacement handler.
+  std::string replacement_test_signal_string_;
+};
+
+TEST_F(SignalReplacementTest, TestSignalReplacement) {
+  const char kMessage[] = "hello, world";
+  // Send the test signal from the exported object.
+  test_service_->SendTestSignal(kMessage);
+  // Receive the signal with the object proxy.
+  WaitForTestSignal();
+  // Verify the string WAS NOT received by the original handler.
+  ASSERT_TRUE(test_signal_string_.empty());
+  // Verify the signal WAS received by the replacement handler.
+  ASSERT_EQ(kMessage, replacement_test_signal_string_);
+}
diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc
index 0784ea7..0cbd93e 100644
--- a/dbus/object_proxy.cc
+++ b/dbus/object_proxy.cc
@@ -281,14 +281,8 @@ void ObjectProxy::ConnectToSignalInternal(
     OnConnectedCallback on_connected_callback) {
   bus_->AssertOnDBusThread();
 
-  // Check if the object is already connected to the signal.
   const std::string absolute_signal_name =
       GetAbsoluteSignalName(interface_name, signal_name);
-  if (method_table_.find(absolute_signal_name) != method_table_.end()) {
-    LOG(ERROR) << "The object proxy is already connected to "
-               << absolute_signal_name;
-    return;
-  }
 
   // Will become true, if everything is successful.
   bool success = false;
diff --git a/dbus/object_proxy.h b/dbus/object_proxy.h
index 3a9fab1..6c786ab4 100644
--- a/dbus/object_proxy.h
+++ b/dbus/object_proxy.h
@@ -29,7 +29,9 @@ class Signal;
 // calling methods of these objects.
 //
 // ObjectProxy is a ref counted object, to ensure that |this| of the
-// object is is alive when callbacks referencing |this| are called.
+// object is is alive when callbacks referencing |this| are called; the
+// bus always holds at least one of those references so object proxies
+// always last as long as the bus that created them.
 class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
  public:
   // Client code should use Bus::GetObjectProxy() or
@@ -96,7 +98,8 @@ class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
                           int timeout_ms,
                           ResponseCallback callback);
 
-  // Requests to connect to the signal from the remote object.
+  // Requests to connect to the signal from the remote object, replacing
+  // any previous |signal_callback| connected to that signal.
   //
   // |signal_callback| will be called in the origin thread, when the
   // signal is received from the remote object. As it's called in the