cashew: defer all D-Bus signal processing to main loop

src/service_manager.cc

 // Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/service_manager.h"
 
 #include <glog/logging.h>
 
 #include "src/cashew_server.h"
 #include "src/service.h"
@@ skipping 15 matching lines @@
 static const char *kFlimflamStateOnline = "online";
 
 // Flimflam service types
 static const char *kTypeCellular = "cellular";
 
 // GetFlimflamProperties retry interval
 static const guint kSecondsPerMinute = 60;
 static const guint kGetFlimflamPropertiesIntervalSeconds =
     1 * kSecondsPerMinute;
 
-ServiceManager::ServiceManager(DBus::Connection& connection)  // NOLINT
+ServiceManager::ServiceManager(DBus::Connection& connection,  // NOLINT
+                               GMainLoop * const main_loop)
     : DBus::ObjectProxy(connection, kFlimflamManagerPath,
                         kFlimflamManagerName),
-      connection_(connection), cashew_server_(NULL),
-      default_technology_(Service::kTypeUnknown),
+      connection_(connection), main_loop_(CHECK_NOTNULL(main_loop)),
+      cashew_server_(NULL), default_technology_(Service::kTypeUnknown),
       default_cellular_service_(NULL),
       connectivity_state_(kConnectivityStateUnknown),
       get_properties_source_id_(0), retrying_get_properties_(false) {
   // schedule a GetProperties() call to Flimflam to init our state
   // we'll keep trying periodically until we succeed
   // we'll subsequently update this state by monitoring PropertyChanged signals
   get_properties_source_id_ =
       g_idle_add(StaticGetFlimflamPropertiesCallback, this);
   if (get_properties_source_id_ == 0) {
     LOG(ERROR) << "ctor: g_idle_add failed";
   }
+  property_changed_handler_.delegate(this);
 }
 
 ServiceManager::~ServiceManager() {
+  DCHECK(!g_main_loop_is_running(main_loop_));
   ClearDefaultCellularService();
   DeleteServices(&services_);
   if (get_properties_source_id_ != 0 &&
       !g_source_remove(get_properties_source_id_)) {
     DLOG(WARNING) << "dtor: g_source_remove failed";
   }
 }
 
 const Service* ServiceManager::GetService(const std::string& service_path)
     const {
@@ skipping 37 matching lines @@
     return true;
   }
   return false;
 }
 
 // Flimflam Manager D-Bus Proxy methods
 
 void ServiceManager::PropertyChanged(const std::string& property_name,
                                      const DBus::Variant& new_value) {
   DLOG(INFO) << "PropertyChanged: property_name = " << property_name;
+  // Queue a tuple representing this signal for later processing from the glib
+  // main loop.  We do this to avoid libdbus-c++ deadlocks that can occur when
+  // sending a dbus message from within a dbus callback like this one.
+  PropertyChangedSignal signal(property_name, new_value);
+  property_changed_handler_.EnqueueSignal(signal);
+}
+
+void ServiceManager::StateChanged(const std::string& new_state_string) {
+  DLOG(INFO) << "StateChanged: new_state_string = " << new_state_string;
+  // Queue a tuple representing this signal for later processing from the glib
+  // main loop. See comments in PropertyChanged above.
+  DBus::Variant new_value;
+  new_value.writer().append_string(new_state_string.c_str());
+  // StateChanged is a special case of PropertyChanged, so we just construct a
+  // PropertyChanged signal containing the new state info
+  PropertyChangedSignal signal(kStateProperty, new_value);
+  property_changed_handler_.EnqueueSignal(signal);
+}
+
+// PropertyChangedDelegate methods
+
+void ServiceManager::OnPropertyChanged(const PropertyChangedHandler *handler,
+                                       const std::string& property_name,
+                                       const DBus::Variant& new_value) {
+  DCHECK(handler == &property_changed_handler_);
+  DLOG(INFO) << "OnPropertyChanged: property_name = " << property_name;
   if (property_name == kDefaultTechnologyProperty) {
     OnDefaultTechnologyUpdate(new_value.reader().get_string());
   } else if (property_name == kServicesProperty) {
     // interpret new_value as a vector of service paths
     ServicePathList paths;
     DBus::MessageIter reader = new_value.reader();
     reader >> paths;
     OnServicesUpdate(paths);
   } else if (property_name == kStateProperty) {
-    // this has its own signal, StateChanged, but just in case...
-    StateChanged(new_value.reader().get_string());
+    OnStateUpdate(new_value.reader().get_string());
   } else {
     // we don't care about this property
   }
 }
 
-void ServiceManager::StateChanged(const std::string& new_state_string) {
-  DLOG(INFO) << "StateChanged: new_state = " << new_state_string;
-  ConnectivityState old_state = connectivity_state_;
-  connectivity_state_ = ConnectivityStateFromString(new_state_string);
-  // see if we're coming online or going offline
-  // NOTE: we consider "unknown" to be offline
-  if (IsOfflineConnectivityState(old_state) &&
-      IsOnlineConnectivityState(connectivity_state_)) {
-    OnFlimflamOnline();
-  } else if (IsOnlineConnectivityState(old_state) &&
-      IsOfflineConnectivityState(connectivity_state_)) {
-    OnFlimflamOffline();
-  }
-}
-
 // Service methods
 
 void ServiceManager::EmitDataPlansUpdate(const Service& service) {
   DLOG(INFO) << "EmitDataPlansUpdate: service = " << service.GetPath();
   if (cashew_server_ == NULL) {
     DLOG(WARNING) << "EmitDataPlansUpdate: no Cashew server";
     return;
   }
   cashew_server_->EmitDataPlansUpdate(service);
 }
@@ skipping 110 matching lines @@
   it = properties.find(kStateProperty);
   if (it != properties.end()) {
     const DBus::Variant& value = static_cast<DBus::Variant>(it->second);
     StateChanged(value.reader().get_string());
   } else {
     DLOG(WARNING) << "GetFlimflamProperties: no State property";
   }
   return true;
 }
 
+void ServiceManager::OnStateUpdate(const std::string& new_state_string) {
+  DLOG(INFO) << "OnStateUpdate: new_state = " << new_state_string;
+  ConnectivityState old_state = connectivity_state_;
+  connectivity_state_ = ConnectivityStateFromString(new_state_string);
+  // see if we're coming online or going offline
+  // NOTE: we consider "unknown" to be offline
+  if (IsOfflineConnectivityState(old_state) &&
+      IsOnlineConnectivityState(connectivity_state_)) {
+    OnFlimflamOnline();
+  } else if (IsOnlineConnectivityState(old_state) &&
+      IsOfflineConnectivityState(connectivity_state_)) {
+    OnFlimflamOffline();
+  }
+}
+
 void ServiceManager::OnDefaultTechnologyUpdate(
     const std::string& default_technology) {
   DLOG(INFO) << "OnDefaultTechnologyUpdate: default technology = "
       << default_technology;
   default_technology_ = Service::TypeFromString(default_technology);
   if (default_technology_ == Service::kTypeUnknown) {
     LOG(WARNING) << "OnDefaultTechnologyUpdate: unknown default technology: "
         << default_technology;
   }
   if (default_technology_ != Service::kTypeCellular &&
@@ skipping 148 matching lines @@
   // notify our child services
   ServiceMap::iterator it;
   for (it = services_.begin(); it != services_.end(); ++it) {
     Service *service = static_cast<Service*>(it->second);
     DCHECK(service != NULL);
     service->OnFlimflamOffline();
   }
 }
 
 }  // namespace cashew