Simplified BluetoothDeviceMac.

device/bluetooth/bluetooth_socket_mac.mm

 // 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_socket_mac.h"
 
 #import <IOBluetooth/objc/IOBluetoothDevice.h>
 #import <IOBluetooth/objc/IOBluetoothRFCOMMChannel.h>
 
 #include <limits>
@@ skipping 28 matching lines @@
                      data:(void*)dataPointer
                    length:(size_t)dataLength {
   socket_->OnDataReceived(rfcommChannel, dataPointer, dataLength);
 }
 
 @end
 
 namespace device {
 
 BluetoothSocketMac::BluetoothSocketMac(IOBluetoothRFCOMMChannel* rfcomm_channel)
-    : rfcomm_channel_(rfcomm_channel),
-      delegate_([[BluetoothRFCOMMChannelDelegate alloc] initWithSocket:this]) {
-  [rfcomm_channel_ setDelegate:delegate_];
+    : rfcomm_channel_(rfcomm_channel) {
+  [rfcomm_channel_ setDelegate:[[BluetoothRFCOMMChannelDelegate alloc]
+      initWithSocket:this]];
   ResetIncomingDataBuffer();
 }
 
 BluetoothSocketMac::~BluetoothSocketMac() {
-  [rfcomm_channel_ release];
-  [delegate_ release];
+  [rfcomm_channel_ closeChannel];

[Mark Mentovai, 2013/04/11 21:48:58]

Does IOBluetoothRFCOMMChannel release its delegate without having retained it? I
wouldn’t expect it to. This looks like a leak of BluetoothRFCOMMChannelDelegate
now.

[Mark Mentovai, 2013/04/11 21:48:58]

How does rfcomm_channel_ get released now?

[youngki, 2013/04/11 22:14:29]

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.

[Mark Mentovai, 2013/04/11 22:24:37]

youngki wrote:
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.

[youngki, 2013/04/12 01:51:47]

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.

 }
 
 // static
 scoped_refptr<BluetoothSocket> BluetoothSocketMac::CreateBluetoothSocket(
     const BluetoothServiceRecord& service_record) {
   BluetoothSocketMac* bluetooth_socket = NULL;
   if (service_record.SupportsRfcomm()) {
     const BluetoothServiceRecordMac* service_record_mac =
         static_cast<const BluetoothServiceRecordMac*>(&service_record);
     IOBluetoothDevice* device = service_record_mac->GetIOBluetoothDevice();
@@ skipping 64 matching lines @@
   incoming_data_buffer_->set_offset(
       incoming_data_buffer_->offset() + data_size);
 }
 
 void BluetoothSocketMac::ResetIncomingDataBuffer() {
   incoming_data_buffer_ = new net::GrowableIOBuffer();
   incoming_data_buffer_->SetCapacity(1024);
 }
 
 }  // namespace device