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_);
+  // we're not in the main loop, so we can delete services immediately
+  bool defer = false;
+  DeleteServices(&services_, defer);
+  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::DeleteServices(ServiceMap *service_map, bool defer) {
+  DLOG(INFO) << "DeleteServices: defer = " << defer;
   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;
     Service *service = static_cast<Service *>(service_map->begin()->second);
     DCHECK(service != NULL);
     DCHECK(service != default_cellular_service_);
     DCHECK(*service_map == services_ || GetService(service->GetPath()) == NULL);
-    delete service;
+    if (defer) {
+      DLOG(INFO) << "DeleteServices: scheduling service for later deletion: "
+          << service->GetPath();
+      ScheduleServiceForLaterDeletion(service);
+    } else {
+      DLOG(INFO) << "DeleteServices: deleting service: " << service->GetPath();
+      delete service;
+    }
     service_map->erase(service_map->begin());
   }
 }
 
+void ServiceManager::DeletePendingServices() {
+  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::ScheduleServiceForLaterDeletion(Service *service) {
+    DCHECK(service != NULL);
+    DCHECK(service != default_cellular_service_);
+    // we might be inside of a D-Bus callback.
+    // add the service to our |services_pending_deletion_| list, and schedule a
+    // glib callback to delete these services later from the main loop if such
+    // a callback is not already scheduled
+    services_pending_deletion_.push_back(service);
+    if (deferred_service_deletion_source_id_ != 0) {
+      return;
+    }
+    deferred_service_deletion_source_id_ =
+        g_idle_add(StaticDeletePendingServicesCallback, this);
+    if (deferred_service_deletion_source_id_ == 0) {
+      LOG(ERROR) << "ScheduleServiceForLaterDeletion: g_idle_add failed";
+      return;
+    }
+}
+
+// static
+gboolean ServiceManager::StaticDeletePendingServicesCallback(gpointer data) {
+  ServiceManager *service_manager = reinterpret_cast<ServiceManager*>(data);
+  DCHECK(service_manager != NULL);
+  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: "
         << "GetFlimflamProperties failed, will retry in "
         << kGetFlimflamPropertiesIntervalSeconds << " secs";
@@ skipping 113 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);
+  // NOTE: We're in a D-Bus callback so we can't delete these services
+  // immediately without triggering a potential deadlock. We schedule them for
+  // deferred deletion.
+  bool defer = true;
+  DeleteServices(&old_services, defer);
 
   // 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