dbus::ObjectManager: Add a match rule for properties before GetManagedObjects.

dbus/object_manager.cc

Index: dbus/object_manager.cc
diff --git a/dbus/object_manager.cc b/dbus/object_manager.cc
index d8eb569cf726ca61b114e95cb40b6115eddf2fff..75822f677467332e1e09e62f5881857f359e4181 100644
--- a/dbus/object_manager.cc
+++ b/dbus/object_manager.cc
@@ -5,14 +5,32 @@
 #include "dbus/object_manager.h"
 
 #include "base/bind.h"
+#include "base/location.h"
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/strings/stringprintf.h"
+#include "base/task_runner_util.h"
 #include "dbus/bus.h"
+#include "dbus/dbus_statistics.h"
 #include "dbus/message.h"
 #include "dbus/object_proxy.h"
 #include "dbus/property.h"
+#include "dbus/scoped_dbus_error.h"
 
 namespace dbus {
 
+namespace {
+
+// Gets the absolute signal name by concatenating the interface name and
+// the signal name.
+std::string GetAbsoluteSignalName(
+    const std::string& interface_name,
+    const std::string& signal_name) {
+  return interface_name + "." + signal_name;
+}

[satorux1, 2014/08/28 04:00:01]

this is anotner one in object_proxy.cc. I think it's time to create util.h/cc
that contains these functions.

[armansito, 2014/08/28 04:30:49]

Ack. I'll add a TODO for this and do it in another CL. This should probably be
an inline function then. In fact, I only use it in one place in this file, so I
can just do the concatenation in place and remove this function entirely.

[satorux1, 2014/08/29 05:20:53]

It's fine to do it in a separate patch, but that cleanup is trivial enough to be
included in this patch. It's up to you.

[armansito, 2014/08/29 16:17:08]

Done.

+
+}  // namespace
+
 ObjectManager::Object::Object()
   : object_proxy(NULL) {
 }
@@ -26,33 +44,26 @@ ObjectManager::ObjectManager(Bus* bus,
     : bus_(bus),
       service_name_(service_name),
       object_path_(object_path),
+      setup_success_(false),
       weak_ptr_factory_(this) {
   DVLOG(1) << "Creating ObjectManager for " << service_name_
            << " " << object_path_.value();
-
   DCHECK(bus_);
+  bus_->AssertOnOriginThread();
   object_proxy_ = bus_->GetObjectProxy(service_name_, object_path_);
   object_proxy_->SetNameOwnerChangedCallback(
       base::Bind(&ObjectManager::NameOwnerChanged,
                  weak_ptr_factory_.GetWeakPtr()));
 
-  object_proxy_->ConnectToSignal(
-      kObjectManagerInterface,
-      kObjectManagerInterfacesAdded,
-      base::Bind(&ObjectManager::InterfacesAddedReceived,
-                 weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&ObjectManager::InterfacesAddedConnected,
-                 weak_ptr_factory_.GetWeakPtr()));
-
-  object_proxy_->ConnectToSignal(
-      kObjectManagerInterface,
-      kObjectManagerInterfacesRemoved,
-      base::Bind(&ObjectManager::InterfacesRemovedReceived,
-                 weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&ObjectManager::InterfacesRemovedConnected,
-                 weak_ptr_factory_.GetWeakPtr()));
-
-  GetManagedObjects();
+  // Set up a match rule and a filter function to handle PropertiesChanged
+  // signals from the service. This is important to avoid any race conditions
+  // that might cause us to miss PropertiesChanged signals once all objects are
+  // initialized via GetManagedObjects.
+  base::PostTaskAndReplyWithResult(
+      bus_->GetDBusTaskRunner(),
+      FROM_HERE,
+      base::Bind(&ObjectManager::SetupMatchRuleAndFilter, this),
+      base::Bind(&ObjectManager::OnSetupMatchRuleAndFilterComplete, this));
 }
 
 ObjectManager::~ObjectManager() {
@@ -144,6 +155,188 @@ void ObjectManager::GetManagedObjects() {
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
+void ObjectManager::CleanUp() {
+  DCHECK(bus_);
+  bus_->AssertOnDBusThread();
+
+  if (!setup_success_)
+    return;
+
+  if (!bus_->RemoveFilterFunction(&ObjectManager::HandleMessageThunk, this))
+    LOG(ERROR) << "Failed to remove filter function";
+
+  ScopedDBusError error;
+  bus_->RemoveMatch(match_rule_, error.get());
+  if (error.is_set())
+    LOG(ERROR) << "Failed to remove match rule: " << match_rule_;
+
+  match_rule_.clear();
+}
+
+void ObjectManager::InitializeObjects() {
+  DCHECK(bus_);
+  object_proxy_->ConnectToSignal(
+      kObjectManagerInterface,
+      kObjectManagerInterfacesAdded,
+      base::Bind(&ObjectManager::InterfacesAddedReceived,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&ObjectManager::InterfacesAddedConnected,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  object_proxy_->ConnectToSignal(
+      kObjectManagerInterface,
+      kObjectManagerInterfacesRemoved,
+      base::Bind(&ObjectManager::InterfacesRemovedReceived,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&ObjectManager::InterfacesRemovedConnected,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  GetManagedObjects();
+}
+
+bool ObjectManager::SetupMatchRuleAndFilter() {
+  DCHECK(bus_);
+  DCHECK(!setup_success_);
+  bus_->AssertOnDBusThread();
+
+  if (!bus_->Connect() || !bus_->SetUpAsyncOperations())
+    return false;
+
+  service_name_owner_ =
+      bus_->GetServiceOwnerAndBlock(service_name_, Bus::SUPPRESS_ERRORS);
+
+  const std::string match_rule =
+      base::StringPrintf(
+          "type='signal', sender='%s', interface='%s', member='%s'",
+          service_name_.c_str(),
+          kPropertiesInterface,
+          kPropertiesChanged);
+
+  if (!bus_->AddFilterFunction(&ObjectManager::HandleMessageThunk, this)) {
+    LOG(ERROR) << "ObjectManager failed to add filter function";
+    return false;
+  }
+
+  ScopedDBusError error;
+  bus_->AddMatch(match_rule, error.get());
+  if (error.is_set()) {
+    LOG(ERROR) << "ObjectManager failed to add match rule \"" << match_rule
+               << "\". Got " << error.name() << ": " << error.message();
+    bus_->RemoveFilterFunction(&ObjectManager::HandleMessageThunk, this);
+    return false;
+  }
+
+  match_rule_ = match_rule;
+  setup_success_ = true;
+
+  return true;
+}
+
+void ObjectManager::OnSetupMatchRuleAndFilterComplete(bool success) {
+  LOG_IF(WARNING, !success) << service_name_ << " " << object_path_.value()
+                            << ": Failed to set up match rule.";
+  if (success)
+    InitializeObjects();
+}
+
+// static
+DBusHandlerResult ObjectManager::HandleMessageThunk(DBusConnection* connection,
+                                                    DBusMessage* raw_message,
+                                                    void* user_data) {
+  ObjectManager* self = reinterpret_cast<ObjectManager*>(user_data);
+  return self->HandleMessage(connection, raw_message);
+}
+
+DBusHandlerResult ObjectManager::HandleMessage(DBusConnection* connection,
+                                               DBusMessage* raw_message) {
+  DCHECK(bus_);
+  bus_->AssertOnDBusThread();
+
+  if (dbus_message_get_type(raw_message) != DBUS_MESSAGE_TYPE_SIGNAL)
+    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+
+  // raw_message will be unrefed on exit of the function. Increment the
+  // reference so we can use it in Signal.
+  dbus_message_ref(raw_message);
+  scoped_ptr<Signal> signal(
+      Signal::FromRawMessage(raw_message));
+
+  const std::string interface = signal->GetInterface();
+  const std::string member = signal->GetMember();
+
+  statistics::AddReceivedSignal(service_name_, interface, member);
+
+  // Only handle the PropertiesChanged signal.
+  const std::string absolute_signal_name =
+      GetAbsoluteSignalName(interface, member);
+  const std::string properties_changed_signal_name =
+      GetAbsoluteSignalName(kPropertiesInterface, kPropertiesChanged);
+  if (absolute_signal_name != properties_changed_signal_name)
+    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+
+  VLOG(1) << "Signal received: " << signal->ToString();
+
+  // Make sure that the signal originated from the correct sender.
+  std::string sender = signal->GetSender();
+  if (service_name_owner_ != sender) {
+    LOG(ERROR) << "Rejecting a message from a wrong sender.";
+    UMA_HISTOGRAM_COUNTS("DBus.RejectedSignalCount", 1);
+    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+  }
+
+  const ObjectPath path = signal->GetPath();
+
+  if (bus_->HasDBusThread()) {
+    // Post a task to run the method in the origin thread. Transfer ownership of
+    // |signal| to NotifyPropertiesChanged, which will handle the clean up.
+    Signal* released_signal = signal.release();
+    bus_->GetOriginTaskRunner()->PostTask(
+        FROM_HERE,
+        base::Bind(&ObjectManager::NotifyPropertiesChanged,
+                   this, path,
+                   released_signal));
+  } else {
+    // If the D-Bus thread is not used, just call the callback on the
+    // current thread. Transfer the ownership of |signal| to
+    // NotifyPropertiesChanged.
+    NotifyPropertiesChanged(path, signal.release());
+  }
+
+  // We don't return DBUS_HANDLER_RESULT_HANDLED for signals because other
+  // objects may be interested in them. (e.g. Signals from org.freedesktop.DBus)
+  return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+}
+
+void ObjectManager::NotifyPropertiesChanged(
+    const dbus::ObjectPath object_path,
+    Signal* signal) {

[satorux1, 2014/08/28 04:00:01]

add   bus_->AssertOnOriginThread(); to be consistent with other functions.

[armansito, 2014/08/28 15:37:59]

Done.

+  NotifyPropertiesChangedHelper(object_path, signal);
+
+  // Delete the message on the D-Bus thread. See comments in HandleMessage.
+  bus_->GetDBusTaskRunner()->PostTask(
+      FROM_HERE,
+      base::Bind(&base::DeletePointer<Signal>, signal));
+}
+
+void ObjectManager::NotifyPropertiesChangedHelper(
+    const dbus::ObjectPath object_path,
+    Signal* signal) {
+  DCHECK(bus_);
+  bus_->AssertOnOriginThread();

[satorux1, 2014/08/29 05:20:53]

let's keep them. a bit verbose, but good to have them for clarity.

[armansito, 2014/08/29 16:17:08]

Done.

+
+  MessageReader reader(signal);
+  std::string interface;
+  if (!reader.PopString(&interface)) {
+    LOG(WARNING) << "Property changed signal has wrong parameters: "
+                 << "expected interface name: " << signal->ToString();
+    return;
+  }
+
+  PropertySet* properties = GetProperties(object_path, interface);
+  if (properties)
+    properties->ChangedReceived(signal);
+}
+
 void ObjectManager::OnGetManagedObjects(Response* response) {
   if (response != NULL) {
     MessageReader reader(response);
@@ -257,7 +450,6 @@ void ObjectManager::AddInterface(const ObjectPath& object_path,
     property_set = object->properties_map[interface_name] =
         interface->CreateProperties(object->object_proxy,
                                     object_path, interface_name);
-    property_set->ConnectSignals();
   } else
     property_set = piter->second;
 
@@ -297,6 +489,8 @@ void ObjectManager::RemoveInterface(const ObjectPath& object_path,
 
 void ObjectManager::NameOwnerChanged(const std::string& old_owner,
                                      const std::string& new_owner) {
+  service_name_owner_ = new_owner;
+
   if (!old_owner.empty()) {
     ObjectMap::iterator iter = object_map_.begin();
     while (iter != object_map_.end()) {