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

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) {
+      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
   get_properties_source_id_ =
       g_idle_add(StaticGetFlimflamPropertiesCallback, this);
   if (get_properties_source_id_ == 0) {
     LOG(ERROR) << "ctor: g_idle_add failed";
   }
 }
 
 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_;
   }
 }
 
 const Service* ServiceManager::GetService(const std::string& service_path)
     const {
   ServiceMap::const_iterator it = services_.find(service_path);
   if (it == services_.end()) {
     return NULL;
   }
   DCHECK(it->second != NULL);
@@ skipping 92 matching lines @@
 }
 
 bool ServiceManager::RetryingGetProperties() const {
   return retrying_get_properties_;
 }
 
 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_);
+    delete service;
+    services_pending_deletion_.erase(services_pending_deletion_.begin());
+  }
+}
+
+void ServiceManager::DeleteServiceOrScheduleForLaterDeletion(Service *service) {
     DCHECK(service != NULL);
     DCHECK(service != default_cellular_service_);
-    DCHECK(*service_map == services_ || GetService(service->GetPath()) == NULL);
-    delete service;
-    service_map->erase(service_map->begin());
-  }
+    // 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);
   CHECK_NOTNULL(service_manager);
   DCHECK_NE(service_manager->GetGetPropertiesSourceId(), 0);
   if (!service_manager->GetFlimflamProperties()) {
     // call failed, so try again later
     DLOG(WARNING) << "StaticGetFlimflamPropertiesCallback: "
@@ skipping 115 matching lines @@
 
     ServiceMap::iterator old_it = old_services.find(path);
     if (old_it != old_services.end()) {
       // service is old: move it from old_services to services_
       Service *service = static_cast<Service *>(old_it->second);
       DCHECK(service != NULL);
       old_services.erase(old_it);
       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);
     }
   }
 
   // clear default service if it failed to appear in the update and we're
   // about to delete it
   if (default_cellular_service_ != NULL) {
     ServiceMap::const_iterator it =
         old_services.find(default_cellular_service_->GetPath());
     if (it != old_services.end()) {
       ClearDefaultCellularService();
     }
   }
 
   // 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
   // default service has changed. Note that |paths| can be empty. We still want
   // to know about this, since it means that there is no default service.
   const DBus::Path *default_service_path = NULL;
   if (!paths.empty()) {
     default_service_path = &*paths.begin();
   }
   OnDefaultServiceUpdate(default_service_path);
@@ skipping 83 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