cashew: do not delete DBus::ObjectProxy objects from D-Bus callbacks

src/service_manager.cc

Index: src/service_manager.cc
diff --git a/src/service_manager.cc b/src/service_manager.cc
index 484f08c98525802d128f3fe7424eefe1f5a7e452..0673e6aa4cd85afda8d5207f7e6757585fcc9686 100644
--- a/src/service_manager.cc
+++ b/src/service_manager.cc
@@ -33,14 +33,16 @@ 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) {
+      get_properties_source_id_(0), retrying_get_properties_(false),
+      deferred_service_deletion_source_id_(0) {
   // 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
@@ -52,11 +54,19 @@ ServiceManager::ServiceManager(DBus::Connection& connection)  // NOLINT
 }
 
 ServiceManager::~ServiceManager() {
+  DCHECK(!g_main_loop_is_running(main_loop_));
   ClearDefaultCellularService();
-  DeleteServices(&services_);
+  DeleteServicesWhenPossible(&services_);
+  DeletePendingServices();
   if (get_properties_source_id_ != 0 &&
       !g_source_remove(get_properties_source_id_)) {
-    DLOG(WARNING) << "dtor: g_source_remove failed";
+    DLOG(WARNING) << "dtor: g_source_remove failed for source id "
+        << get_properties_source_id_;
+  }
+  if (deferred_service_deletion_source_id_ != 0 &&
+      !g_source_remove(deferred_service_deletion_source_id_)) {
+    DLOG(WARNING) << "dtor: g_source_remove failed for source id "
+        << deferred_service_deletion_source_id_;
   }
 }
 
@@ -169,23 +179,98 @@ void ServiceManager::OnRetryingGetProperties(bool retrying) {
   retrying_get_properties_ = retrying;
 }
 
+guint ServiceManager::GetDeferredServiceDeletionSourceId() const {
+  return deferred_service_deletion_source_id_;
+}
+
+void ServiceManager::SetDeferredServiceDeletionSourceId(guint source_id) {
+  deferred_service_deletion_source_id_ = source_id;
+}
+
 // Private methods
 
-void ServiceManager::DeleteServices(ServiceMap *service_map) {
+void ServiceManager::DeleteServicesWhenPossible(ServiceMap *service_map) {
   DCHECK(service_map != NULL);
   while (!service_map->empty()) {
     const std::string& path =
         static_cast<std::string>(service_map->begin()->first);
-    DLOG(INFO) << "DeleteServices: deleting service: " << path;
+    DLOG(INFO) << "DeleteServicesWhenPossible: considering deleting service: "
+        << path;
     Service *service = static_cast<Service *>(service_map->begin()->second);
+    DCHECK(*service_map == services_ || GetService(service->GetPath()) == NULL);
+    DeleteServiceOrScheduleForLaterDeletion(service);
+    service_map->erase(service_map->begin());
+  }
+}
+
+void ServiceManager::DeletePendingServices() {

[Daniel Kurtz, 2010/11/24 02:33:47]

Why not just services_pending_deletion_.clear() ?

All the elements in the container are dropped: their destructors are called, and
then they are removed from the container, leaving it with a size of 0.

[Vince Laviano, 2010/11/24 04:48:34]

The elements are pointers to Services. This suggestion would work if the
elements were Services, but destructing the pointers is not the same as
destructing the Services to which they point.

+  while (!services_pending_deletion_.empty()) {
+    Service *service = *(services_pending_deletion_.begin());
     DCHECK(service != NULL);
+    DLOG(INFO) << "DeletePendingServices: deleting service: "
+        << service->GetPath();
     DCHECK(service != default_cellular_service_);
-    DCHECK(*service_map == services_ || GetService(service->GetPath()) == NULL);
     delete service;
-    service_map->erase(service_map->begin());
+    services_pending_deletion_.erase(services_pending_deletion_.begin());
   }
 }
 
+void ServiceManager::DeleteServiceOrScheduleForLaterDeletion(Service *service) {
+    DCHECK(service != NULL);
+    DCHECK(service != default_cellular_service_);
+    // if the main loop is not running, then we can delete the service
+    // immediately without fear of sending a D-Bus msg from a D-Bus callback (by
+    // virtue of calling the DBus::ObjectProxy dtor) and triggering a dbus-c++
+    // deadlock.
+    //
+    // TODO(vlaviano): There is a race here if we receive a signal and call
+    // g_main_loop_quit() from the signal handler. This may not be much of an
+    // issue since we are likely receiving the signal from upstart and it will
+    // proceed to kill -9 us if we become unresponsive and fail to exit. If we
+    // care, we could have the sighandler g_idle_add a callback that quits the
+    // main loop instead of quitting it directly.
+    if (!g_main_loop_is_running(main_loop_)) {

[Daniel Kurtz, 2010/11/24 02:33:47]

Break this into two methods:
1) always defers, and can be called while main_loop is running.
2) a pure deconstructor that only gets called after main_loop is terminated.

[Vince Laviano, 2010/11/24 04:48:34]

Done. (Good call.)

+      DLOG(INFO)
+          << "DeleteServiceOrScheduleForLaterDeletion: deleting service: "
+          << service->GetPath();
+      delete service;
+      return;
+    }
+    // If the main loop is running, then we might be inside of a D-Bus callback.
+    // We add the service to our |services_pending_deletion_| list and schedule
+    // a glib callback to delete these services from the main loop if one is not
+    // already scheduled.
+    DLOG(INFO) << "DeleteServiceOrScheduleForLaterDeletion: scheduling service "
+        << "for later deletion: " << service->GetPath();
+    services_pending_deletion_.push_back(service);
+    if (deferred_service_deletion_source_id_ != 0) {
+      DLOG(INFO) << "DeleteServiceOrScheduleForLaterDeletion: "
+          << "deferred service deletion callback already scheduled";
+      return;
+    }
+    deferred_service_deletion_source_id_ =
+        g_idle_add(StaticDeletePendingServicesCallback, this);
+    if (deferred_service_deletion_source_id_ == 0) {
+      LOG(ERROR)
+          << "DeleteServiceOrScheduleForLaterDeletion: g_idle_add failed";
+      return;
+    }
+    DLOG(INFO) << "DeleteServiceOrScheduleForLaterDeletion: "
+        << "scheduled deferred service deletion callback";
+}
+
+// static
+gboolean ServiceManager::StaticDeletePendingServicesCallback(gpointer data) {
+  ServiceManager *service_manager = reinterpret_cast<ServiceManager*>(data);
+  CHECK_NOTNULL(service_manager);
+  DCHECK_NE(service_manager->GetDeferredServiceDeletionSourceId(), 0);
+  service_manager->DeletePendingServices();
+  // we don't want to be run again automatically
+  // DeleteServiceOrScheduleForLaterDeletion will schedule us as needed
+  service_manager->SetDeferredServiceDeletionSourceId(0);
+  return FALSE;
+}
+
 // static
 gboolean ServiceManager::StaticGetFlimflamPropertiesCallback(gpointer data) {
   ServiceManager *service_manager = reinterpret_cast<ServiceManager*>(data);
@@ -319,6 +404,9 @@ void ServiceManager::OnServicesUpdate(const ServicePathList& paths) {
       services_[path] = service;
     } else {
       // service is new: create a Service obj for it and add it to services_
+      // TODO(vlaviano): If a service went away and came back immediately, there
+      // might still be a corresponding service object pending deletion. We
+      // could rescue it rather than allocating a new object here.
       OnNewService(path);
     }
   }
@@ -334,7 +422,7 @@ void ServiceManager::OnServicesUpdate(const ServicePathList& paths) {
   }
 
   // services left in old_services failed to appear in the update: delete them
-  DeleteServices(&old_services);
+  DeleteServicesWhenPossible(&old_services);
 
   // Flimflam always places the default service first in its list of services,
   // so we'll take a look at the first element in the paths list and see if the