troystribling / BlueCap

iOS Bluetooth LE framework
MIT License
714 stars 114 forks source link

Critical bug #69

Closed JeniaTretiakova-ezlo closed 7 years ago

JeniaTretiakova-ezlo commented 7 years ago

There are wrong logic when called function loadRetrievedPeripheral().

  1. Call func loadRetrievedPeripheral(_ peripheral: CBPeripheralInjectable) -> Peripheral
  2. Created new instance of newBCPeripheral (line 262 CentralManager.swift) and it sets
    self.cbPeripheral.delegate = self (line 203 in Peripheral.swift)
  3. Replaced peripheral (line 262 CentralManager.swift) and self._discoveredPeripherals[peripheral.identifier] = newBCPeripheral and it removes old peripheral and calls deinit (line 208 in Peripheral.swift) BUT it also removes delegate for new peripheral
  4. As result it never calls any of peripheral delegate functions

Way to fix: Peripheral.swift 1) remove deinit { self.cbPeripheral.delegate = nil } 2 ) and replace: var delegate: CBPeripheralDelegate? { get set } with: weak var delegate: CBPeripheralDelegate? { get set }

troystribling commented 7 years ago

I do not understand item 3. If oldBCPeripheral and newBCPeripheral have same identifier will the CBPeripheral delegate be dealloced for newBCPeripheral? I do not see this since the reference to old CBPeripheralwill be held until the reassignment. Are you saying it is released?

JeniaTretiakova-ezlo commented 7 years ago

oldBCPeripheral and newBCPeripheral is two different instance, BUT they both have the same reference to cbPeripheral. So newBCPeripheral sets itself like delegate for self.cbPeripheral, and after it oldBCPeripheral sets delegate to nil for the same instance.

deinit { self.cbPeripheral.delegate = nil } Peripheral.swift(line 207) as result newBCPeripheral has property self.cbPeripheral, but delegate for it has been set to nil.

troystribling commented 7 years ago

I understand the issue now. I think this make be a better fix,

deinit {
   if  cbPeripheral.delegate === self {
      cbPeripheral.delegate = nil
   }
}

I will write a couple of new test cases to verify fix.

Good catch! Thanks.

troystribling commented 7 years ago

I just pushed a fix for this to master. I will do a pod release this weekend.

JeniaTretiakova-ezlo commented 7 years ago

if cbPeripheral.delegate === self { cbPeripheral.delegate = nil }

I think it should work, but I don't like how it seems. Instead of make delegate weak and lets manage memory with Automatic Reference Counting, you handle it yourself and added some extra code.

troystribling commented 7 years ago

The CBPeripheralDelegate is a property on CBPeripheral I cannot change it.