Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13983004:
Simplified BluetoothDeviceMac. (Closed)
|
Created:
7 years, 8 months ago by youngki Modified:
7 years, 8 months ago CC:
chromium-reviews, sail+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSimplified BluetoothDeviceMac.
BluetoothDeviceMac holds IOBluetoothDevice, an instance to a single remote Bluetooth device in OSX. BluetoothDeviceMac will use IOBluetoothDevice instance to run all the device-related functions instead of using cached data.
Also fixed rfcomm_channel bug in BluetoothSocketMac.
BUG=229636
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194020
Patch Set 1 #Patch Set 2 : Add comments. #
Total comments: 15
Patch Set 3 : Added DCHECK. #Patch Set 4 : set delegate to nil. #
Messages
Total messages: 10 (0 generated)
Mark, could you review this for obj-c? Thanks!
https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... File device/bluetooth/bluetooth_device_mac.mm (right): https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:42: std::string numbers_only = uuid; DCHECK that the string has the right length, and that it has dashes in the expected spots? https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:43: numbers_only.erase(8, 1); It’s a tiny and pointless bit less memmoving if you do 20-16-12-8. https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:48: base::HexStringToBytes(numbers_only, &uuid_bytes_vector); …DCHECK that uuid_bytes_vector is the expected size. https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:49: uint8 uuid_bytes[16]; Why the separate array and the copy? Why not uuidWithBytes:&uuid_bytes_vector[0] length:uuid_bytes_vector.size() ? https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:97: // TODO(youngki): BluetoothServiceRecord is deprecated; implement it without “it” seems to refer to BluetoothServiceRecord here, but you’re obviously didn’t mean to say “implement BluetoothServiceRecord without using BluetoothServiceRecord.” https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... File device/bluetooth/bluetooth_socket_mac.mm (right): https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_socket_mac.mm:56: [rfcomm_channel_ closeChannel]; Does IOBluetoothRFCOMMChannel release its delegate without having retained it? I wouldn’t expect it to. This looks like a leak of BluetoothRFCOMMChannelDelegate now. https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_socket_mac.mm:56: [rfcomm_channel_ closeChannel]; How does rfcomm_channel_ get released now?
https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... File device/bluetooth/bluetooth_device_mac.mm (right): https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:42: std::string numbers_only = uuid; On 2013/04/11 21:48:58, Mark Mentovai wrote: > DCHECK that the string has the right length, and that it has dashes in the > expected spots? Done. https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:43: numbers_only.erase(8, 1); On 2013/04/11 21:48:58, Mark Mentovai wrote: > It’s a tiny and pointless bit less memmoving if you do 20-16-12-8. Done. https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:48: base::HexStringToBytes(numbers_only, &uuid_bytes_vector); On 2013/04/11 21:48:58, Mark Mentovai wrote: > …DCHECK that uuid_bytes_vector is the expected size. Done. https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:49: uint8 uuid_bytes[16]; On 2013/04/11 21:48:58, Mark Mentovai wrote: > Why the separate array and the copy? Why not uuidWithBytes:&uuid_bytes_vector[0] > length:uuid_bytes_vector.size() ? Done. https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:97: // TODO(youngki): BluetoothServiceRecord is deprecated; implement it without On 2013/04/11 21:48:58, Mark Mentovai wrote: > “it” seems to refer to BluetoothServiceRecord here, but you’re obviously didn’t > mean to say “implement BluetoothServiceRecord without using > BluetoothServiceRecord.” Changed 'it' to 'this method'. https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... File device/bluetooth/bluetooth_socket_mac.mm (right): https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_socket_mac.mm:56: [rfcomm_channel_ closeChannel]; On 2013/04/11 21:48:58, Mark Mentovai wrote: > How does rfcomm_channel_ get released now? So.. I did a bunch of testing with a bluetooth app, and if I put [rfcomm_channel_ closeChannel]; [rfcomm_channel_ release]; [delegate_ release]; altogether, the binary crashes, throwing errors like: Backtrace from -dealloc: 0 Chromium Framework 0x052161f2 (anonymous namespace)::ZombieDealloc(objc_object*, objc_selector*) + 722 1 libobjc.A.dylib 0x9565d54e _objc_rootRelease + 47 2 Chromium Framework 0x040893c0 device::BluetoothSocketMac::~BluetoothSocketMac() + 128 3 Chromium Framework 0x0408931b device::BluetoothSocketMac::~BluetoothSocketMac() + 43 4 Chromium Framework 0x040892be device::BluetoothSocketMac::~BluetoothSocketMac() + 46 5 Chromium Framework 0x0408a34e base::RefCounted<device::BluetoothSocket>::Release() const + 94 6 Chromium Framework 0x04086435 scoped_refptr<device::BluetoothSocket>::~scoped_refptr() + 69 7 Chromium Framework 0x0408638b scoped_refptr<device::BluetoothSocket>::~scoped_refptr() + 43 I also tried retain & release, and it still leads to the crash with the same backtrace. I couldn't find a clear documentation what's going on in [rfcomm_channel_ closeChannel], but my guess by looking at the backtrace is that calling closeChannel effectively releases rfcomm_channel_ and also its delegate as well, and releasing rfcomm_channel_ and delegate_ again leads to the above crash.
https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... File device/bluetooth/bluetooth_socket_mac.mm (right): https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_socket_mac.mm:56: [rfcomm_channel_ closeChannel]; youngki wrote: > On 2013/04/11 21:48:58, Mark Mentovai wrote: > > How does rfcomm_channel_ get released now? > > So.. I did a bunch of testing with a bluetooth app, and if I put > > [rfcomm_channel_ closeChannel]; > [rfcomm_channel_ release]; > [delegate_ release]; > > altogether, the binary crashes, throwing errors like: > > Backtrace from -dealloc: > 0 Chromium Framework 0x052161f2 (anonymous > namespace)::ZombieDealloc(objc_object*, objc_selector*) + 722 > 1 libobjc.A.dylib 0x9565d54e _objc_rootRelease + 47 > 2 Chromium Framework 0x040893c0 > device::BluetoothSocketMac::~BluetoothSocketMac() + 128 > 3 Chromium Framework 0x0408931b > device::BluetoothSocketMac::~BluetoothSocketMac() + 43 > 4 Chromium Framework 0x040892be > device::BluetoothSocketMac::~BluetoothSocketMac() + 46 > 5 Chromium Framework 0x0408a34e > base::RefCounted<device::BluetoothSocket>::Release() const + 94 > 6 Chromium Framework 0x04086435 > scoped_refptr<device::BluetoothSocket>::~scoped_refptr() + 69 > 7 Chromium Framework 0x0408638b > scoped_refptr<device::BluetoothSocket>::~scoped_refptr() + 43 > > I also tried retain & release, and it still leads to the crash with the same > backtrace. > > I couldn't find a clear documentation what's going on in [rfcomm_channel_ > closeChannel], but my guess by looking at the backtrace is that calling > closeChannel effectively releases rfcomm_channel_ and also its delegate as well, > and releasing rfcomm_channel_ and delegate_ again leads to the above crash. You need to do some more work to convince me (and yourself) that the IOBluetoothRFCOMMChannel and BluetoothRFCOMMChannelDelegate objects aren’t being leaked. I can appreciate that the documentation is frustratingly incomplete, but that doesn’t absolve you of the duty to get this right. “I guess” isn’t good enough, even with a backtrace from a crash, because you’ve only shown that you can avoid a use-after-free crash, but you’ve shown no evidence of avoiding a leak. Observing -dealloc being called 1-to-1 for each object of these classes that’s allocated would be the bare minimum that you would need to see. Being able to trace a path from -closeChannel to -release and then observing the delegate’s -release being called would be even better. As written now in this change, it looks like at least one of these objects is being leaked, and perhaps both are. For example, while it’s unconventional for a class to release its own delegate, it’s not unheard of, but releasing its own delegate without ever calling retain on the delegate when it was set would be very far outside of ordinary behavior, yet that’s exactly what your code as written assumes is happening. The bar is intentionally very high when the code seemingly makes assumptions that defy convention this substantially.
https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... File device/bluetooth/bluetooth_socket_mac.mm (right): https://chromiumcodereview.appspot.com/13983004/diff/2001/device/bluetooth/bl... device/bluetooth/bluetooth_socket_mac.mm:56: [rfcomm_channel_ closeChannel]; On 2013/04/11 22:24:37, Mark Mentovai wrote: > youngki wrote: > > On 2013/04/11 21:48:58, Mark Mentovai wrote: > > > How does rfcomm_channel_ get released now? > > > > So.. I did a bunch of testing with a bluetooth app, and if I put > > > > [rfcomm_channel_ closeChannel]; > > [rfcomm_channel_ release]; > > [delegate_ release]; > > > > altogether, the binary crashes, throwing errors like: > > > > Backtrace from -dealloc: > > 0 Chromium Framework 0x052161f2 (anonymous > > namespace)::ZombieDealloc(objc_object*, objc_selector*) + 722 > > 1 libobjc.A.dylib 0x9565d54e _objc_rootRelease + 47 > > 2 Chromium Framework 0x040893c0 > > device::BluetoothSocketMac::~BluetoothSocketMac() + 128 > > 3 Chromium Framework 0x0408931b > > device::BluetoothSocketMac::~BluetoothSocketMac() + 43 > > 4 Chromium Framework 0x040892be > > device::BluetoothSocketMac::~BluetoothSocketMac() + 46 > > 5 Chromium Framework 0x0408a34e > > base::RefCounted<device::BluetoothSocket>::Release() const + 94 > > 6 Chromium Framework 0x04086435 > > scoped_refptr<device::BluetoothSocket>::~scoped_refptr() + 69 > > 7 Chromium Framework 0x0408638b > > scoped_refptr<device::BluetoothSocket>::~scoped_refptr() + 43 > > > > I also tried retain & release, and it still leads to the crash with the same > > backtrace. > > > > I couldn't find a clear documentation what's going on in [rfcomm_channel_ > > closeChannel], but my guess by looking at the backtrace is that calling > > closeChannel effectively releases rfcomm_channel_ and also its delegate as > well, > > and releasing rfcomm_channel_ and delegate_ again leads to the above crash. > > You need to do some more work to convince me (and yourself) that the > IOBluetoothRFCOMMChannel and BluetoothRFCOMMChannelDelegate objects aren’t being > leaked. I can appreciate that the documentation is frustratingly incomplete, but > that doesn’t absolve you of the duty to get this right. “I guess” isn’t good > enough, even with a backtrace from a crash, because you’ve only shown that you > can avoid a use-after-free crash, but you’ve shown no evidence of avoiding a > leak. Observing -dealloc being called 1-to-1 for each object of these classes > that’s allocated would be the bare minimum that you would need to see. Being > able to trace a path from -closeChannel to -release and then observing the > delegate’s -release being called would be even better. > > As written now in this change, it looks like at least one of these objects is > being leaked, and perhaps both are. For example, while it’s unconventional for a > class to release its own delegate, it’s not unheard of, but releasing its own > delegate without ever calling retain on the delegate when it was set would be > very far outside of ordinary behavior, yet that’s exactly what your code as > written assumes is happening. The bar is intentionally very high when the code > seemingly makes assumptions that defy convention this substantially. Thanks for pointing this out; and you are right, I debugged this further and actually realized that rfcomm_channel_ wasn't deallocating the delegate; and the cause of the crash was that rfcomm_channel_ was trying to call channelClosed delegate method, when delegate_ no longer exists because BluetoothSocketMac released it. I set the delegate to nil before closing the channel since at the socket destruction we are not interested in listening to the channel event anyways.
LGTM
Thanks for the review, Mark. Scott, could you take a look?
lgtm glad that this got equivalently simplified too
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/youngki@chromium.org/13983004/16001
Message was sent while issue was closed.
Change committed as 194020 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
