Revert "cashew: do not delete DBus::ObjectProxy objects from D-Bus callbacks" This reverts commit 35386a9949355d796f2b195b0577e5a39fe72477. Reverting this CL in preparation for a more comprehensive CL that will also address the DBus::ObjectProxy creation case, which can also trigger a deadlock if done in a D-Bus callback. Change-Id: Ic60a1eb517b1cd76997aa3f8e00b42c44eeb018c BUG=chromium-os:8892 TEST=emerge cashew Review URL: http://codereview.chromium.org/5284004
diff --git a/src/cashew_server.cc b/src/cashew_server.cc index e56829b..dfef92f 100644 --- a/src/cashew_server.cc +++ b/src/cashew_server.cc
@@ -18,16 +18,13 @@ "org.chromium.Cashew.Error.ServiceUnknown"; CashewServer::CashewServer(DBus::Connection& connection, // NOLINT - ServiceManager * const service_manager, - GMainLoop * const main_loop) + ServiceManager * const service_manager) : DBus::ObjectAdaptor(connection, kServicePath), - service_manager_(CHECK_NOTNULL(service_manager)), - main_loop_(CHECK_NOTNULL(main_loop)) { + service_manager_(CHECK_NOTNULL(service_manager)) { service_manager_->SetCashewServer(this); } CashewServer::~CashewServer() { - DCHECK(!g_main_loop_is_running(main_loop_)); service_manager_->SetCashewServer(NULL); }
diff --git a/src/cashew_server.h b/src/cashew_server.h index 9764eeb..09d1d53 100644 --- a/src/cashew_server.h +++ b/src/cashew_server.h
@@ -5,8 +5,6 @@ #ifndef SRC_CASHEW_SERVER_H_ #define SRC_CASHEW_SERVER_H_ -#include <glib.h> - #include <map> #include <string> #include <vector> @@ -28,8 +26,7 @@ public DBus::ObjectAdaptor { public: CashewServer(DBus::Connection& connection, // NOLINT - ServiceManager * const service_manager, - GMainLoop * const main_loop); + ServiceManager * const service_manager); virtual ~CashewServer(); // Cashew D-Bus API methods @@ -56,9 +53,6 @@ private: ServiceManager * const service_manager_; - // glib main loop - GMainLoop * const main_loop_; - // Emit a DataPlansUpdate D-Bus signal for a service void EmitDataPlansUpdateSignal(const Service& service);
diff --git a/src/main.cc b/src/main.cc index d7f64fe..9d23397 100644 --- a/src/main.cc +++ b/src/main.cc
@@ -47,8 +47,6 @@ static void OnSignal(int signal) { DLOG(INFO) << "received signal " << signal; if (main_loop != NULL) { - // TODO(vlaviano): g_idle_add a callback to quit the main loop instead of - // doing it directly here to remove some low severity races g_main_loop_quit(main_loop); } } @@ -80,7 +78,6 @@ // should not be called while event loop is running static void CleanUpForExit() { - DCHECK(main_loop == NULL || !g_main_loop_is_running(main_loop)); DLOG(INFO) << "cleaning up"; delete server; server = NULL; @@ -128,7 +125,7 @@ DBus::Connection client_conn = DBus::Connection::SystemBus(); client_conn.set_timeout(cashew::kDBusConnectionTimeoutMilliseconds); cashew::service_manager = new(std::nothrow) cashew::ServiceManager( - client_conn, cashew::main_loop); + client_conn); if (cashew::service_manager == NULL) { LOG(ERROR) << "couldn't create service manager"; cashew::CleanUpForExit(); @@ -141,7 +138,7 @@ server_conn.request_name(cashew::CashewServer::kServiceName); server_conn.set_timeout(cashew::kDBusConnectionTimeoutMilliseconds); cashew::server = new(std::nothrow) cashew::CashewServer(server_conn, - cashew::service_manager, cashew::main_loop); + cashew::service_manager); if (cashew::server == NULL) { LOG(ERROR) << "couldn't create cashew server"; cashew::CleanUpForExit();
diff --git a/src/service.cc b/src/service.cc index 81c951a..797189b 100644 --- a/src/service.cc +++ b/src/service.cc
@@ -83,8 +83,7 @@ update_timeout_source_(NULL), policy_(NULL), is_default_service_(false), get_properties_source_id_(0), retrying_get_properties_(false), - connectivity_state_(kConnectivityStateUnknown), - deferred_device_deletion_source_id_(0) { + connectivity_state_(kConnectivityStateUnknown) { // schedule a GetProperties() call to our Flimflam service path to init state // we'll keep trying periodically until we succeed // we'll subsequently update this state by monitoring PropertyChanged signals @@ -98,24 +97,14 @@ Service::~Service() { DeleteCarrierState(); DeleteDataPlans(&data_plans_); - // ServiceManager ensures that this dtor is not being called from within a - // D-Bus callback, so it's ok to delete |device_| and any members of - // |devices_pending_deletion_| directly. if (device_ != NULL) { - DLOG(INFO) << path_ << ": dtor: deleting device " << device_->GetPath(); + DLOG(INFO) << path_ << ": deleting device " << device_->GetPath(); delete device_; device_ = NULL; } - DeletePendingDevices(); if (get_properties_source_id_ != 0 && !g_source_remove(get_properties_source_id_)) { - DLOG(WARNING) << path_ << ": dtor: g_source_remove failed for source id " - << get_properties_source_id_; - } - if (deferred_device_deletion_source_id_ != 0 && - !g_source_remove(deferred_device_deletion_source_id_)) { - DLOG(WARNING) << path_ << ": dtor: g_source_remove failed for source id " - << deferred_device_deletion_source_id_; + DLOG(WARNING) << path_ << ": dtor: g_source_remove failed"; } } @@ -461,66 +450,8 @@ retrying_get_properties_ = retrying; } -guint Service::GetDeferredDeviceDeletionSourceId() const { - return deferred_device_deletion_source_id_; -} - -void Service::SetDeferredDeviceDeletionSourceId(guint source_id) { - deferred_device_deletion_source_id_ = source_id; -} - // Private methods -void Service::DeletePendingDevices() { - while (!devices_pending_deletion_.empty()) { - Device *device = *(devices_pending_deletion_.begin()); - DCHECK(device != NULL); - DLOG(INFO) << path_ << ": DeletePendingDevices: deleting device: " - << device->GetPath(); - DCHECK(device != device_); - delete device; - devices_pending_deletion_.erase(devices_pending_deletion_.begin()); - } -} - -void Service::ScheduleDeviceForLaterDeletion(Device *device) { - DCHECK(device != NULL); - DCHECK(device != device_); - // We might be inside of a D-Bus callback. We add the device to our - // |devices_pending_deletion_| list and schedule a glib callback to - // delete these devices from the main loop if one is not already - // scheduled. - DLOG(INFO) << path_ << ": ScheduleDeviceForLaterDeletion: " - << device->GetPath(); - devices_pending_deletion_.push_back(device); - if (deferred_device_deletion_source_id_ != 0) { - DLOG(INFO) << path_ << ": ScheduleDeviceForLaterDeletion: " - << "deferred device deletion callback already scheduled"; - return; - } - deferred_device_deletion_source_id_ = - g_idle_add(StaticDeletePendingDevicesCallback, this); - if (deferred_device_deletion_source_id_ == 0) { - LOG(ERROR) << path_ - << ": ScheduleDeviceForLaterDeletion: g_idle_add failed"; - return; - } - DLOG(INFO) << path_ << ": ScheduleDeviceForLaterDeletion: " - << "scheduled deferred device deletion callback"; -} - -// static -gboolean Service::StaticDeletePendingDevicesCallback(gpointer data) { - Service *service = reinterpret_cast<Service*>(data); - DCHECK(service != NULL); - DCHECK_NE(service->GetDeferredDeviceDeletionSourceId(), 0); - service->DeletePendingDevices(); - // we don't want to be run again automatically - // ScheduleDeviceForLaterDeletion will schedule us as needed - service->SetDeferredDeviceDeletionSourceId(0); - return FALSE; -} - // static Service::ConnectivityState Service::ConnectivityStateFromString( const std::string& connectivity_state) { @@ -576,12 +507,8 @@ if (device_ != NULL && device_->GetPath() != device_path) { LOG(WARNING) << path_ << ": OnDeviceUpdate: device path changed from " << device_->GetPath() << " to " << device_path; - // We might be in a D-Bus callback, so schedule device for later deletion - // to avoid possible deadlock. See the comments in service_manager.h and - // service_manager.cc for more on this issue. - Device *device_to_delete = device_; + delete device_; device_ = NULL; - ScheduleDeviceForLaterDeletion(device_to_delete); } // if there's an existing device with matching path: all is well
diff --git a/src/service.h b/src/service.h index 7bda672..5143fed 100644 --- a/src/service.h +++ b/src/service.h
@@ -11,7 +11,6 @@ #include <map> #include <string> -#include <vector> #include <base/basictypes.h> // NOLINT #include <dbus-c++/dbus.h> // NOLINT @@ -29,9 +28,6 @@ // map of key, value pairs representing service properties received via D-Bus typedef std::map<std::string, DBus::Variant> PropertyMap; -// vector of Device objects -typedef std::vector<Device*> DeviceList; - // represents a cellular service and monitors Flimflam state for that service class Service : public org::chromium::flimflam::Service_proxy, public DBus::IntrospectableProxy, @@ -147,12 +143,6 @@ // set the retrying flag virtual void OnRetryingGetProperties(bool retrying); - // get deferred device deletion source id - virtual guint GetDeferredDeviceDeletionSourceId() const; - - // set deferred device deletion source id - virtual void SetDeferredDeviceDeletionSourceId(guint source_id); - private: // back pointer to our parent ServiceManager ServiceManager * const parent_; @@ -209,24 +199,6 @@ // finer-grained connectivity info applicable when |state_| == kStateReady ConnectivityState connectivity_state_; - // Device objects pending deletion from the main loop - // We need to do this to work around a dbus-c++ deadlock issue that arises - // when we send a D-Bus message from a D-Bus callback - DeviceList devices_pending_deletion_; - - // glib source id for deferred device deletion - guint deferred_device_deletion_source_id_; - - // deletes members of |devices_pending_deletion_| immediately - void DeletePendingDevices(); - - // schedule a device for later deletion from the main loop - void ScheduleDeviceForLaterDeletion(Device *device); - - // glib integration: static wrapper for DeletePendingDevices - // takes object ptr as data and invokes object->DeletePendingDevices() - static gboolean StaticDeletePendingDevicesCallback(gpointer data); - // convert connectivity state string to ConnectivityState enum value static ConnectivityState ConnectivityStateFromString( const std::string& connectivity_state);
diff --git a/src/service_manager.cc b/src/service_manager.cc index 97dbf12..484f08c 100644 --- a/src/service_manager.cc +++ b/src/service_manager.cc
@@ -33,16 +33,14 @@ 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 @@ -54,21 +52,11 @@ } 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"; } } @@ -181,79 +169,23 @@ 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); @@ -387,9 +319,6 @@ 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); } } @@ -405,11 +334,7 @@ } // 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
diff --git a/src/service_manager.h b/src/service_manager.h index f248025..e12fd91 100644 --- a/src/service_manager.h +++ b/src/service_manager.h
@@ -5,8 +5,6 @@ #ifndef SRC_SERVICE_MANAGER_H_ #define SRC_SERVICE_MANAGER_H_ -#include <glib.h> - #include <map> #include <string> #include <vector> @@ -24,9 +22,6 @@ // map of service path names to Service objects typedef std::map<std::string, Service*> ServiceMap; -// vector of Service objects -typedef std::vector<Service*> ServiceList; - // vector of DBus::Path objects (strings) representing service path names typedef std::vector<DBus::Path> ServicePathList; @@ -35,8 +30,7 @@ public DBus::IntrospectableProxy, public DBus::ObjectProxy { public: - explicit ServiceManager(DBus::Connection& connection, // NOLINT - GMainLoop * const main_loop); + explicit ServiceManager(DBus::Connection& connection); // NOLINT virtual ~ServiceManager(); // look up a service by its path name @@ -98,20 +92,11 @@ // set the retrying flag virtual void OnRetryingGetProperties(bool retrying); - // get deferred service deletion source id - virtual guint GetDeferredServiceDeletionSourceId() const; - - // set deferred service deletion source id - virtual void SetDeferredServiceDeletionSourceId(guint source_id); - private: // D-Bus connection owned by our creator // shared with Service objs that we create DBus::Connection& connection_; - // glib main loop - GMainLoop * const main_loop_; - // collection of Service objs representing cellular services ServiceMap services_; @@ -138,29 +123,8 @@ // and our subsequent timer calls bool retrying_get_properties_; - // Service objects pending deletion from the main loop - // we need to do this to work around a dbus-c++ deadlock issue that arises - // when we send a D-Bus message from a D-Bus callback - ServiceList services_pending_deletion_; - - // glib source id for deferred service deletion - guint deferred_service_deletion_source_id_; - // delete a collection of services - // deletes services immediately if |defer| == false - // schedules services for later deletion if |defer| == true - void DeleteServices(ServiceMap *service_map, bool defer); - - // deletes members of |services_pending_deletion_| immediately - void DeletePendingServices(); - - // schedule service for later deletion from the main loop - void ScheduleServiceForLaterDeletion(Service *service); - - // glib integration: static wrapper for DeletePendingServices - // takes object ptr as data and invokes object->DeletePendingServices() - // does not reschedule itself to run again - static gboolean StaticDeletePendingServicesCallback(gpointer data); + void DeleteServices(ServiceMap *service_map); // glib integration: static wrapper for GetFlimflamProperties // takes object ptr as data and invokes object->GetFlimflamProperties()