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
CrOS-Libchrome-Original-Commit: 12e2599805235b139796ffee9598e8c95768b21a
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 @@
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 @@
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 @@
// 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 @@
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::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 @@
// 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().
//