Fix a bug in dbus::Bus::AddFilterFunction().

We should not reject the same function if it's associated with different data.

BUG=99258
TEST=adde a unit test and confirmed it passed.

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

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

4 files changed, 70 insertions, 20 deletions

diff --git a/dbus/bus.cc b/dbus/bus.cc
index 89918e0..87fe9c7 100644
--- a/dbus/bus.cc
+++ b/dbus/bus.cc
@@ -439,37 +439,45 @@ void Bus::Send(DBusMessage* request, uint32* serial) {
   CHECK(success) << "Unable to allocate memory";
 }
 
-void Bus::AddFilterFunction(DBusHandleMessageFunction filter_function,
+bool Bus::AddFilterFunction(DBusHandleMessageFunction filter_function,
                             void* user_data) {
   DCHECK(connection_);
   AssertOnDBusThread();
 
-  if (filter_functions_added_.find(filter_function) !=
+  std::pair<DBusHandleMessageFunction, void*> filter_data_pair =
+      std::make_pair(filter_function, user_data);
+  if (filter_functions_added_.find(filter_data_pair) !=
       filter_functions_added_.end()) {
-    LOG(ERROR) << "Filter function already exists: " << filter_function;
-    return;
+    VLOG(1) << "Filter function already exists: " << filter_function
+            << " with associated data: " << user_data;
+    return false;
   }
 
   const bool success = dbus_connection_add_filter(
       connection_, filter_function, user_data, NULL);
   CHECK(success) << "Unable to allocate memory";
-  filter_functions_added_.insert(filter_function);
+  filter_functions_added_.insert(filter_data_pair);
+  return true;
 }
 
-void Bus::RemoveFilterFunction(DBusHandleMessageFunction filter_function,
+bool Bus::RemoveFilterFunction(DBusHandleMessageFunction filter_function,
                                void* user_data) {
   DCHECK(connection_);
   AssertOnDBusThread();
 
-  if (filter_functions_added_.find(filter_function) ==
+  std::pair<DBusHandleMessageFunction, void*> filter_data_pair =
+      std::make_pair(filter_function, user_data);
+  if (filter_functions_added_.find(filter_data_pair) ==
       filter_functions_added_.end()) {
-    LOG(ERROR) << "Requested to remove an unknown filter function: "
-               << filter_function;
-    return;
+    VLOG(1) << "Requested to remove an unknown filter function: "
+            << filter_function
+            << " with associated data: " << user_data;
+    return false;
   }
 
   dbus_connection_remove_filter(connection_, filter_function, user_data);
-  filter_functions_added_.erase(filter_function);
+  filter_functions_added_.erase(filter_data_pair);
+  return true;
 }
 
 void Bus::AddMatch(const std::string& match_rule, DBusError* error) {
diff --git a/dbus/bus.h b/dbus/bus.h
index b64ae8f..3d64a08 100644
--- a/dbus/bus.h
+++ b/dbus/bus.h
@@ -9,6 +9,7 @@
 #include <map>
 #include <set>
 #include <string>
+#include <utility>
 #include <dbus/dbus.h>
 
 #include "base/callback.h"
@@ -284,22 +285,24 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   virtual void Send(DBusMessage* request, uint32* serial);
 
   // Adds the message filter function. |filter_function| will be called
-  // when incoming messages are received.
+  // when incoming messages are received. Returns true on success.
   //
   // When a new incoming message arrives, filter functions are called in
   // the order that they were added until the the incoming message is
   // handled by a filter function.
   //
-  // The same filter function must not be added more than once.
+  // The same filter function associated with the same user data cannot be
+  // added more than once. Returns false for this case.
   //
   // BLOCKING CALL.
-  virtual void AddFilterFunction(DBusHandleMessageFunction filter_function,
+  virtual bool AddFilterFunction(DBusHandleMessageFunction filter_function,
                                  void* user_data);
 
   // Removes the message filter previously added by AddFilterFunction().
+  // Returns true on success.
   //
   // BLOCKING CALL.
-  virtual void RemoveFilterFunction(DBusHandleMessageFunction filter_function,
+  virtual bool RemoveFilterFunction(DBusHandleMessageFunction filter_function,
                                     void* user_data);
 
   // Adds the match rule. Messages that match the rule will be processed
@@ -444,7 +447,8 @@ class Bus : public base::RefCountedThreadSafe<Bus> {
   // are properly cleaned up before destruction of the bus object.
   std::set<std::string> match_rules_added_;
   std::set<std::string> registered_object_paths_;
-  std::set<DBusHandleMessageFunction> filter_functions_added_;
+  std::set<std::pair<DBusHandleMessageFunction, void*> >
+      filter_functions_added_;
 
   // ObjectProxyTable is used to hold the object proxies created by the
   // bus object. Key is a concatenated string of service name + object path,
diff --git a/dbus/bus_unittest.cc b/dbus/bus_unittest.cc
index 6209bc3..999727a 100644
--- a/dbus/bus_unittest.cc
+++ b/dbus/bus_unittest.cc
@@ -13,6 +13,17 @@
 
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+
+// Used to test AddFilterFunction().
+DBusHandlerResult DummyHandler(DBusConnection* connection,
+                               DBusMessage* raw_message,
+                               void* user_data) {
+  return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+}
+
+}  // namespace
+
 TEST(BusTest, GetObjectProxy) {
   dbus::Bus::Options options;
   scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
@@ -89,3 +100,24 @@ TEST(BusTest, ShutdownAndBlockWithDBusThread) {
   EXPECT_TRUE(bus->shutdown_completed());
   dbus_thread.Stop();
 }
+
+TEST(BusTest, AddFilterFunction) {
+  dbus::Bus::Options options;
+  scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
+  // Should connect before calling AddFilterFunction().
+  bus->Connect();
+
+  int data1 = 100;
+  int data2 = 200;
+  ASSERT_TRUE(bus->AddFilterFunction(&DummyHandler, &data1));
+  // Cannot add the same function with the same data.
+  ASSERT_FALSE(bus->AddFilterFunction(&DummyHandler, &data1));
+  // Can add the same function with different data.
+  ASSERT_TRUE(bus->AddFilterFunction(&DummyHandler, &data2));
+
+  ASSERT_TRUE(bus->RemoveFilterFunction(&DummyHandler, &data1));
+  ASSERT_FALSE(bus->RemoveFilterFunction(&DummyHandler, &data1));
+  ASSERT_TRUE(bus->RemoveFilterFunction(&DummyHandler, &data2));
+
+  bus->ShutdownAndBlock();
+}
diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc
index 7d24c7e..f4e4ac0 100644
--- a/dbus/object_proxy.cc
+++ b/dbus/object_proxy.cc
@@ -124,8 +124,11 @@ void ObjectProxy::ConnectToSignal(const std::string& interface_name,
 void ObjectProxy::Detach() {
   bus_->AssertOnDBusThread();
 
-  if (filter_added_)
-    bus_->RemoveFilterFunction(&ObjectProxy::HandleMessageThunk, this);
+  if (filter_added_) {
+    if (!bus_->RemoveFilterFunction(&ObjectProxy::HandleMessageThunk, this)) {
+      LOG(ERROR) << "Failed to remove filter function";
+    }
+  }
 
   for (size_t i = 0; i < match_rules_.size(); ++i) {
     ScopedDBusError error;
@@ -277,8 +280,11 @@ void ObjectProxy::ConnectToSignalInternal(
     // We should add the filter only once. Otherwise, HandleMessage() will
     // be called more than once.
     if (!filter_added_) {
-      bus_->AddFilterFunction(&ObjectProxy::HandleMessageThunk, this);
-      filter_added_ = true;
+      if (bus_->AddFilterFunction(&ObjectProxy::HandleMessageThunk, this)) {
+        filter_added_ = true;
+      } else {
+        LOG(ERROR) << "Failed to add filter function";
+      }
     }
     // Add a match rule so the signal goes through HandleMessage().
     //