Fix BluetoothAdapterProfileChromeOS lifecycle management

device/bluetooth/bluetooth_socket_chromeos.cc

Index: device/bluetooth/bluetooth_socket_chromeos.cc
diff --git a/device/bluetooth/bluetooth_socket_chromeos.cc b/device/bluetooth/bluetooth_socket_chromeos.cc
index 3026113a5cb9a1de191b98e29b04759b29129b4e..a67e974fc0849d0e5702b9b3b7beb138303fd17a 100644
--- a/device/bluetooth/bluetooth_socket_chromeos.cc
+++ b/device/bluetooth/bluetooth_socket_chromeos.cc
@@ -110,6 +110,8 @@ void BluetoothSocketChromeOS::Connect(
   if (security_level == SECURITY_LEVEL_LOW)
     options_->require_authentication.reset(new bool(false));
 
+  adapter_ = device->adapter();
+
   RegisterProfile(device->adapter(), success_callback, error_callback);
 }
 
@@ -128,7 +130,8 @@ void BluetoothSocketChromeOS::Listen(
     return;
   }
 
-  adapter->AddObserver(this);
+  adapter_ = adapter;
+  adapter_->AddObserver(this);
 
   uuid_ = uuid;
   options_.reset(new BluetoothProfileManagerClient::Options());
@@ -221,12 +224,10 @@ void BluetoothSocketChromeOS::RegisterProfile(
   DCHECK(!profile_);
   DCHECK(adapter);

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

Should you also DCHECK that adapter_ and adapter are the same object?  Or am I
misunderstanding the changes that you've made in terms of where adapter_ is
being set?

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

we don't use adapter_ in this function anymore.  this is just reverting the
places adapter_ is set to the way it was before.

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

I agree that |adapter_| isn't accessed, but should it be guaranteed that
|adapter| and |adapter_| are equal?  When you say "reverting ... to the way it
was before", when is the "before" that you are referring to -- is this something
that you changed in a recent CL?

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

I mean from before this CL.  Earlier patches moved the assignment, later patches
switch it back to the original way.

[Ilya Sherman, 2015/02/24 19:00:21]

Per offline discussion, this CL is partially restoring the state from [
https://codereview.chromium.org/910433002 ], and also adding the the |adapter_|
caching in the Connect() method.

 
-  adapter_ = adapter;
-
   // If the adapter is not present, this is a listening socket and the
   // adapter isn't running yet.  Report success and carry on;
   // the profile will be registered when the daemon becomes available.
-  if (!adapter_->IsPresent()) {
+  if (!adapter->IsPresent()) {
     VLOG(1) << uuid_.canonical_value() << " on " << device_path_.value()
             << ": Delaying profile registration.";
     base::MessageLoop::current()->PostTask(FROM_HERE, success_callback);
@@ -549,20 +550,9 @@ void BluetoothSocketChromeOS::UnregisterProfile() {
 
   VLOG(1) << profile_->object_path().value() << ": Release profile";
 
-  profile_->RemoveDelegate(
-      device_path_,
-      base::Bind(&BluetoothSocketChromeOS::ReleaseProfile, this, profile_));
-
+  static_cast<BluetoothAdapterChromeOS*>(adapter_.get())
+      ->ReleaseProfile(device_path_, profile_);

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

Is it guaranteed that the adapter_ is non-null?  I'm asking because the code
previously had the guard "if (adapter_) ..."

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

Here it is: if we have a profile to unregister at all, it would have been set,
and before we set it to NULL we unregister the profile.

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

You're probably right, as I do not know this code as well as you do, but: Could
you please clarify why this wasn't equally true for the ReleaseProfile() method,
which had the guard "if (adapter_) ..."?

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

ReleaseProfile() was called asynchronously from a D-Bus callback, which meant
that sometimes it would be called after the adapter_ got set to NULL (in
Close()).

[Ilya Sherman, 2015/02/24 19:00:21]

Ok, that makes sense -- thanks!

   profile_ = nullptr;
 }
 
-void BluetoothSocketChromeOS::ReleaseProfile(
-    BluetoothAdapterProfileChromeOS* profile) {
-  if (adapter_)
-    static_cast<BluetoothAdapterChromeOS*>(adapter_.get())
-        ->ReleaseProfile(uuid_);
-  else
-    delete profile;
-}
-
 }  // namespace chromeos