Fix BluetoothAdapterProfileChromeOS lifecycle management

device/bluetooth/bluetooth_adapter_profile_chromeos.cc

 // Copyright 2015 The Chromium 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 "device/bluetooth/bluetooth_adapter_profile_chromeos.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "chromeos/dbus/bluetooth_profile_service_provider.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "dbus/bus.h"
 #include "dbus/object_path.h"
 #include "device/bluetooth/bluetooth_adapter_chromeos.h"
 #include "device/bluetooth/bluetooth_uuid.h"
 
 namespace chromeos {
 
+namespace {
+
+void OnRegisterProfileError(
+    scoped_ptr<BluetoothAdapterProfileChromeOS> profile,
+    const BluetoothProfileManagerClient::ErrorCallback& error_callback,
+    const std::string& error_name,
+    const std::string& error_message) {
+  error_callback.Run(error_name, error_message);
+}
+
+}  // anonymous namespace
+
 // static
-BluetoothAdapterProfileChromeOS* BluetoothAdapterProfileChromeOS::Register(
-    BluetoothAdapterChromeOS* adapter,
+void BluetoothAdapterProfileChromeOS::Register(
     const device::BluetoothUUID& uuid,
     const BluetoothProfileManagerClient::Options& options,
-    const base::Closure& success_callback,
+    const ProfileRegisteredCallback& success_callback,
     const BluetoothProfileManagerClient::ErrorCallback& error_callback) {
-  DCHECK(adapter);
-
-  BluetoothAdapterProfileChromeOS* profile =
-      new BluetoothAdapterProfileChromeOS(adapter, uuid);
+  scoped_ptr<BluetoothAdapterProfileChromeOS> profile(
+      new BluetoothAdapterProfileChromeOS(uuid));
 
   VLOG(1) << "Registering profile: " << profile->object_path().value();
   DBusThreadManager::Get()->GetBluetoothProfileManagerClient()->RegisterProfile(
-      profile->object_path(), uuid.canonical_value(), options, success_callback,
-      error_callback);
-
-  return profile;
+      profile->object_path(),
+      uuid.canonical_value(),
+      options,
+      base::Bind(success_callback, base::Passed(&profile)),
+      base::Bind(OnRegisterProfileError, base::Passed(&profile),
+                 error_callback));

[Ilya Sherman, 2015/03/05 04:33:54]

This looks to me like it will actually lead to a double free, as you're passing
ownership of the profile both to the success callback and to the error callback.
 Or, maybe it will actually just mean that one callback will get the profile
value, and the other will get a null value.  If so, which value is non-null
might be compiler dependent, as some compilers evaluate arguments in LTR order,
and others in RTL order.

Either way, you shouldn't need to pass the profile to the error callback
anymore, and you can remove the OnRegisterProfileError() wrapper function
entirely.  You're already passing ownership to the success callback.  If the
success callback is never called, then the profile will be deleted when the
success callback is deleted.

[Marie Janssen, 2015/03/09 16:22:49]

Done.

 }
 
 BluetoothAdapterProfileChromeOS::BluetoothAdapterProfileChromeOS(
-    BluetoothAdapterChromeOS* adapter,
     const device::BluetoothUUID& uuid)
-    : uuid_(uuid), adapter_(adapter), weak_ptr_factory_(this) {
+    : uuid_(uuid), weak_ptr_factory_(this) {
   std::string uuid_path;
   base::ReplaceChars(uuid.canonical_value(), ":-", "_", &uuid_path);
   object_path_ =
       dbus::ObjectPath("/org/chromium/bluetooth_profile/" + uuid_path);
 
   dbus::Bus* system_bus = DBusThreadManager::Get()->GetSystemBus();
   profile_.reset(
       BluetoothProfileServiceProvider::Create(system_bus, object_path_, this));
   DCHECK(profile_.get());
 }
 
 BluetoothAdapterProfileChromeOS::~BluetoothAdapterProfileChromeOS() {
-  profile_.reset();
 }
 
 bool BluetoothAdapterProfileChromeOS::SetDelegate(
     const dbus::ObjectPath& device_path,
     BluetoothProfileServiceProvider::Delegate* delegate) {
   DCHECK(delegate);
   VLOG(1) << "SetDelegate: " << object_path_.value() << " dev "
           << device_path.value();
 
   if (delegates_.find(device_path.value()) != delegates_.end()) {
@@ skipping 88 matching lines @@
   // Cancel() should only go to a delegate accepting connections.
   if (delegates_.find("") == delegates_.end()) {
     VLOG(1) << object_path_.value() << ": Cancel with no delegate!";
     return;
   }
 
   delegates_[""]->Cancel();
 }
 
 }  // namespace chromeos