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

src/service.cc

Index: src/service.cc
diff --git a/src/service.cc b/src/service.cc
index 797189b63aacdc4e2210b3487e2057f1aa673518..17d915ace314a7641b50be8aa766d3c468b8a7eb 100644
--- a/src/service.cc
+++ b/src/service.cc
@@ -83,7 +83,8 @@ Service::Service(ServiceManager * const parent,
       update_timeout_source_(NULL), policy_(NULL),
       is_default_service_(false), get_properties_source_id_(0),
       retrying_get_properties_(false),
-      connectivity_state_(kConnectivityStateUnknown) {
+      connectivity_state_(kConnectivityStateUnknown),
+      deferred_device_deletion_source_id_(0) {
   // 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
@@ -97,14 +98,24 @@ Service::Service(ServiceManager * const parent,
 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_ << ": deleting device " << device_->GetPath();
+    DLOG(INFO) << path_ << ": dtor: 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";
+    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_;
   }
 }
 
@@ -450,8 +461,66 @@ void Service::OnRetryingGetProperties(bool retrying) {
   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: "

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

Can you use __func__ or __FUNCTION__ instead of typing it manually?
Typing manually everytime seems a bit brittle.

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

I'll look at doing this cashew-wide in the future. It would probably reduce the
executable size for debug builds.

+        << 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);
+  CHECK_NOTNULL(service);

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

DCHECK(service != NULL); ??

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

Done.

+  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) {
@@ -507,7 +576,10 @@ void Service::OnDeviceUpdate(const DBus::Path& device_path) {
   if (device_ != NULL && device_->GetPath() != device_path) {
     LOG(WARNING) << path_ << ": OnDeviceUpdate: device path changed from "
         << device_->GetPath() << " to " << device_path;
-    delete device_;
+    // 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.
+    ScheduleDeviceForLaterDeletion(device_);

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

save a ref to device_, then NULL device_, then ScheduleForDelete

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

Done.

     device_ = NULL;
   }