Fix broken timeout handling in osxbtcentral manager

Trying to do several things (for example, discovering chars on
several services) in non-sequential manner is allowed but will
result in the broken 'object under watch' logic and thus a
failure to report some operation finished.

Task-number: QTBUG-72487
Change-Id: I9674f93e0c4d5cbfd50ac2f828d0d650031e056c
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>

5 files changed, 136 insertions, 101 deletions

diff --git a/src/bluetooth/osx/osxbtcentralmanager.mm b/src/bluetooth/osx/osxbtcentralmanager.mm
index 75d9f5c0..cadabbaf 100644
--- a/src/bluetooth/osx/osxbtcentralmanager.mm
+++ b/src/bluetooth/osx/osxbtcentralmanager.mm
@@ -46,6 +46,7 @@
 #include <QtCore/qdebug.h>
 
 #include <algorithm>
+#include <vector>
 #include <limits>
 
 Q_DECLARE_METATYPE(QLowEnergyHandle)
@@ -92,13 +93,24 @@ ObjCStrongReference<NSError> qt_timeoutNSError(OperationTimeout type)
     return ObjCStrongReference<NSError>(nsError, false /*do not retain, done already*/);
 }
 
+auto qt_find_watchdog(const std::vector<GCDTimer> &watchdogs, id object, OperationTimeout type)
+{
+    return std::find_if(watchdogs.begin(), watchdogs.end(), [object, type](const GCDTimer &other){
+                        return [other objectUnderWatch] == object && [other timeoutType] == type;});
+}
+
 } // namespace OSXBluetooth
 
 QT_END_NAMESPACE
 
+QT_USE_NAMESPACE
 
 @interface QT_MANGLE_NAMESPACE(OSXBTCentralManager) (PrivateAPI)
 
+- (void)watchAfter:(id)object timeout:(OSXBluetooth::OperationTimeout)type;
+- (bool)objectIsUnderWatch:(id)object operation:(OSXBluetooth::OperationTimeout)type;
+- (void)stopWatchingAfter:(id)object operation:(OSXBluetooth::OperationTimeout)type;
+- (void)stopWatchers;
 - (void)retrievePeripheralAndConnect;
 - (void)connectToPeripheral;
 - (void)discoverIncludedServices;
@@ -125,6 +137,45 @@ QT_END_NAMESPACE
 @end
 
 @implementation QT_MANGLE_NAMESPACE(OSXBTCentralManager)
+{
+@private
+    CBCentralManager *manager;
+    OSXBluetooth::CentralManagerState managerState;
+    bool disconnectPending;
+
+    QBluetoothUuid deviceUuid;
+
+    OSXBluetooth::LECBManagerNotifier *notifier;
+
+    // Quite a verbose service discovery machinery
+    // (a "graph traversal").
+    OSXBluetooth::ObjCStrongReference<NSMutableArray> servicesToVisit;
+    // The service we're discovering now (included services discovery):
+    NSUInteger currentService;
+    // Included services, we'll iterate through at the end of 'servicesToVisit':
+    OSXBluetooth::ObjCStrongReference<NSMutableArray> servicesToVisitNext;
+    // We'd like to avoid loops in a services' topology:
+    OSXBluetooth::ObjCStrongReference<NSMutableSet> visitedServices;
+
+    QList<QBluetoothUuid> servicesToDiscoverDetails;
+
+    OSXBluetooth::ServiceHash serviceMap;
+    OSXBluetooth::CharHash charMap;
+    OSXBluetooth::DescHash descMap;
+
+    QLowEnergyHandle lastValidHandle;
+
+    bool requestPending;
+    OSXBluetooth::RequestQueue requests;
+    QLowEnergyHandle currentReadHandle;
+
+    OSXBluetooth::ValueHash valuesToWrite;
+
+    qint64 timeoutMS;
+    std::vector<OSXBluetooth::GCDTimer> timeoutWatchdogs;
+
+    CBPeripheral *peripheral;
+}
 
 - (id)initWith:(OSXBluetooth::LECBManagerNotifier *)aNotifier
 {
@@ -140,7 +191,6 @@ QT_END_NAMESPACE
         lastValidHandle = 0;
         requestPending = false;
         currentReadHandle = 0;
-        timeoutType = OperationTimeout::none;
 
         if (Q_UNLIKELY(!qEnvironmentVariableIsEmpty("BLUETOOTH_GATT_TIMEOUT"))) {
             bool ok = false;
@@ -176,25 +226,44 @@ QT_END_NAMESPACE
     if (notifier)
         notifier->deleteLater();
 
+    [self stopWatchers];
     [super dealloc];
 }
 
+- (CBPeripheral *)peripheral
+{
+    return peripheral;
+}
+
 - (void)watchAfter:(id)object timeout:(OSXBluetooth::OperationTimeout)type
 {
     using namespace OSXBluetooth;
 
-    objectUnderWatch = object;
-    timeoutType = type;
-    [timeoutWatchdog cancelTimer];
-    timeoutWatchdog.reset([[GCDTimerObjC alloc] initWithDelegate:self]);
-    [timeoutWatchdog startWithTimeout:timeoutMS step:200];
+    GCDTimer newWatcher([[GCDTimerObjC alloc] initWithDelegate:self], false /*do not retain*/);
+    [newWatcher watchAfter:object withTimeoutType:type];
+    timeoutWatchdogs.push_back(newWatcher);
+    [newWatcher startWithTimeout:timeoutMS step:200];
+}
+
+- (bool)objectIsUnderWatch:(id)object operation:(OSXBluetooth::OperationTimeout)type
+{
+    return OSXBluetooth::qt_find_watchdog(timeoutWatchdogs, object, type) != timeoutWatchdogs.end();
 }
 
-- (void)stopWatchdog
+- (void)stopWatchingAfter:(id)object operation:(OSXBluetooth::OperationTimeout)type
 {
-    [timeoutWatchdog cancelTimer];
-    objectUnderWatch = nil;
-    timeoutType = OSXBluetooth::OperationTimeout::none;
+    auto pos = OSXBluetooth::qt_find_watchdog(timeoutWatchdogs, object, type);
+    if (pos != timeoutWatchdogs.end()) {
+        [*pos cancelTimer];
+        timeoutWatchdogs.erase(pos);
+    }
+}
+
+- (void)stopWatchers
+{
+    for (auto &watchdog : timeoutWatchdogs)
+        [watchdog cancelTimer];
+    timeoutWatchdogs.clear();
 }
 
 - (void)timeout:(id)sender
@@ -203,37 +272,45 @@ QT_END_NAMESPACE
 
     using namespace OSXBluetooth;
 
-    Q_ASSERT(objectUnderWatch);
-    NSLog(@"Timeout caused by: %@", objectUnderWatch);
-    const ObjCStrongReference<NSError> nsError(qt_timeoutNSError(timeoutType));
-    switch (timeoutType) {
+    GCDTimerObjC *watcher = static_cast<GCDTimerObjC *>(sender);
+    id cbObject = [watcher objectUnderWatch];
+    const OperationTimeout type = [watcher timeoutType];
+
+    Q_ASSERT([self objectIsUnderWatch:cbObject operation:type]);
+
+    NSLog(@"Timeout caused by: %@", cbObject);
+
+    // Note that after this switch the 'watcher' is released (we don't
+    // own it anymore), though GCD is probably still holding a reference.
+    const ObjCStrongReference<NSError> nsError(qt_timeoutNSError(type));
+    switch (type) {
     case OperationTimeout::serviceDiscovery:
         qCWarning(QT_BT_OSX, "Timeout in services discovery");
         [self peripheral:peripheral didDiscoverServices:nsError];
         break;
     case OperationTimeout::includedServicesDiscovery:
         qCWarning(QT_BT_OSX, "Timeout in included services discovery");
-        [self peripheral:peripheral didDiscoverIncludedServicesForService:objectUnderWatch error:nsError];
+        [self peripheral:peripheral didDiscoverIncludedServicesForService:cbObject error:nsError];
         break;
     case OperationTimeout::characteristicsDiscovery:
         qCWarning(QT_BT_OSX, "Timeout in characteristics discovery");
-        [self peripheral:peripheral didDiscoverCharacteristicsForService:objectUnderWatch error:nsError];
+        [self peripheral:peripheral didDiscoverCharacteristicsForService:cbObject error:nsError];
         break;
     case OperationTimeout::characteristicRead:
         qCWarning(QT_BT_OSX, "Timeout while reading a characteristic");
-        [self peripheral:peripheral didUpdateValueForCharacteristic:objectUnderWatch error:nsError];
+        [self peripheral:peripheral didUpdateValueForCharacteristic:cbObject error:nsError];
         break;
     case OperationTimeout::descriptorsDiscovery:
         qCWarning(QT_BT_OSX, "Timeout in descriptors discovery");
-        [self peripheral:peripheral didDiscoverDescriptorsForCharacteristic:objectUnderWatch error:nsError];
+        [self peripheral:peripheral didDiscoverDescriptorsForCharacteristic:cbObject error:nsError];
         break;
     case OperationTimeout::descriptorRead:
         qCWarning(QT_BT_OSX, "Timeout while reading a descriptor");
-        [self peripheral:peripheral didUpdateValueForDescriptor:objectUnderWatch error:nsError];
+        [self peripheral:peripheral didUpdateValueForDescriptor:cbObject error:nsError];
         break;
     case OperationTimeout::characteristicWrite:
         qCWarning(QT_BT_OSX, "Timeout while writing a characteristic with response");
-        [self peripheral:peripheral didWriteValueForCharacteristic:objectUnderWatch error:nsError];
+        [self peripheral:peripheral didWriteValueForCharacteristic:cbObject error:nsError];
     default:;
     }
 }
@@ -1121,7 +1198,7 @@ QT_END_NAMESPACE
     charMap.clear();
     descMap.clear();
     currentReadHandle = 0;
-    [self stopWatchdog];
+    [self stopWatchers];
     // TODO: also serviceToVisit/VisitNext and visitedServices ?
 }
 
@@ -1274,12 +1351,14 @@ QT_END_NAMESPACE
         return;
     }
 
-    if (objectUnderWatch != aPeripheral) {
-        // Timed out.
+    using namespace OSXBluetooth;
+    if (![self objectIsUnderWatch:aPeripheral operation:OperationTimeout::serviceDiscovery]) // Timed out already
         return;
-    }
 
-    [self stopWatchdog];
+    QT_BT_MAC_AUTORELEASEPOOL;
+
+    [self stopWatchingAfter:aPeripheral operation:OperationTimeout::serviceDiscovery];
+
     managerState = OSXBluetooth::CentralManagerIdle;
 
     if (error) {
@@ -1305,16 +1384,14 @@ QT_END_NAMESPACE
         return;
     }
 
-    if (service != objectUnderWatch) {
-        // Timed out.
+    if (![self objectIsUnderWatch:service operation:OperationTimeout::includedServicesDiscovery])
         return;
-    }
 
     QT_BT_MAC_AUTORELEASEPOOL;
 
     Q_ASSERT_X(peripheral, Q_FUNC_INFO, "invalid peripheral (nil)");
 
-    [self stopWatchdog];
+    [self stopWatchingAfter:service operation:OperationTimeout::includedServicesDiscovery];
     managerState = CentralManagerIdle;
 
     if (error) {
@@ -1382,14 +1459,14 @@ QT_END_NAMESPACE
         return;
     }
 
-    if (service != objectUnderWatch) {
-        // Timed out already?
+    using namespace OSXBluetooth;
+
+    if (![self objectIsUnderWatch:service operation:OperationTimeout::characteristicsDiscovery])
         return;
-    }
 
-    [self stopWatchdog];
+    QT_BT_MAC_AUTORELEASEPOOL;
 
-    using namespace OSXBluetooth;
+    [self stopWatchingAfter:service operation:OperationTimeout::characteristicsDiscovery];
 
     Q_ASSERT_X(managerState != CentralManagerUpdating, Q_FUNC_INFO, "invalid state");
 
@@ -1409,21 +1486,21 @@ QT_END_NAMESPACE
 {
     Q_UNUSED(aPeripheral)
 
-    if (!notifier) {
-        // Detached.
+    if (!notifier) // Detached.
         return;
-    }
-
-    const bool readMatch = characteristic == objectUnderWatch;
-    if (readMatch)
-        [self stopWatchdog];
 
     using namespace OSXBluetooth;
 
+    QT_BT_MAC_AUTORELEASEPOOL;
+
+    const bool readMatch = [self objectIsUnderWatch:characteristic operation:OperationTimeout::characteristicRead];
+    if (readMatch)
+        [self stopWatchingAfter:characteristic operation:OperationTimeout::characteristicRead];
+
     Q_ASSERT_X(managerState != CentralManagerUpdating, Q_FUNC_INFO, "invalid state");
     Q_ASSERT_X(peripheral, Q_FUNC_INFO, "invalid peripheral (nil)");
 
-    QT_BT_MAC_AUTORELEASEPOOL;
+
     // First, let's check if we're discovering a service details now.
     CBService *const service = characteristic.service;
     const QBluetoothUuid qtUuid(qt_uuid(service.UUID));
@@ -1497,12 +1574,12 @@ QT_END_NAMESPACE
 
     QT_BT_MAC_AUTORELEASEPOOL;
 
-    if (characteristic != objectUnderWatch)
-        return;
+    using namespace OSXBluetooth;
 
-    [self stopWatchdog];
+    if (![self objectIsUnderWatch:characteristic operation:OperationTimeout::descriptorsDiscovery])
+        return;
 
-    using namespace OSXBluetooth;
+    [self stopWatchingAfter:characteristic operation:OperationTimeout::descriptorsDiscovery];
 
     if (error) {
         NSLog(@"%s failed with error %@", Q_FUNC_INFO, error);
@@ -1535,12 +1612,12 @@ QT_END_NAMESPACE
 
     QT_BT_MAC_AUTORELEASEPOOL;
 
-    if (descriptor != objectUnderWatch)
-        return;
+    using namespace OSXBluetooth;
 
-    [self stopWatchdog];
+    if (![self objectIsUnderWatch:descriptor operation:OperationTimeout::descriptorRead])
+        return;
 
-    using namespace OSXBluetooth;
+    [self stopWatchingAfter:descriptor operation:OperationTimeout::descriptorRead];
 
     CBService *const service = descriptor.characteristic.service;
     const QBluetoothUuid qtUuid(qt_uuid(service.UUID));
@@ -1627,13 +1704,12 @@ QT_END_NAMESPACE
 
     QT_BT_MAC_AUTORELEASEPOOL;
 
-    if (characteristic != objectUnderWatch)
+    if (![self objectIsUnderWatch:characteristic operation:OperationTimeout::characteristicWrite])
         return;
 
-    [self stopWatchdog];
+    [self stopWatchingAfter:characteristic operation:OperationTimeout::characteristicWrite];
     requestPending = false;
 
-
     // Error or not, but the cached value has to be deleted ...
     const QByteArray valueToReport(valuesToWrite.value(characteristic, QByteArray()));
     if (!valuesToWrite.remove(characteristic)) {
@@ -1729,7 +1805,7 @@ QT_END_NAMESPACE
         notifier = nullptr;
     }
 
-    [self stopWatchdog];
+    [self stopWatchers];
     [self disconnectFromDevice];
 }
 
diff --git a/src/bluetooth/osx/osxbtcentralmanager_p.h b/src/bluetooth/osx/osxbtcentralmanager_p.h
index a1d2db66..ce348dc6 100644
--- a/src/bluetooth/osx/osxbtcentralmanager_p.h
+++ b/src/bluetooth/osx/osxbtcentralmanager_p.h
@@ -136,52 +136,11 @@ QT_END_NAMESPACE
 @interface QT_MANGLE_NAMESPACE(OSXBTCentralManager) : NSObject<CBCentralManagerDelegate,
                                                                CBPeripheralDelegate,
                                                                QT_MANGLE_NAMESPACE(GCDTimerDelegate)>
-{
-@private
-    CBCentralManager *manager;
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::CentralManagerState managerState;
-    bool disconnectPending;
-
-    QT_PREPEND_NAMESPACE(QBluetoothUuid) deviceUuid;
-
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::LECBManagerNotifier *notifier;
-
-    // Quite a verbose service discovery machinery
-    // (a "graph traversal").
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::ObjCStrongReference<NSMutableArray> servicesToVisit;
-    // The service we're discovering now (included services discovery):
-    NSUInteger currentService;
-    // Included services, we'll iterate through at the end of 'servicesToVisit':
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::ObjCStrongReference<NSMutableArray> servicesToVisitNext;
-    // We'd like to avoid loops in a services' topology:
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::ObjCStrongReference<NSMutableSet> visitedServices;
-
-    QT_PREPEND_NAMESPACE(QList)<QT_PREPEND_NAMESPACE(QBluetoothUuid)> servicesToDiscoverDetails;
-
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::ServiceHash serviceMap;
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::CharHash charMap;
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::DescHash descMap;
-
-    QT_PREPEND_NAMESPACE(QLowEnergyHandle) lastValidHandle;
-
-    bool requestPending;
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::RequestQueue requests;
-    QT_PREPEND_NAMESPACE(QLowEnergyHandle) currentReadHandle;
-
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::ValueHash valuesToWrite;
-
-    qint64 timeoutMS;
-    id objectUnderWatch;
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::OperationTimeout timeoutType;
-    QT_PREPEND_NAMESPACE(OSXBluetooth)::GCDTimer timeoutWatchdog;
-
-@public
-    CBPeripheral *peripheral;
-}
-
 - (id)initWith:(QT_PREPEND_NAMESPACE(OSXBluetooth)::LECBManagerNotifier *)notifier;
 - (void)dealloc;
 
+- (CBPeripheral *)peripheral;
+
 // IMPORTANT: _all_ these methods are to be executed on qt_LE_queue,
 // when passing parameters - C++ objects _must_ be copied (see the controller's code).
 - (void)connectToDevice:(const QT_PREPEND_NAMESPACE(QBluetoothUuid) &)aDeviceUuid;
diff --git a/src/bluetooth/osx/osxbtgcdtimer_p.h b/src/bluetooth/osx/osxbtgcdtimer_p.h
index 153e9033..6bd82c9a 100644
--- a/src/bluetooth/osx/osxbtgcdtimer_p.h
+++ b/src/bluetooth/osx/osxbtgcdtimer_p.h
@@ -98,7 +98,7 @@ QT_BEGIN_NAMESPACE
 namespace OSXBluetooth {
 
 using GCDTimerObjC = QT_MANGLE_NAMESPACE(OSXBTGCDTimer);
-using GCDTimer = ObjCScopedPointer<GCDTimerObjC>;
+using GCDTimer = ObjCStrongReference<GCDTimerObjC>;
 
 } // namespace OSXBluetooth
 
diff --git a/src/bluetooth/osx/osxbtledeviceinquiry.mm b/src/bluetooth/osx/osxbtledeviceinquiry.mm
index c82cb31d..c616d470 100644
--- a/src/bluetooth/osx/osxbtledeviceinquiry.mm
+++ b/src/bluetooth/osx/osxbtledeviceinquiry.mm
@@ -206,7 +206,7 @@ QT_USE_NAMESPACE
 
             if (inquiryTimeoutMS > 0) {
                 [elapsedTimer cancelTimer];
-                elapsedTimer.reset([[GCDTimerObjC alloc] initWithDelegate:self]);
+                elapsedTimer.resetWithoutRetain([[GCDTimerObjC alloc] initWithDelegate:self]);
                 [elapsedTimer startWithTimeout:inquiryTimeoutMS step:timeStepMS];
             }
 
@@ -241,7 +241,7 @@ QT_USE_NAMESPACE
             // we'll receive 'PoweredOn' state update later.
             // No change in internalState. Wait for 30 seconds.
             [elapsedTimer cancelTimer];
-            elapsedTimer.reset([[GCDTimerObjC alloc] initWithDelegate:self]);
+            elapsedTimer.resetWithoutRetain([[GCDTimerObjC alloc] initWithDelegate:self]);
             [elapsedTimer startWithTimeout:powerOffTimeoutMS step:300];
             return;
 #else
diff --git a/src/bluetooth/qlowenergycontroller_osx.mm b/src/bluetooth/qlowenergycontroller_osx.mm
index d01686ae..8cef621c 100644
--- a/src/bluetooth/qlowenergycontroller_osx.mm
+++ b/src/bluetooth/qlowenergycontroller_osx.mm
@@ -234,7 +234,7 @@ void QLowEnergyControllerPrivateOSX::_q_serviceDiscoveryFinished()
 
     QT_BT_MAC_AUTORELEASEPOOL;
 
-    NSArray *const services = centralManager.data()->peripheral.services;
+    NSArray *const services = [centralManager.data() peripheral].services;
     // Now we have to traverse the discovered services tree.
     // Essentially it's an iterative version of more complicated code from the
     // OSXBTCentralManager's code.