Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
935383003:
Fix BluetoothAdapterProfileChromeOS lifecycle management (Closed)
|
Created:
5 years, 10 months ago by Marie Janssen Modified:
5 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix BluetoothAdapterProfileChromeOS lifecycle management
Have BluetoothAdapterProfileChromeOS remove itself from adapters when it
is deleted via it's deconstructor, to avoid references being left behind
after deletion.
BUG=457978
Committed: https://crrev.com/0cca591aaf6f6a0cbf46840faaead236dd8317a2
Cr-Commit-Position: refs/heads/master@{#318925}
Patch Set 1 #
Total comments: 9
Patch Set 2 : further fix ownership, address comments #
Total comments: 21
Patch Set 3 : address nits / comments #Patch Set 4 : add DCHECK confirming profiles are gone during shutdown #
Total comments: 10
Patch Set 5 : address comments, fix simultaneous useprofile #
Total comments: 8
Patch Set 6 : address comments from armansito #Patch Set 7 : rebase, fix call-after-shutdown problems #Patch Set 8 : fix for leak detected by LSAN #Messages
Total messages: 36 (9 generated)
jamuraa@chromium.org changed reviewers: + armansito@chromium.org, isherman@chromium.org, keybuk@chromium.org
Here's a fix which should eliminate these lifecycle bugs that are coming up surrounding BluetoothAdapterProfileChromeOS / BluetoothAdapterChromeOS. I've tested this on a falco and it fixes the bug, and you can still setup smart lock after failing numerous times (because your device is in airplane mode).
Thanks for working on this! The ownership semantics for the profiles are super unclear to me. It would be very helpful to have them explicitly documented somewhere. Even better would be to use smart pointers to control their lifetimes, so that there is no risk of memory leaks or double-frees. Also, I'm concerned about the apparent weak pointer bug that I identified below. Is the case where |adapter_| would be null not exercised by tests? It would be really helpful to add test coverage, if that's the case. https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:975: if (profiles_.find(uuid) != profiles_.end()) { nit: I don't think you need the if stmt at all anymore -- erase() should be safe to call even if the key isn't present. (Though, I'm not 100% sure on this, so please check that I'm not telling you lies =) https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:976: profiles_.erase(uuid); What code is responsible for freeing the memory for the profile, in this case? The ownership semantics for profiles are not clear to me. https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:1013: delete profiles_[uuid]; Are you sure it's appropriate to do this *before* clients have a chance to handle the error callback? https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_profile_chromeos.cc:57: adapter_->RemoveProfile(uuid_); Using a weak pointer doesn't prevent this line from crashing. You still have to explicitly null-check the profile. https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_profile_chromeos.cc:58: profile_.reset(); This line should not be necessary in the destructor -- the scoped_ptr will reset itself as it goes out of scope anyway.
Updated CL with a rework of ownership for BluetoothAdapterProfileChromeOS which simplifies things significantly. Added a unit test for the behavior too, for good measure. Tested against the bug behavior on a falco and everything is good. PTAL. https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:975: if (profiles_.find(uuid) != profiles_.end()) { On 2015/02/19 20:37:58, Ilya Sherman wrote: > nit: I don't think you need the if stmt at all anymore -- erase() should be safe > to call even if the key isn't present. (Though, I'm not 100% sure on this, so > please check that I'm not telling you lies =) Done. https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:976: profiles_.erase(uuid); On 2015/02/19 20:37:58, Ilya Sherman wrote: > What code is responsible for freeing the memory for the profile, in this case? > The ownership semantics for profiles are not clear to me. Profiles are now owned by BluetoothAdapterChromeOS and released through there. https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:1013: delete profiles_[uuid]; On 2015/02/19 20:37:58, Ilya Sherman wrote: > Are you sure it's appropriate to do this *before* clients have a chance to > handle the error callback? Sockets don't get this pointer until the RegisterProfile callback completes, so this is okay here. https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_profile_chromeos.cc:58: profile_.reset(); On 2015/02/19 20:37:58, Ilya Sherman wrote: > This line should not be necessary in the destructor -- the scoped_ptr will reset > itself as it goes out of scope anyway. After refactoring the ownership, these aren't needed here so we should be good.
Thanks, this looks much better! I still have some more minor questions about the memory model, but I think this is pretty close to a good state. Note that I'm not an owner of the code, so you'll need Scott or Arman's review as well. https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... device/bluetooth/bluetooth_adapter_chromeos.cc:131: } 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.) https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... device/bluetooth/bluetooth_adapter_chromeos.cc:979: weak_ptr_factory_.GetWeakPtr(), profile->uuid())); nit: Is this what git cl format produces? The formatting looks a little odd to me. https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... File device/bluetooth/bluetooth_adapter_chromeos.h (right): https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... device/bluetooth/bluetooth_adapter_chromeos.h:141: // Release use of a profile by a device nit: Please end the sentence with a period. https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... device/bluetooth/bluetooth_adapter_chromeos.h:287: // Called by BluetoothAdapterProfileChromeOS when there no users of a profile nit: s/there// https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... File device/bluetooth/bluetooth_adapter_profile_chromeos.h (right): https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... device/bluetooth/bluetooth_adapter_profile_chromeos.h:33: static BluetoothAdapterProfileChromeOS* Register( Optional: It would be nice if this returned a scoped_ptr, rather than a raw pointer. That would help clarify ownership semantics a bit, and possibly obviate the need for the ownership comment you added, since it would be clear that ownership is being transferred to whoever calls Register(). In general, smart pointers are the way to go whenever possible, for clarifying ownership semantics. https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... File device/bluetooth/bluetooth_socket_chromeos.cc (right): https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... device/bluetooth/bluetooth_socket_chromeos.cc:225: DCHECK(adapter); 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? https://chromiumcodereview.appspot.com/935383003/diff/20001/device/bluetooth/... device/bluetooth/bluetooth_socket_chromeos.cc:554: ->ReleaseProfile(device_path_, profile_); Is it guaranteed that the adapter_ is non-null? I'm asking because the code previously had the guard "if (adapter_) ..."
Updated the CL for these comments. Thanks! https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:131: } On 2015/02/21 01:22:10, Ilya Sherman wrote: > 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.) 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. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:979: weak_ptr_factory_.GetWeakPtr(), profile->uuid())); On 2015/02/21 01:22:10, Ilya Sherman wrote: > nit: Is this what git cl format produces? The formatting looks a little odd to > me. 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. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.h (right): https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.h:141: // Release use of a profile by a device On 2015/02/21 01:22:10, Ilya Sherman wrote: > nit: Please end the sentence with a period. Done. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.h:287: // Called by BluetoothAdapterProfileChromeOS when there no users of a profile On 2015/02/21 01:22:10, Ilya Sherman wrote: > nit: s/there// Done. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_socket_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.cc:225: DCHECK(adapter); On 2015/02/21 01:22:10, Ilya Sherman wrote: > 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? we don't use adapter_ in this function anymore. this is just reverting the places adapter_ is set to the way it was before. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.cc:554: ->ReleaseProfile(device_path_, profile_); On 2015/02/21 01:22:10, Ilya Sherman wrote: > Is it guaranteed that the adapter_ is non-null? I'm asking because the code > previously had the guard "if (adapter_) ..." 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.
Thanks, Michael. I think this looks pretty good at this point. I still have questions because I don't know the code super well, and the memory ownership model is still a little bit more manual than I'd like... but I think at this point Scott or Arman need to review. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:131: } On 2015/02/23 17:49:09, Michael Janssen wrote: > On 2015/02/21 01:22:10, Ilya Sherman wrote: > > 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.) > > All profiles will get unregistered (and subsequently deleted) when the adapter > gets destructed through PresentChanged(false). 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? > 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. Yeah, scoped_ptr isn't quite as smart as unique_ptr. Thanks for checking =) https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_socket_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.cc:225: DCHECK(adapter); On 2015/02/23 17:49:10, Michael Janssen wrote: > On 2015/02/21 01:22:10, Ilya Sherman wrote: > > 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? > > we don't use adapter_ in this function anymore. this is just reverting the > places adapter_ is set to the way it was before. 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? https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.cc:554: ->ReleaseProfile(device_path_, profile_); On 2015/02/23 17:49:09, Michael Janssen wrote: > On 2015/02/21 01:22:10, Ilya Sherman wrote: > > Is it guaranteed that the adapter_ is non-null? I'm asking because the code > > previously had the guard "if (adapter_) ..." > > 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. 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_) ..."?
https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:131: } On 2015/02/24 05:26:06, Ilya Sherman wrote: > On 2015/02/23 17:49:09, Michael Janssen wrote: > > On 2015/02/21 01:22:10, Ilya Sherman wrote: > > > 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.) > > > > All profiles will get unregistered (and subsequently deleted) when the adapter > > gets destructed through PresentChanged(false). > > 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? 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. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_socket_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.cc:225: DCHECK(adapter); On 2015/02/24 05:26:06, Ilya Sherman wrote: > On 2015/02/23 17:49:10, Michael Janssen wrote: > > On 2015/02/21 01:22:10, Ilya Sherman wrote: > > > 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? > > > > we don't use adapter_ in this function anymore. this is just reverting the > > places adapter_ is set to the way it was before. > > 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? I mean from before this CL. Earlier patches moved the assignment, later patches switch it back to the original way. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.cc:554: ->ReleaseProfile(device_path_, profile_); On 2015/02/24 05:26:06, Ilya Sherman wrote: > On 2015/02/23 17:49:09, Michael Janssen wrote: > > On 2015/02/21 01:22:10, Ilya Sherman wrote: > > > Is it guaranteed that the adapter_ is non-null? I'm asking because the code > > > previously had the guard "if (adapter_) ..." > > > > 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. > > 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_) ..."? 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()).
LGTM, thanks. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_socket_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.cc:225: DCHECK(adapter); On 2015/02/24 18:49:21, Michael Janssen wrote: > On 2015/02/24 05:26:06, Ilya Sherman wrote: > > On 2015/02/23 17:49:10, Michael Janssen wrote: > > > On 2015/02/21 01:22:10, Ilya Sherman wrote: > > > > 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? > > > > > > we don't use adapter_ in this function anymore. this is just reverting the > > > places adapter_ is set to the way it was before. > > > > 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? > > I mean from before this CL. Earlier patches moved the assignment, later patches > switch it back to the original way. 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. https://codereview.chromium.org/935383003/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.cc:554: ->ReleaseProfile(device_path_, profile_); On 2015/02/24 18:49:21, Michael Janssen wrote: > On 2015/02/24 05:26:06, Ilya Sherman wrote: > > On 2015/02/23 17:49:09, Michael Janssen wrote: > > > On 2015/02/21 01:22:10, Ilya Sherman wrote: > > > > Is it guaranteed that the adapter_ is non-null? I'm asking because the > code > > > > previously had the guard "if (adapter_) ..." > > > > > > 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. > > > > 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_) ..."? > > 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()). Ok, that makes sense -- thanks!
https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:88: DCHECK(profiles_.empty()); Is this because RemoveAdapter clears |profiles_|? If so, add a note above ("// Cleans up |devices_| and |profiles_|.") https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:977: BluetoothAdapterProfileChromeOS* profile) { Add VLOG https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:1020: LOG(WARNING) << object_path_.value() Change this to VLOG https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:1024: profiles_.erase(uuid); 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. https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_socket_chromeos.h (right): https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.h:151: // Unregister use of the profile. Maybe elaborate more? What does it mean to unregister "use of" the profile?
Addressed comments by armansito@ https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:88: DCHECK(profiles_.empty()); On 2015/02/24 23:32:51, armansito wrote: > Is this because RemoveAdapter clears |profiles_|? If so, add a note above ("// > Cleans up |devices_| and |profiles_|.") RemoveAdapter() signals the profiles through PresentChanged(false), which should clean up all the profiles_. Added a clarifying comment. https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:977: BluetoothAdapterProfileChromeOS* profile) { On 2015/02/24 23:32:51, armansito wrote: > Add VLOG Done. https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:1020: LOG(WARNING) << object_path_.value() On 2015/02/24 23:32:51, armansito wrote: > Change this to VLOG Done. https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:1024: profiles_.erase(uuid); On 2015/02/24 23:32:51, armansito wrote: > 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. 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. https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_socket_chromeos.h (right): https://codereview.chromium.org/935383003/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_socket_chromeos.h:151: // Unregister use of the profile. On 2015/02/24 23:32:51, armansito wrote: > Maybe elaborate more? What does it mean to unregister "use of" the profile? Done.
Mostly nits. https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:90: DCHECK(profiles_.empty()); Do you need a DCHECK for |profile_queues_| being empty? Either way you should clean it up either here or in the class destructor. https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:1035: } nit: no need for curly braces in single-line if body. https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_profile_chromeos.cc:33: base::Bind(success_callback, profile), error_callback); I think it would be better formatting to put each argument in its own line. https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc (right): https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:129: ASSERT_TRUE(profile == profile_); nit: ASSERT_EQ
https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:90: DCHECK(profiles_.empty()); On 2015/02/27 21:04:57, armansito wrote: > Do you need a DCHECK for |profile_queues_| being empty? Either way you should > clean it up either here or in the class destructor. Done. https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:1035: } On 2015/02/27 21:04:57, armansito wrote: > nit: no need for curly braces in single-line if body. Done. https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right): https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_profile_chromeos.cc:33: base::Bind(success_callback, profile), error_callback); On 2015/02/27 21:04:57, armansito wrote: > I think it would be better formatting to put each argument in its own line. Done. https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc (right): https://codereview.chromium.org/935383003/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:129: ASSERT_TRUE(profile == profile_); On 2015/02/27 21:04:57, armansito wrote: > nit: ASSERT_EQ Done.
lgtm
The CQ bit was checked by jamuraa@chromium.org
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/935383003/#ps100001 (title: "address comments from armansito")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/935383003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jamuraa@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/935383003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Had to rebase and fix a few minor things related to functions being called after Shutdown() of the Adapter. PTAL.
The CQ bit was checked by jamuraa@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from armansito@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/935383003/#ps120001 (title: "rebase, fix call-after-shutdown problems")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/935383003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0cca591aaf6f6a0cbf46840faaead236dd8317a2 Cr-Commit-Position: refs/heads/master@{#318925}
Message was sent while issue was closed.
On 2015/03/02 22:51:18, Michael Janssen wrote: > Had to rebase and fix a few minor things related to functions being called after > Shutdown() of the Adapter. PTAL. Hmm, did Arman look over these changes and approve them? I looked at them but did not know enough to determine whether they were appropriate or not. Apologies for not posting a comment indicating this on the CL.
Message was sent while issue was closed.
On 2015/03/03 20:49:30, Ilya Sherman wrote: > On 2015/03/02 22:51:18, Michael Janssen wrote: > > Had to rebase and fix a few minor things related to functions being called > after > > Shutdown() of the Adapter. PTAL. > > Hmm, did Arman look over these changes and approve them? I looked at them but > did not know enough to determine whether they were appropriate or not. > Apologies for not posting a comment indicating this on the CL. Arman said he looked over when I asked him about them this morning, so I went forward.
Message was sent while issue was closed.
On 2015/03/03 21:17:33, Michael Janssen wrote: > On 2015/03/03 20:49:30, Ilya Sherman wrote: > > On 2015/03/02 22:51:18, Michael Janssen wrote: > > > Had to rebase and fix a few minor things related to functions being called > > after > > > Shutdown() of the Adapter. PTAL. > > > > Hmm, did Arman look over these changes and approve them? I looked at them but > > did not know enough to determine whether they were appropriate or not. > > Apologies for not posting a comment indicating this on the CL. > > Arman said he looked over when I asked him about them this morning, so I went > forward. Okay, great! Thanks for confirming =)
Message was sent while issue was closed.
stgao@chromium.org changed reviewers: + stgao@chromium.org
Message was sent while issue was closed.
FYI: I happened to found this CL might cause a breakage. http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...
Message was sent while issue was closed.
On 2015/03/03 23:44:46, Shuotao wrote: > FYI: > > I happened to found this CL might cause a breakage. > http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... I really wish that these kinds of failures reported by Leak Sanitizer would be detected by the CQ bots...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/975323002/ by finnur@chromium.org. The reason for reverting is: Leaks detected. https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... ================================================================= ==22324==ERROR: LeakSanitizer: detected memory leaks Indirect leak of 96 byte(s) in 1 object(s) allocated from: #0 0x4d9b8b in operator new(unsigned long) (/mnt/data/b/build/slave/Linux_Chromium_OS_ASan_LSan_Tests__1_/build/src/out/Release/device_unittests+0x4d9b8b) #1 0x901bd6 in chromeos::BluetoothAdapterProfileChromeOS::Register(device::BluetoothUUID const&, chromeos::BluetoothProfileManagerClient::Options const&, base::Callback<void (chromeos::BluetoothAdapterProfileChromeOS*)> const&, base::Callback<void (std::string const&, std::string const&)> const&) device/bluetooth/bluetooth_adapter_profile_chromeos.cc:28:7 #2 0x8f6c47 in chromeos::BluetoothAdapterChromeOS::UseProfile(device::BluetoothUUID const&, dbus::ObjectPath const&, chromeos::BluetoothProfileManagerClient::Options const&, chromeos::BluetoothProfileServiceProvider::Delegate*, base::Callback<void (chromeos::BluetoothAdapterProfileChromeOS*)> const&, base::Callback<void (std::string const&)> const&) device/bluetooth/bluetooth_adapter_chromeos.cc:982:5 #3 0x4ec8e2 in chromeos::BluetoothAdapterProfileChromeOSTest_SimultaneousRegisterFail_Test::TestBody() device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:359:3 #4 0x85da05 in HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2420:12 #5 0x85da05 in testing::Test::Run() testing/gtest/src/gtest.cc:2436 #6 0x85f619 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2612:5 #7 0x86040a in testing::TestCase::Run() testing/gtest/src/gtest.cc:2730:5 #8 0x874063 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4602:11 #9 0x8736b7 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2420:12 #10 0x8736b7 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4220 #11 0x814bda in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2326:10 #12 0x814bda in base::TestSuite::Run() base/test/test_suite.cc:230 #13 0x80ad38 in Run base/callback.h:396:12 #14 0x80ad38 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int ()> const&, int, bool, base::Callback<void ()> const&) base/test/launcher/unit_test_launcher.cc:181 #15 0x80a863 in base::LaunchUnitTests(int, char**, base::Callback<int ()> const&) base/test/launcher/unit_test_launcher.cc:423:10 #16 0x7d25ac in main device/test/run_all_unittests.cc:14:10 #17 0x7f35c8c7276c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 Indirect leak of 93 byte(s) in 1 object(s) allocated from: #0 0x4d9b8b in operator new(unsigned long) (/mnt/data/b/build/slave/Linux_Chromium_OS_ASan_LSan_Tests__1_/build/src/out/Release/device_unittests+0x4d9b8b) #1 0x7f35c92df739 in __gnu_cxx::new_allocator<char>::allocate(unsigned long, void const*) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/ext/new_allocator.h:92 #2 0x7f35c92ded2c in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609 #3 0x7f35c92def04 in std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:631 #4 0x7f35c92dbc48 in std::string::reserve(unsigned long) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:512 #5 0x9021d7 in operator+<char, std::char_traits<char>, std::allocator<char> > /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/basic_string.tcc:702:7 #6 0x9021d7 in chromeos::BluetoothAdapterProfileChromeOS::BluetoothAdapterProfileChromeOS(device::BluetoothUUID const&) device/bluetooth/bluetooth_adapter_profile_chromeos.cc:47 #7 0x901be8 in chromeos::BluetoothAdapterProfileChromeOS::Register(device::BluetoothUUID const&, chromeos::BluetoothProfileManagerClient::Options const&, base::Callback<void (chromeos::BluetoothAdapterProfileChromeOS*)> const&, base::Callback<void (std::string const&, std::string const&)> const&) device/bluetooth/bluetooth_adapter_profile_chromeos.cc:28:11 #8 0x8f6c47 in chromeos::BluetoothAdapterChromeOS::UseProfile(device::BluetoothUUID const&, dbus::ObjectPath const&, chromeos::BluetoothProfileManagerClient::Options const&, chromeos::BluetoothProfileServiceProvider::Delegate*, base::Callback<void (chromeos::BluetoothAdapterProfileChromeOS*)> const&, base::Callback<void (std::string const&)> const&) device/bluetooth/bluetooth_adapter_chromeos.cc:982:5 #9 0x4ec8e2 in chromeos::BluetoothAdapterProfileChromeOSTest_SimultaneousRegisterFail_Test::TestBody() device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:359:3 #10 0x85da05 in HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2420:12 #11 0x85da05 in testing::Test::Run() testing/gtest/src/gtest.cc:2436 #12 0x85f619 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2612:5 #13 0x86040a in testing::TestCase::Run() testing/gtest/src/gtest.cc:2730:5 #14 0x874063 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4602:11 #15 0x8736b7 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2420:12 #16 0x8736b7 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4220 #17 0x814bda in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2326:10 #18 0x814bda in base::TestSuite::Run() base/test/test_suite.cc:230 #19 0x80ad38 in Run base/callback.h:396:12 #20 0x80ad38 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int ()> const&, int, bool, base::Callback<void ()> const&) base/test/launcher/unit_test_launcher.cc:181 #21 0x80a863 in base::LaunchUnitTests(int, char**, base::Callback<int ()> const&) base/test/launcher/unit_test_launcher.cc:423:10 #22 0x7d25ac in main device/test/run_all_unittests.cc:14:10 #23 0x7f35c8c7276c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 Indirect leak of 24 byte(s) in 1 object(s) allocated from: #0 0x4d9b8b in operator new(unsigned long) (/mnt/data/b/build/slave/Linux_Chromium_OS_ASan_LSan_Tests__1_/build/src/out/Release/device_unittests+0x4d9b8b) #1 0xd9c743 in chromeos::BluetoothProfileServiceProvider::Create(dbus::Bus*, dbus::ObjectPath const&, chromeos::BluetoothProfileServiceProvider::Delegate*) chromeos/dbus/bluetooth_profile_service_provider.cc:251:12 #2 0x90227f in chromeos::BluetoothAdapterProfileChromeOS::BluetoothAdapterProfileChromeOS(device::BluetoothUUID const&) device/bluetooth/bluetooth_adapter_profile_chromeos.cc:51:7 #3 0x901be8 in chromeos::BluetoothAdapterProfileChromeOS::Register(device::BluetoothUUID const&, chromeos::BluetoothProfileManagerClient::Options const&, base::Callback<void (chromeos::BluetoothAdapterProfileChromeOS*)> const&, base::Callback<void (std::string const&, std::string const&)> const&) device/bluetooth/bluetooth_adapter_profile_chromeos.cc:28:11 #4 0x8f6c47 in chromeos::BluetoothAdapterChromeOS::UseProfile(device::BluetoothUUID const&, dbus::ObjectPath const&, chromeos::BluetoothProfileManagerClient::Options const&, chromeos::BluetoothProfileServiceProvider::Delegate*, base::Callback<void (chromeos::BluetoothAdapterProfileChromeOS*)> const&, base::Callback<void (std::string const&)> const&) device/bluetooth/bluetooth_adapter_chromeos.cc:982:5 #5 0x4ec8e2 in chromeos::BluetoothAdapterProfileChromeOSTest_SimultaneousRegisterFail_Test::TestBody() device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:359:3 #6 0x85da05 in HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2420:12 #7 0x85da05 in testing::Test::Run() testing/gtest/src/gtest.cc:2436 #8 0x85f619 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2612:5 #9 0x86040a in testing::TestCase::Run() testing/gtest/src/gtest.cc:2730:5 #10 0x874063 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4602:11 #11 0x8736b7 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2420:12 #12 0x8736b7 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4220 #13 0x814bda in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2326:10 #14 0x814bda in base::TestSuite::Run() base/test/test_suite.cc:230 #15 0x80ad38 in Run base/callback.h:396:12 #16 0x80ad38 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int ()> const&, int, bool, base::Callback<void ()> const&) base/test/launcher/unit_test_launcher.cc:181 #17 0x80a863 in base::LaunchUnitTests(int, char**, base::Callback<int ()> const&) base/test/launcher/unit_test_launcher.cc:423:10 #18 0x7d25ac in main device/test/run_all_unittests.cc:14:10 #19 0x7f35c8c7276c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226. |
