Bluetooth: macOS: Adding counter for service discovery callbacks.

device/bluetooth/bluetooth_low_energy_device_mac.mm

Index: device/bluetooth/bluetooth_low_energy_device_mac.mm
diff --git a/device/bluetooth/bluetooth_low_energy_device_mac.mm b/device/bluetooth/bluetooth_low_energy_device_mac.mm
index a52d2bcf618f00e0e6c18768ed6259e400a02219..00c1475926ffb1b9d82458f2b7fd761f8f0eb80a 100644
--- a/device/bluetooth/bluetooth_low_energy_device_mac.mm
+++ b/device/bluetooth/bluetooth_low_energy_device_mac.mm
@@ -25,7 +25,8 @@ BluetoothLowEnergyDeviceMac::BluetoothLowEnergyDeviceMac(
     BluetoothAdapterMac* adapter,
     CBPeripheral* peripheral)
     : BluetoothDeviceMac(adapter),
-      peripheral_(peripheral, base::scoped_policy::RETAIN) {
+      peripheral_(peripheral, base::scoped_policy::RETAIN),
+      discovery_pending_count_(0) {
   DCHECK(BluetoothAdapterMac::IsLowEnergyAvailable());
   DCHECK(peripheral_.get());
   peripheral_delegate_.reset([[BluetoothLowEnergyPeripheralDelegate alloc]
@@ -193,7 +194,8 @@ void BluetoothLowEnergyDeviceMac::DidDiscoverPrimaryServices(NSError* error) {
             << ": " << error.code << ")";
     return;
   }
-  VLOG(1) << "DidDiscoverPrimaryServices.";
+  VLOG(1) << "DidDiscoverPrimaryServices, pending count: "
+          << discovery_pending_count_;
 
   if (!IsGattConnected()) {
     // Don't create services if the device disconnected.
@@ -212,11 +214,16 @@ void BluetoothLowEnergyDeviceMac::DidDiscoverPrimaryServices(NSError* error) {
       adapter_->NotifyGattServiceAdded(gatt_service);
     }
   }
-  for (auto it = gatt_services_.begin(); it != gatt_services_.end(); ++it) {
-    device::BluetoothRemoteGattService* gatt_service = it->second.get();
-    device::BluetoothRemoteGattServiceMac* gatt_service_mac =
-        static_cast<BluetoothRemoteGattServiceMac*>(gatt_service);
-    gatt_service_mac->DiscoverCharacteristics();
+  --discovery_pending_count_;
+  DCHECK(discovery_pending_count_ >= 0);

[ortuno, 2017/01/22 20:39:14]

Looking at the code it seems we are calling DiscoverCharacteristics twice
because we are getting two calls to DidDiscoverPrimaryServices. Is that correct?
If so then I think this patch introduces another DCHECK that will fail:
DCHECK(discovery_pending_count_ >= 0);:

1. DiscoverPrimaryServices(); // discovery_pending_count_ is now 1
2. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now 0.
3. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -1 and
DCHECK fails.

[jlebel, 2017/01/22 21:23:33]

discovery_pending_count_ should never be -1. Do you want me to change the line
219 to:
if (discovery_pending_count_ <= 0) {

[ortuno, 2017/01/23 00:18:23]

I'm not sure that solves the problem. For example:

1. DiscoverPrimaryServices(); // discovery_pending_count_ is now 1
2. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now 0.
3. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -1 and
DCHECK fails.
// Device disconnects and connects again.
4. DiscoverPrimaryServices(); // discovery_pending_count_ is now 0.
5. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -1 and
DiscoverCharacteristics is called.
6. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -2 and
DiscoverCharacteristics is called again.

What about just using a boolean and checking it at the beginning of the method:

DiscoverPrimaryServices() {
  pending_services_discovery_ = true;
  // initiate service discovery.
}

DidDiscoverPrimaryServices() {
  // Check for error and print
  if (!pending_services_discovery_)
    LOG(WARNING) << "Incorrect DidDiscoverPrimaryServices call."
    return;

  pending_services_discovery_ = false;
  // ...
}

+  if (discovery_pending_count_ == 0) {
+    for (auto it = gatt_services_.begin(); it != gatt_services_.end(); ++it) {
+      device::BluetoothRemoteGattService* gatt_service = it->second.get();
+      device::BluetoothRemoteGattServiceMac* gatt_service_mac =
+          static_cast<BluetoothRemoteGattServiceMac*>(gatt_service);
+      gatt_service_mac->DiscoverCharacteristics();
+    }
+    SendNotificationIfDiscoveryComplete();
   }
 }
 
@@ -261,7 +268,7 @@ void BluetoothLowEnergyDeviceMac::DidModifyServices(
   device_uuids_.ClearServiceUUIDs();
   SetGattServicesDiscoveryComplete(false);
   adapter_->NotifyDeviceChanged(this);
-  [GetPeripheral() discoverServices:nil];
+  DiscoverPrimaryServices();
 }
 
 void BluetoothLowEnergyDeviceMac::DidUpdateValue(
@@ -337,9 +344,16 @@ std::string BluetoothLowEnergyDeviceMac::GetPeripheralHashAddress(
   return BluetoothDevice::CanonicalizeAddress(hash);
 }
 
+void BluetoothLowEnergyDeviceMac::DiscoverPrimaryServices() {
+  [GetPeripheral() discoverServices:nil];
+  VLOG(1) << "DidDiscoverDescriptors pending count" << discovery_pending_count_;
+  ++discovery_pending_count_;
+}
+
 void BluetoothLowEnergyDeviceMac::SendNotificationIfDiscoveryComplete() {
   // Notify when all services have been discovered.
   bool discovery_complete =
+      discovery_pending_count_ == 0 &&
       std::find_if_not(
           gatt_services_.begin(), gatt_services_.end(),
           [](GattServiceMap::value_type& pair) {