Fix BluetoothAdapterProfileChromeOS lifecycle management

Fix BluetoothAdapterProfileChromeOS lifecycle management

Have BluetoothAdapterProfileChromeOS objects managed through
BluetoothAdapterChromeOS, avoiding problems where references were being
left behind in BluetoothSocketChromeOS after deletion.

Adds some unit tests to confirm lifecycle works correctly even when
multiple profiles are requested simultaneously.

This is a resubmit of https://codereview.chromium.org/935383003/ after a revert
for LSan test failures https://codereview.chromium.org/975323002/, a patch with
just changes from the old CL can be found there.

BUG=457978
R=armansito, isherman@chromium.org, keybuk

Committed: https://crrev.com/202fef401ecfdef89837e709847f4e2189f0ee31
Cr-Commit-Position: refs/heads/master@{#319721}

[Marie Janssen, 2015-03-04 18:20:40]

Fix for memory leaks.  

See https://codereview.chromium.org/935383003/ patchset 8 for diffs from
previous CL.

[Ilya Sherman, 2015-03-04 19:12:48]

Thanks, Michael.  Note that the best practice is to upload the reverted patch as
Patch Set 1, and to upload the changes in subsequent patch sets.

https://codereview.chromium.org/982593002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.h (right):

https://codereview.chromium.org/982593002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:31:
ProfileRegisteredCallback;
Can this be a scoped_ptr rather than a raw pointer?

[Ilya Sherman, 2015-03-04 19:20:20]

Sorry, I added my other comment to the wrong CL:

https://codereview.chromium.org/982593002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right):

https://codereview.chromium.org/982593002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:41: new
BluetoothAdapterProfileChromeOS(uuid);
Please use scoped_ptrs rather than manual memory management.  You'll want to use
base::Passed() to pass the scoped_ptr into the success callback.

[Marie Janssen, 2015-03-04 23:45:02]

https://codereview.chromium.org/982593002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right):

https://codereview.chromium.org/982593002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:41: new
BluetoothAdapterProfileChromeOS(uuid);
On 2015/03/04 19:20:20, Ilya Sherman wrote:
> Please use scoped_ptrs rather than manual memory management.  You'll want to
use
> base::Passed() to pass the scoped_ptr into the success callback.

Done.

https://codereview.chromium.org/982593002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.h (right):

https://codereview.chromium.org/982593002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:31:
ProfileRegisteredCallback;
On 2015/03/04 19:12:48, Ilya Sherman wrote:
> Can this be a scoped_ptr rather than a raw pointer?

Done.

[Ilya Sherman, 2015-03-05 04:33:55]

https://codereview.chromium.org/982593002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right):

https://codereview.chromium.org/982593002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:49: error_callback));
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.

https://codereview.chromium.org/982593002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc (right):

https://codereview.chromium.org/982593002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:129:
BluetoothAdapterProfileChromeOS* profile_ptr_;
It's kind of confusing to have both profile_ and profile_ptr_.  It looks like
tests use either one or the other.  Maybe it makes sense to separate these sets
of tests somehow?  Or, maybe the two different profile pointer variables could
have names that hint more directly at how they are used?

https://codereview.chromium.org/982593002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:129:
BluetoothAdapterProfileChromeOS* profile_ptr_;
FWIW, it's also surprising to me that this test class has public member
variables.  The typical style is to list all member variables at the bottom of a
visibility section.  In test fixture classes, the member variables are typically
protected.

[Marie Janssen, 2015-03-09 16:22:50]

https://codereview.chromium.org/982593002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right):

https://codereview.chromium.org/982593002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:49: error_callback));
On 2015/03/05 04:33:54, Ilya Sherman wrote:
> 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.

Done.

https://codereview.chromium.org/982593002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc (right):

https://codereview.chromium.org/982593002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:129:
BluetoothAdapterProfileChromeOS* profile_ptr_;
On 2015/03/05 04:33:54, Ilya Sherman wrote:
> It's kind of confusing to have both profile_ and profile_ptr_.  It looks like
> tests use either one or the other.  Maybe it makes sense to separate these
sets
> of tests somehow?  Or, maybe the two different profile pointer variables could
> have names that hint more directly at how they are used?

Done.

[Ilya Sherman, 2015-03-09 18:18:51]

Thanks, Michael.  The memory management LGTM now.  I defer to Arman and Scott
for the rest.

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right):

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:36: error_callback);
nit: Looks like the alignment is off here -- error_callback should be aligned
with "base::" of "base::Bind", right?

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc (right):

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:65:
profile_.reset();
Are you calling this explicitly because the order of operations is important? 
If not, please omit the explicit call.

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:129: VLOG(1) <<
"Matched Profile Callback";
nit: You probably don't actually want log statements in tests, unless they're
LOG(ERROR) statements that you're temporarily including to debug an issue that
you can only reproduce on the bots.  By default, tests should be silent, so that
test output is easier to scan.  (This applies throughout)

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:160:
BluetoothAdapterProfileChromeOS* profile_user_ptr_;
IMO, it would be nice to document these, to clarify the difference between them,
as well as the memory ownership expectations for the profile_user_ptr_ variable.
 I defer to Arman and the other owners for this code, though.

[Marie Janssen, 2015-03-09 18:38:40]

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right):

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:36: error_callback);
On 2015/03/09 18:18:51, Ilya Sherman wrote:
> nit: Looks like the alignment is off here -- error_callback should be aligned
> with "base::" of "base::Bind", right?

Done.

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc (right):

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:65:
profile_.reset();
On 2015/03/09 18:18:51, Ilya Sherman wrote:
> Are you calling this explicitly because the order of operations is important? 
> If not, please omit the explicit call.

Order is important here, the profile should be released before DBus is shutdown.

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:129: VLOG(1) <<
"Matched Profile Callback";
On 2015/03/09 18:18:51, Ilya Sherman wrote:
> nit: You probably don't actually want log statements in tests, unless they're
> LOG(ERROR) statements that you're temporarily including to debug an issue that
> you can only reproduce on the bots.  By default, tests should be silent, so
that
> test output is easier to scan.  (This applies throughout)

Done.

https://codereview.chromium.org/982593002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:160:
BluetoothAdapterProfileChromeOS* profile_user_ptr_;
On 2015/03/09 18:18:51, Ilya Sherman wrote:
> IMO, it would be nice to document these, to clarify the difference between
them,
> as well as the memory ownership expectations for the profile_user_ptr_
variable.
>  I defer to Arman and the other owners for this code, though.

Done.

[armansito, 2015-03-09 19:14:37]

lgtm

[Marie Janssen, 2015-03-09 19:14:53]

The CQ bit was checked by jamuraa@chromium.org

[Marie Janssen, 2015-03-09 19:14:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/982593002/#ps60001 (title:
"address nits, remove test debug logging")

[commit-bot: I haz the power, 2015-03-09 19:15:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/982593002/60001

[commit-bot: I haz the power, 2015-03-09 20:20:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-09 20:21:25]

Patchset 4 (id:??) landed as
https://crrev.com/202fef401ecfdef89837e709847f4e2189f0ee31
Cr-Commit-Position: refs/heads/master@{#319721}