Fix BluetoothAdapterProfileChromeOS lifecycle management

device/bluetooth/bluetooth_adapter_chromeos.cc

 // Copyright 2013 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_chromeos.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/location.h"
@@ skipping 110 matching lines @@
       DBusThreadManager::Get()->GetBluetoothAdapterClient()->GetAdapters();
 
   if (!object_paths.empty()) {
     VLOG(1) << object_paths.size() << " Bluetooth adapter(s) available.";
     SetAdapter(object_paths[0]);
   }
 }
 
 BluetoothAdapterChromeOS::~BluetoothAdapterChromeOS() {
   Shutdown();
 }

[Ilya Sherman, 2015/02/21 01:22:10]

Should any remaining profiles be deleted as part of the destructor?  If not, who
is responsible for cleaning up their memory?  (Again, smart pointers make this
easier to manage... though I'm not sure whether we have any smart pointers that
play nicely with maps.  Sadly we can't use C++11's unique_ptr yet.)

[Marie Janssen, 2015/02/23 17:49:09]

All profiles will get unregistered (and subsequently deleted) when the adapter
gets destructed through PresentChanged(false). 

Regarding the smart pointers, if there is one we have I haven't found it
looking, and trying scoped_ptr doesn't work even though it has some of the same
move semantics as unique_ptr.

[Ilya Sherman, 2015/02/24 05:26:06]

I'm not following how PresentChanged(false) results in the adapter being
destructed -- it looks like that method just fires off a notification.  Could
you please clarify your comment?

In any case, if you believe that the map is guaranteed to be empty, could you
please add a DCHECK to verify this?
Yeah, scoped_ptr isn't quite as smart as unique_ptr.  Thanks for checking =)

[Marie Janssen, 2015/02/24 18:49:21]

Sorry this was unclear wording.  Shutdown() eventually calls
PresentChanged(false), which will cause all BluetoothSockets which are using a
Profile to ReleaseProfile(), which will cause all profiles to be unused and
deleted / removed.

I can add a DCHECK to verify, since I believe it will always be empty.

 
 void BluetoothAdapterChromeOS::DeleteOnCorrectThread() const {
   if (ui_task_runner_->RunsTasksOnCurrentThread() ||
       !ui_task_runner_->DeleteSoon(FROM_HERE, this))
     delete this;
 }
 
 void BluetoothAdapterChromeOS::AddObserver(
     BluetoothAdapter::Observer* observer) {
   DCHECK(observer);
@@ skipping 815 matching lines @@
 
   if (profiles_.find(uuid) != profiles_.end()) {
     // TODO(jamuraa) check that the options are the same and error when they are
     // not.
     SetProfileDelegate(uuid, device_path, delegate, success_callback,
                        error_callback);
     return;
   }
 
   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) {
+  profile->RemoveDelegate(
+      device_path, base::Bind(&BluetoothAdapterChromeOS::RemoveProfile,
+                              weak_ptr_factory_.GetWeakPtr(), profile->uuid()));

[Ilya Sherman, 2015/02/21 01:22:10]

nit: Is this what git cl format produces?  The formatting looks a little odd to
me.

[Marie Janssen, 2015/02/23 17:49:09]

This is in fact what cl format produced.  It looks a little strange to me too
(probably would put device_path on the first line) but I usually defer to cl
format.

+}
+
+void BluetoothAdapterChromeOS::RemoveProfile(const BluetoothUUID& uuid) {
   if (profiles_.find(uuid) != profiles_.end()) {
     delete profiles_[uuid];
     profiles_.erase(uuid);
   }
 }
 
 void BluetoothAdapterChromeOS::OnRegisterProfile(
     const BluetoothUUID& uuid,
     const dbus::ObjectPath& device_path,
     BluetoothProfileServiceProvider::Delegate* delegate,
@@ skipping 19 matching lines @@
 }
 
 void BluetoothAdapterChromeOS::OnRegisterProfileError(
     const BluetoothUUID& uuid,
     const ErrorCompletionCallback& error_callback,
     const std::string& error_name,
     const std::string& error_message) {
   LOG(WARNING) << object_path_.value()
                << ": Failed to register profile: " << error_name << ": "
                << error_message;
+  delete profiles_[uuid];
   error_callback.Run(error_message);
-  ReleaseProfile(uuid);
 }
 
 void BluetoothAdapterChromeOS::OnSetDiscoverable(
     const base::Closure& callback,
     const ErrorCallback& error_callback,
     bool success) {
   DCHECK(IsPresent());
   // Set the discoverable_timeout property to zero so the adapter remains
   // discoverable forever.
   DBusThreadManager::Get()->GetBluetoothAdapterClient()->
@@ skipping 191 matching lines @@
 
     // If the queued request resulted in a pending call, then let it
     // asynchonously process the remaining queued requests once the pending
     // call returns.
     if (discovery_request_pending_)
       return;
   }
 }
 
 }  // namespace chromeos