dbus: verify object path of incoming signals

The existing behavior, while convenient for debugging, is wrong.

D-Bus will not call any further filter functions once one returns
DBUS_HANDLER_RESULT_HANDLED, in order for the next to be called a
filter must return DBUS_HANDLER_RESULT_NOT_YET_HANDLED if it does
not handle the incoming signal.

We also can't defer this to the signal function since we have to
post that to a different thread, and return values get hard.

Since object proxies are constructed per-path, and match common
interfaces and members, this means signals must be matched on an
object otherwise only the first registered object proxy for any
client will be called, and will be called for all signals.

BUG=chromium-os:27113
TEST=ran unit tests, and manually verified existing code that uses ConnectToSignal

Change-Id: Ia4cbc064dff0421a37fe4c4b7c719acf25eb630c


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

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

2 files changed, 49 insertions, 12 deletions

diff --git a/dbus/end_to_end_async_unittest.cc b/dbus/end_to_end_async_unittest.cc
index 9a88b42..302b7b4 100644
--- a/dbus/end_to_end_async_unittest.cc
+++ b/dbus/end_to_end_async_unittest.cc
@@ -83,6 +83,24 @@ class EndToEndAsyncTest : public testing::Test {
                    base::Unretained(this)));
     // Wait until the object proxy is connected to the signal.
     message_loop_.Run();
+
+    // Create a second object proxy for the root object.
+    root_object_proxy_ = bus_->GetObjectProxy(
+        "org.chromium.TestService",
+        dbus::ObjectPath("/"));
+    ASSERT_TRUE(bus_->HasDBusThread());
+
+    // Connect to the "Test" signal of "org.chromium.TestInterface" from
+    // the root remote object too.
+    root_object_proxy_->ConnectToSignal(
+        "org.chromium.TestInterface",
+        "Test",
+        base::Bind(&EndToEndAsyncTest::OnRootTestSignal,
+                   base::Unretained(this)),
+        base::Bind(&EndToEndAsyncTest::OnConnected,
+                   base::Unretained(this)));
+    // Wait until the root object proxy is connected to the signal.
+    message_loop_.Run();
   }
 
   virtual void TearDown() {
@@ -140,6 +158,15 @@ class EndToEndAsyncTest : public testing::Test {
     message_loop_.Quit();
   }
 
+  // Called when the "Test" signal is received, in the main thread, by
+  // the root object proxy. Copy the string payload to
+  // |root_test_signal_string_|.
+  void OnRootTestSignal(dbus::Signal* signal) {
+    dbus::MessageReader reader(signal);
+    ASSERT_TRUE(reader.PopString(&root_test_signal_string_));
+    message_loop_.Quit();
+  }
+
   // Called when the "Test2" signal is received, in the main thread.
   void OnTest2Signal(dbus::Signal* signal) {
     dbus::MessageReader reader(signal);
@@ -165,9 +192,12 @@ class EndToEndAsyncTest : public testing::Test {
   scoped_ptr<base::Thread> dbus_thread_;
   scoped_refptr<dbus::Bus> bus_;
   dbus::ObjectProxy* object_proxy_;
+  dbus::ObjectProxy* root_object_proxy_;
   scoped_ptr<dbus::TestService> test_service_;
   // Text message from "Test" signal.
   std::string test_signal_string_;
+  // Text message from "Test" signal delivered to root.
+  std::string root_test_signal_string_;
 };
 
 TEST_F(EndToEndAsyncTest, Echo) {
@@ -302,11 +332,14 @@ TEST_F(EndToEndAsyncTest, TestSignal) {
 
 TEST_F(EndToEndAsyncTest, TestSignalFromRoot) {
   const char kMessage[] = "hello, world";
-  // Send the test signal from the root object path, to see if we can
-  // handle signals sent from "/", like dbus-send does.
+  // Object proxies are tied to a particular object path, if a signal
+  // arrives from a different object path like "/" the first object proxy
+  // |object_proxy_| should not handle it, and should leave it for the root
+  // object proxy |root_object_proxy_|.
   test_service_->SendTestSignalFromRoot(kMessage);
-  // Receive the signal with the object proxy. The signal is handled in
-  // EndToEndAsyncTest::OnTestSignal() in the main thread.
   WaitForTestSignal();
-  ASSERT_EQ(kMessage, test_signal_string_);
+  // Verify the signal was not received by the specific proxy.
+  ASSERT_TRUE(test_signal_string_.empty());
+  // Verify the string WAS received by the root proxy.
+  ASSERT_EQ(kMessage, root_test_signal_string_);
 }
diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc
index 32f6a60..0784ea7 100644
--- a/dbus/object_proxy.cc
+++ b/dbus/object_proxy.cc
@@ -304,14 +304,10 @@ void ObjectProxy::ConnectToSignalInternal(
       }
     }
     // Add a match rule so the signal goes through HandleMessage().
-    //
-    // We don't restrict the sender object path to be |object_path_| here,
-    // to make it easy to test D-Bus signal handling with dbus-send, that
-    // uses "/" as the sender object path. We can make the object path
-    // restriction customizable when it becomes necessary.
     const std::string match_rule =
-        base::StringPrintf("type='signal', interface='%s'",
-                           interface_name.c_str());
+        base::StringPrintf("type='signal', interface='%s', path='%s'",
+                           interface_name.c_str(),
+                           object_path_.value().c_str());
 
     // Add the match rule if we don't have it.
     if (match_rules_.find(match_rule) == match_rules_.end()) {
@@ -366,6 +362,14 @@ DBusHandlerResult ObjectProxy::HandleMessage(
   scoped_ptr<Signal> signal(
       Signal::FromRawMessage(raw_message));
 
+  // Verify the signal comes from the object we're proxying for, this is
+  // our last chance to return DBUS_HANDLER_RESULT_NOT_YET_HANDLED and
+  // allow other object proxies to handle instead.
+  const dbus::ObjectPath path = signal->GetPath();
+  if (path != object_path_) {
+    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+  }
+
   const std::string interface = signal->GetInterface();
   const std::string member = signal->GetMember();