Fix BluetoothAdapterProfileChromeOS lifecycle management

device/bluetooth/bluetooth_adapter_chromeos.cc

Index: device/bluetooth/bluetooth_adapter_chromeos.cc
diff --git a/device/bluetooth/bluetooth_adapter_chromeos.cc b/device/bluetooth/bluetooth_adapter_chromeos.cc
index e6c628322d349b629b507b17f753d8d541b35955..6af221ad2569848e390f23ee226b077e027d8cab 100644
--- a/device/bluetooth/bluetooth_adapter_chromeos.cc
+++ b/device/bluetooth/bluetooth_adapter_chromeos.cc
@@ -85,6 +85,7 @@ void BluetoothAdapterChromeOS::Shutdown() {
   if (IsPresent())
     RemoveAdapter();  // Also deletes devices_.
   DCHECK(devices_.empty());
+  DCHECK(profiles_.empty());

[armansito, 2015/02/24 23:32:51]

Is this because RemoveAdapter clears |profiles_|? If so, add a note above ("//
Cleans up |devices_| and |profiles_|.")

[Marie Janssen, 2015/02/27 18:56:58]

RemoveAdapter() signals the profiles through PresentChanged(false), which should
clean up all the profiles_.  Added a clarifying comment.

 
   DBusThreadManager::Get()->GetBluetoothAdapterClient()->RemoveObserver(this);
   DBusThreadManager::Get()->GetBluetoothDeviceClient()->RemoveObserver(this);
@@ -964,14 +965,22 @@ void BluetoothAdapterChromeOS::UseProfile(
   }
 
   profiles_[uuid] = BluetoothAdapterProfileChromeOS::Register(
-      this, uuid, options,
+      uuid, options,
       base::Bind(&BluetoothAdapterChromeOS::OnRegisterProfile, this, uuid,
                  device_path, delegate, success_callback, error_callback),
       base::Bind(&BluetoothAdapterChromeOS::OnRegisterProfileError, this, uuid,
                  error_callback));
 }
 
-void BluetoothAdapterChromeOS::ReleaseProfile(const BluetoothUUID& uuid) {
+void BluetoothAdapterChromeOS::ReleaseProfile(
+    const dbus::ObjectPath& device_path,
+    BluetoothAdapterProfileChromeOS* profile) {

[armansito, 2015/02/24 23:32:51]

Add VLOG

[Marie Janssen, 2015/02/27 18:56:58]

Done.

+  profile->RemoveDelegate(
+      device_path, base::Bind(&BluetoothAdapterChromeOS::RemoveProfile,
+                              weak_ptr_factory_.GetWeakPtr(), profile->uuid()));
+}
+
+void BluetoothAdapterChromeOS::RemoveProfile(const BluetoothUUID& uuid) {
   if (profiles_.find(uuid) != profiles_.end()) {
     delete profiles_[uuid];
     profiles_.erase(uuid);
@@ -1011,8 +1020,9 @@ void BluetoothAdapterChromeOS::OnRegisterProfileError(
   LOG(WARNING) << object_path_.value()

[armansito, 2015/02/24 23:32:51]

Change this to VLOG

[Marie Janssen, 2015/02/27 18:56:58]

Done.

                << ": Failed to register profile: " << error_name << ": "
                << error_message;
+  delete profiles_[uuid];
+  profiles_.erase(uuid);

[armansito, 2015/02/24 23:32:51]

I think you can simplify the memory management here. Instead of returning a
profile pointer directly from BluetoothAdapterProfileChromeOS::Register, you can
change the success callback signature to accept a
scoped_ptr<BluetoothAdapterProfileChromeOS>. In the success callback, you could
then take ownership of the scoped_ptr while setting profiles_[uuid] and simply
do nothing in the error callback.

This would mean that you would have to change how you condition the
SetProfileDelegate case in ::UseProfile above but I think it's already
inconsistent in the following case:

1. I call UseProfile, it tries to register BluetoothAdapterProfileChromeOS, the
call is pending.

2. I call UseProfile, it calls SetProfileDelegate and calls my success callback
with a pointer to the profile.

3. My call from #1 fails, the profile gets deleted and my error callback is
called. The initial pointer that I received in the intermediate success callback
is invalid.

[Marie Janssen, 2015/02/27 18:56:58]

I've reworked this to avoid these problems.  Now the callbacks go onto a queue
for when the profile suceeds / fails, with all clients getting the profile (or
error) when the single RegisterProfile returns.

   error_callback.Run(error_message);
-  ReleaseProfile(uuid);
 }
 
 void BluetoothAdapterChromeOS::OnSetDiscoverable(