Revert "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
-                               GMainLoop * const main_loop)
+ServiceManager::ServiceManager(DBus::Connection& connection)  // NOLINT
     : DBus::ObjectProxy(connection, kFlimflamManagerPath,
                         kFlimflamManagerName),
-      connection_(connection), main_loop_(CHECK_NOTNULL(main_loop)),
-      cashew_server_(NULL), default_technology_(Service::kTypeUnknown),
+      connection_(connection), cashew_server_(NULL),
+      default_technology_(Service::kTypeUnknown),
       default_cellular_service_(NULL),
       connectivity_state_(kConnectivityStateUnknown),
-      get_properties_source_id_(0), retrying_get_properties_(false),
-      deferred_service_deletion_source_id_(0) {
+      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";
   }
 }
 
 ServiceManager::~ServiceManager() {
-  DCHECK(!g_main_loop_is_running(main_loop_));
   ClearDefaultCellularService();
-  // we're not in the main loop, so we can delete services immediately
-  bool defer = false;
-  DeleteServices(&services_, defer);
-  DeletePendingServices();
+  DeleteServices(&services_);
   if (get_properties_source_id_ != 0 &&
       !g_source_remove(get_properties_source_id_)) {
-    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_;
+    DLOG(WARNING) << "dtor: g_source_remove failed";
   }
 }
 
 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, bool defer) {
-  DLOG(INFO) << "DeleteServices: defer = " << defer;
+void ServiceManager::DeleteServices(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;
     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);
-    if (defer) {
-      DLOG(INFO) << "DeleteServices: scheduling service for later deletion: "
-          << service->GetPath();
-      ScheduleServiceForLaterDeletion(service);
-    } else {
-      DLOG(INFO) << "DeleteServices: deleting service: " << service->GetPath();
-      delete service;
-    }
+    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
-  // 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);
+  DeleteServices(&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