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

src/service.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.h"
 
 #include <glog/logging.h>
 
 #include "src/data_plan_provider.h"
 #include "src/device.h"
@@ skipping 65 matching lines @@
 Service::Service(ServiceManager * const parent,
                  DBus::Connection& connection,  // NOLINT
                  const DBus::Path& path)
     : DBus::ObjectProxy(connection, path, kFlimflamServiceName),
       parent_(CHECK_NOTNULL(parent)), connection_(connection), path_(path),
       state_(kStateUnknown), type_(kTypeUnknown), device_(NULL),
       provider_(NULL), request_in_progress_(false),
       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
   get_properties_source_id_ =
       g_idle_add(StaticGetServicePropertiesCallback, this);
   if (get_properties_source_id_ == 0) {
     LOG(ERROR) << path_ << ": ctor: g_idle_add failed";
   }
 }
 
 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_;
   }
 }
 
 const DBus::Path& Service::GetPath() const {
   return path_;
 }
 
 Service::State Service::GetState() const {
   return state_;
 }
@@ skipping 325 matching lines @@
 }
 
 bool Service::RetryingGetProperties() const {
   return retrying_get_properties_;
 }
 
 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) {
   if (connectivity_state == kFlimflamServiceConnectivityStateUnknown) {
     return kConnectivityStateUnknown;
   }
   if (connectivity_state == kFlimflamServiceConnectivityStateRestricted) {
     return kConnectivityStateRestricted;
   }
   if (connectivity_state == kFlimflamServiceConnectivityStateUnrestricted) {
@@ skipping 35 matching lines @@
 }
 
 void Service::OnDeviceUpdate(const DBus::Path& device_path) {
   DLOG(INFO) << path_ << ": OnDeviceUpdate: device_path = " << device_path;
 
   // if there's an existing device with a non-matching path: destroy it and
   // fall through to make a new one below.
   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;
   }
 
   // if there's an existing device with matching path: all is well
   if (device_ != NULL) {
     return;
   }
 
   // if there's no existing device: make one
   device_ = new(std::nothrow) Device(this, connection_, device_path);
@@ skipping 394 matching lines @@
   DCHECK(policy_ != NULL);
   if (policy_->ShouldEmitDataPlansUpdate(data_plans_)) {
     DLOG(INFO) << path_ << ": MaybeEmitDataPlansUpdate: sending update";
     parent_->EmitDataPlansUpdate(*this);
   } else {
     DLOG(INFO) << path_ << ": MaybeEmitDataPlansUpdate: not sending update";
   }
 }
 
 }  // namespace cashew