From 0ad9ef8e4c41b13a5f889a485677f225d7e5a934 Mon Sep 17 00:00:00 2001
From: "satorux@chromium.org"
 <satorux@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Wed, 23 Nov 2011 22:08:38 +0000
Subject: dbus: Fix a bug where we were emitting spurious error messages.

Error messages like "Requested to remove an unknown match rule:
type='signal', interface='...'" were emitted at shutdown of Bus
object if an object proxy was connected to more than one signal of
the same interface.

TEST=changed the end_to_end_async_unittest to reproduce this bug, and confirm that the error messages were gone after fixing object_proxy.{cc,h}
BUG=chromium-os:23382

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@111423 0039d316-1c4b-4281-b951-d872f2087c98
---
 dbus/end_to_end_async_unittest.cc | 21 ++++++++++++++++++++-
 dbus/object_proxy.cc              | 30 ++++++++++++++++++++----------
 dbus/object_proxy.h               |  4 ++--
 3 files changed, 42 insertions(+), 13 deletions(-)

(limited to 'dbus')

diff --git a/dbus/end_to_end_async_unittest.cc b/dbus/end_to_end_async_unittest.cc
index 7ba782b..2f486f4 100644
--- a/dbus/end_to_end_async_unittest.cc
+++ b/dbus/end_to_end_async_unittest.cc
@@ -55,7 +55,8 @@ class EndToEndAsyncTest : public testing::Test {
                                          "/org/chromium/TestObject");
     ASSERT_TRUE(bus_->HasDBusThread());
 
-    // Connect to the "Test" signal from the remote object.
+    // Connect to the "Test" signal of "org.chromium.TestInterface" from
+    // the remote object.
     object_proxy_->ConnectToSignal(
         "org.chromium.TestInterface",
         "Test",
@@ -63,6 +64,18 @@ class EndToEndAsyncTest : public testing::Test {
                    base::Unretained(this)),
         base::Bind(&EndToEndAsyncTest::OnConnected,
                    base::Unretained(this)));
+    // Connect to the "Test2" signal of "org.chromium.TestInterface" from
+    // the remote object. There was a bug where we were emitting error
+    // messages like "Requested to remove an unknown match rule: ..." at
+    // the shutdown of Bus when an object proxy is connected to more than
+    // one signal of the same interface. See crosbug.com/23382 for details.
+    object_proxy_->ConnectToSignal(
+        "org.chromium.TestInterface",
+        "Test2",
+        base::Bind(&EndToEndAsyncTest::OnTest2Signal,
+                   base::Unretained(this)),
+        base::Bind(&EndToEndAsyncTest::OnConnected,
+                   base::Unretained(this)));
     // Wait until the object proxy is connected to the signal.
     message_loop_.Run();
   }
@@ -122,6 +135,12 @@ class EndToEndAsyncTest : public testing::Test {
     message_loop_.Quit();
   }
 
+  // Called when the "Test2" signal is received, in the main thread.
+  void OnTest2Signal(dbus::Signal* signal) {
+    dbus::MessageReader reader(signal);
+    message_loop_.Quit();
+  }
+
   // Called when connected to the signal.
   void OnConnected(const std::string& interface_name,
                    const std::string& signal_name,
diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc
index d9a53d9..5da5901 100644
--- a/dbus/object_proxy.cc
+++ b/dbus/object_proxy.cc
@@ -134,14 +134,16 @@ void ObjectProxy::Detach() {
     }
   }
 
-  for (size_t i = 0; i < match_rules_.size(); ++i) {
+  for (std::set<std::string>::iterator iter = match_rules_.begin();
+       iter != match_rules_.end(); ++iter) {
     ScopedDBusError error;
-    bus_->RemoveMatch(match_rules_[i], error.get());
+    bus_->RemoveMatch(*iter, error.get());
     if (error.is_set()) {
       // There is nothing we can do to recover, so just print the error.
-      LOG(ERROR) << "Failed to remove match rule: " << match_rules_[i];
+      LOG(ERROR) << "Failed to remove match rule: " << *iter;
     }
   }
+  match_rules_.clear();
 }
 
 // static
@@ -304,14 +306,22 @@ void ObjectProxy::ConnectToSignalInternal(
     const std::string match_rule =
         base::StringPrintf("type='signal', interface='%s'",
                            interface_name.c_str());
-    ScopedDBusError error;
-    bus_->AddMatch(match_rule, error.get());;
-    if (error.is_set()) {
-      LOG(ERROR) << "Failed to add match rule: " << match_rule;
+
+    // Add the match rule if we don't have it.
+    if (match_rules_.find(match_rule) == match_rules_.end()) {
+      ScopedDBusError error;
+      bus_->AddMatch(match_rule, error.get());;
+      if (error.is_set()) {
+        LOG(ERROR) << "Failed to add match rule: " << match_rule;
+      } else {
+        // Store the match rule, so that we can remove this in Detach().
+        match_rules_.insert(match_rule);
+        // Add the signal callback to the method table.
+        method_table_[absolute_signal_name] = signal_callback;
+        success = true;
+      }
     } else {
-      // Store the match rule, so that we can remove this in Detach().
-      match_rules_.push_back(match_rule);
-      // Add the signal callback to the method table.
+      // We already have the match rule.
       method_table_[absolute_signal_name] = signal_callback;
       success = true;
     }
diff --git a/dbus/object_proxy.h b/dbus/object_proxy.h
index dcba44d..ef45305 100644
--- a/dbus/object_proxy.h
+++ b/dbus/object_proxy.h
@@ -9,8 +9,8 @@
 #include <dbus/dbus.h>
 
 #include <map>
+#include <set>
 #include <string>
-#include <vector>
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
@@ -192,7 +192,7 @@ class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
   typedef std::map<std::string, SignalCallback> MethodTable;
   MethodTable method_table_;
 
-  std::vector<std::string> match_rules_;
+  std::set<std::string> match_rules_;
 
   DISALLOW_COPY_AND_ASSIGN(ObjectProxy);
 };
-- 
cgit v1.1