From 68cb332228df1c8730e721061134c18bda37cf02 Mon Sep 17 00:00:00 2001 From: Oliver Wolff Date: Wed, 17 Apr 2019 12:17:21 +0200 Subject: qlowenergycontroller_winrt_new: Avoid late callbacks that lead to crashes Users may run into crashes on device disconnects in case that we run into a callback while the disconnects happen. Thus we have to avoid calling functions on deleted objects by avoiding lambdas if possible or using QPointers in lambda captures. Change-Id: Idcd3781bd396d4ef785191e4c65bae20e5149c04 Reviewed-by: Alex Blasche --- src/bluetooth/qlowenergycontroller_winrt_new.cpp | 174 ++++++++++++----------- src/bluetooth/qlowenergycontroller_winrt_new_p.h | 6 +- 2 files changed, 97 insertions(+), 83 deletions(-) diff --git a/src/bluetooth/qlowenergycontroller_winrt_new.cpp b/src/bluetooth/qlowenergycontroller_winrt_new.cpp index c3efec7f..fb83371b 100644 --- a/src/bluetooth/qlowenergycontroller_winrt_new.cpp +++ b/src/bluetooth/qlowenergycontroller_winrt_new.cpp @@ -91,15 +91,18 @@ typedef IGattReadClientCharacteristicConfigurationDescriptorResult IClientCharCo continue; \ } -#define CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, msg, ret) \ +#define CHECK_FOR_DEVICE_CONNECTION_ERROR_IMPL(this, hr, msg, ret) \ if (FAILED(hr)) { \ qCWarning(QT_BT_WINRT) << msg; \ - unregisterFromStatusChanges(); \ - setError(QLowEnergyController::ConnectionError); \ - setState(QLowEnergyController::UnconnectedState); \ + this->unregisterFromStatusChanges(); \ + this->setError(QLowEnergyController::ConnectionError); \ + this->setState(QLowEnergyController::UnconnectedState); \ ret; \ } +#define CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, msg, ret) \ + CHECK_FOR_DEVICE_CONNECTION_ERROR_IMPL(this, hr, msg, ret) + #define CHECK_HR_AND_SET_SERVICE_ERROR(hr, msg, service, error, ret) \ if (FAILED(hr)) { \ qCDebug(QT_BT_WINRT) << msg; \ @@ -668,18 +671,8 @@ void QLowEnergyControllerPrivateWinRTNew::registerForValueChanges(const QBluetoo EventRegistrationToken token; HRESULT hr; hr = characteristic->add_ValueChanged( - Callback( - [this](IGattCharacteristic *characteristic, IGattValueChangedEventArgs *args) { - HRESULT hr; - quint16 handle; - hr = characteristic->get_AttributeHandle(&handle); - RETURN_IF_FAILED("Could not obtain characteristic's handle", return S_OK) - ComPtr buffer; - hr = args->get_CharacteristicValue(&buffer); - RETURN_IF_FAILED("Could not obtain characteristic's value", return S_OK) - characteristicChanged(handle, byteArrayFromBuffer(buffer)); - return S_OK; - }).Get(), &token); + Callback(this, &QLowEnergyControllerPrivateWinRTNew::onValueChange).Get(), + &token); RETURN_IF_FAILED("Could not register characteristic for value changes", return) mValueChangedTokens.append(ValueChangedEntry(characteristic, token)); qCDebug(QT_BT_WINRT) << "Characteristic" << charUuid << "in service" @@ -703,6 +696,19 @@ void QLowEnergyControllerPrivateWinRTNew::unregisterFromValueChanges() mValueChangedTokens.clear(); } +HRESULT QLowEnergyControllerPrivateWinRTNew::onValueChange(IGattCharacteristic *characteristic, IGattValueChangedEventArgs *args) +{ + HRESULT hr; + quint16 handle; + hr = characteristic->get_AttributeHandle(&handle); + RETURN_IF_FAILED("Could not obtain characteristic's handle", return S_OK) + ComPtr buffer; + hr = args->get_CharacteristicValue(&buffer); + RETURN_IF_FAILED("Could not obtain characteristic's value", return S_OK) + characteristicChanged(handle, byteArrayFromBuffer(buffer)); + return S_OK; +} + bool QLowEnergyControllerPrivateWinRTNew::registerForStatusChanges() { if (!mDevice) @@ -812,10 +818,68 @@ void QLowEnergyControllerPrivateWinRTNew::obtainIncludedServices( } } -void QLowEnergyControllerPrivateWinRTNew::discoverServices() +HRESULT QLowEnergyControllerPrivateWinRTNew::onServiceDiscoveryFinished(ABI::Windows::Foundation::IAsyncOperation *op, AsyncStatus status) { Q_Q(QLowEnergyController); + if (status != AsyncStatus::Completed) { + qCDebug(QT_BT_WINRT) << "Could not obtain services"; + return S_OK; + } + ComPtr result; + ComPtr> deviceServices; + HRESULT hr = op->GetResults(&result); + CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not obtain service discovery result", + return S_OK); + GattCommunicationStatus commStatus; + hr = result->get_Status(&commStatus); + CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not obtain service discovery status", + return S_OK); + if (commStatus != GattCommunicationStatus_Success) + return S_OK; + + hr = result->get_Services(&deviceServices); + CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not obtain service list", + return S_OK); + + uint serviceCount; + hr = deviceServices->get_Size(&serviceCount); + CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not obtain service list size", + return S_OK); + for (uint i = 0; i < serviceCount; ++i) { + ComPtr deviceService; + hr = deviceServices->GetAt(i, &deviceService); + WARN_AND_CONTINUE_IF_FAILED(hr, "Could not obtain service"); + GUID guuid; + hr = deviceService->get_Uuid(&guuid); + WARN_AND_CONTINUE_IF_FAILED(hr, "Could not obtain service's Uuid"); + const QBluetoothUuid service(guuid); + + QSharedPointer pointer; + if (serviceList.contains(service)) { + pointer = serviceList.value(service); + } else { + QLowEnergyServicePrivate *priv = new QLowEnergyServicePrivate(); + priv->uuid = service; + priv->setController(this); + pointer = QSharedPointer(priv); + serviceList.insert(service, pointer); + } + pointer->type |= QLowEnergyService::PrimaryService; + + obtainIncludedServices(pointer, deviceService); + + emit q->serviceDiscovered(service); + } + + setState(QLowEnergyController::DiscoveredState); + emit q->discoveryFinished(); + + return S_OK; +} + +void QLowEnergyControllerPrivateWinRTNew::discoverServices() +{ qCDebug(QT_BT_WINRT) << "Service discovery initiated"; ComPtr device3; @@ -824,67 +888,10 @@ void QLowEnergyControllerPrivateWinRTNew::discoverServices() ComPtr> asyncResult; hr = device3->GetGattServicesAsync(&asyncResult); CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not obtain services", return); - hr = QEventDispatcherWinRT::runOnXamlThread( [asyncResult, q, this] () { + hr = QEventDispatcherWinRT::runOnXamlThread( [asyncResult, this] () { HRESULT hr = asyncResult->put_Completed( Callback>( - [this, q](IAsyncOperation *op, - AsyncStatus status) { - if (status != AsyncStatus::Completed) { - qCDebug(QT_BT_WINRT) << "Could not obtain services"; - return S_OK; - } - ComPtr result; - ComPtr> deviceServices; - HRESULT hr = op->GetResults(&result); - CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not obtain service discovery result", - return S_OK); - GattCommunicationStatus commStatus; - hr = result->get_Status(&commStatus); - CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not obtain service discovery status", - return S_OK); - if (commStatus != GattCommunicationStatus_Success) - return S_OK; - - hr = result->get_Services(&deviceServices); - CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not obtain service list", - return S_OK); - - uint serviceCount; - hr = deviceServices->get_Size(&serviceCount); - CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not obtain service list size", - return S_OK); - for (uint i = 0; i < serviceCount; ++i) { - ComPtr deviceService; - hr = deviceServices->GetAt(i, &deviceService); - WARN_AND_CONTINUE_IF_FAILED(hr, "Could not obtain service"); - GUID guuid; - hr = deviceService->get_Uuid(&guuid); - WARN_AND_CONTINUE_IF_FAILED(hr, "Could not obtain service's Uuid"); - const QBluetoothUuid service(guuid); - - QSharedPointer pointer; - if (serviceList.contains(service)) { - pointer = serviceList.value(service); - } else { - QLowEnergyServicePrivate *priv = new QLowEnergyServicePrivate(); - priv->uuid = service; - priv->setController(this); - - pointer = QSharedPointer(priv); - serviceList.insert(service, pointer); - } - pointer->type |= QLowEnergyService::PrimaryService; - - obtainIncludedServices(pointer, deviceService); - - emit q->serviceDiscovered(service); - } - - setState(QLowEnergyController::DiscoveredState); - emit q->discoveryFinished(); - - return S_OK; - }).Get()); + this, &QLowEnergyControllerPrivateWinRTNew::onServiceDiscoveryFinished).Get()); CHECK_FOR_DEVICE_CONNECTION_ERROR(hr, "Could not register service discovery callback", return S_OK) return hr; @@ -1326,7 +1333,8 @@ void QLowEnergyControllerPrivateWinRTNew::writeCharacteristic( hr = characteristic->WriteValueWithOptionAsync(buffer.Get(), option, &writeOp); CHECK_HR_AND_SET_SERVICE_ERROR(hr, "Could write characteristic", service, QLowEnergyService::CharacteristicWriteError, return S_OK) - auto writeCompletedLambda =[charData, charHandle, newValue, service, writeWithResponse, this] + QPointer thisPtr(this); + auto writeCompletedLambda = [charData, charHandle, newValue, service, writeWithResponse, thisPtr] (IAsyncOperation *op, AsyncStatus status) { if (status == AsyncStatus::Canceled || status == AsyncStatus::Error) { @@ -1353,7 +1361,7 @@ void QLowEnergyControllerPrivateWinRTNew::writeCharacteristic( // only update cache when property is readable. Otherwise it remains // empty. if (charData.properties & QLowEnergyCharacteristic::Read) - updateValueOfCharacteristic(charHandle, newValue, false); + thisPtr->updateValueOfCharacteristic(charHandle, newValue, false); if (writeWithResponse) emit service->characteristicWritten(QLowEnergyCharacteristic(service, charHandle), newValue); @@ -1433,7 +1441,8 @@ void QLowEnergyControllerPrivateWinRTNew::writeDescriptor( HRESULT hr = characteristic->WriteClientCharacteristicConfigurationDescriptorAsync(value, &writeOp); CHECK_HR_AND_SET_SERVICE_ERROR(hr, "Could not write client characteristic configuration", service, QLowEnergyService::DescriptorWriteError, return S_OK) - auto writeCompletedLambda = [charHandle, descHandle, newValue, service, this] + QPointer thisPtr(this); + auto writeCompletedLambda = [charHandle, descHandle, newValue, service, thisPtr] (IAsyncOperation *op, AsyncStatus status) { if (status == AsyncStatus::Canceled || status == AsyncStatus::Error) { @@ -1451,7 +1460,7 @@ void QLowEnergyControllerPrivateWinRTNew::writeDescriptor( service->setError(QLowEnergyService::DescriptorWriteError); return S_OK; } - updateValueOfDescriptor(charHandle, descHandle, newValue, false); + thisPtr->updateValueOfDescriptor(charHandle, descHandle, newValue, false); emit service->descriptorWritten(QLowEnergyDescriptor(service, charHandle, descHandle), newValue); return S_OK; @@ -1526,7 +1535,8 @@ void QLowEnergyControllerPrivateWinRTNew::writeDescriptor( hr = descriptor->WriteValueAsync(buffer.Get(), &writeOp); CHECK_HR_AND_SET_SERVICE_ERROR(hr, "Could not write descriptor value", service, QLowEnergyService::DescriptorWriteError, return S_OK) - auto writeCompletedLambda = [charHandle, descHandle, newValue, service, this] + QPointer thisPtr(this); + auto writeCompletedLambda = [charHandle, descHandle, newValue, service, thisPtr] (IAsyncOperation *op, AsyncStatus status) { if (status == AsyncStatus::Canceled || status == AsyncStatus::Error) { @@ -1544,7 +1554,7 @@ void QLowEnergyControllerPrivateWinRTNew::writeDescriptor( service->setError(QLowEnergyService::DescriptorWriteError); return S_OK; } - updateValueOfDescriptor(charHandle, descHandle, newValue, false); + thisPtr->updateValueOfDescriptor(charHandle, descHandle, newValue, false); emit service->descriptorWritten(QLowEnergyDescriptor(service, charHandle, descHandle), newValue); return S_OK; diff --git a/src/bluetooth/qlowenergycontroller_winrt_new_p.h b/src/bluetooth/qlowenergycontroller_winrt_new_p.h index 69ee3fca..fa8e516f 100644 --- a/src/bluetooth/qlowenergycontroller_winrt_new_p.h +++ b/src/bluetooth/qlowenergycontroller_winrt_new_p.h @@ -62,6 +62,7 @@ #include #include +#include #include @@ -142,6 +143,8 @@ private: void registerForValueChanges(const QBluetoothUuid &serviceUuid, const QBluetoothUuid &charUuid); void unregisterFromValueChanges(); + HRESULT onValueChange(ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::IGattCharacteristic *characteristic, + ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::IGattValueChangedEventArgs *args); bool registerForStatusChanges(); void unregisterFromStatusChanges(); @@ -149,7 +152,8 @@ private: void obtainIncludedServices(QSharedPointer servicePointer, Microsoft::WRL::ComPtr nativeService); - + HRESULT onServiceDiscoveryFinished(ABI::Windows::Foundation::IAsyncOperation *op, + AsyncStatus status); }; QT_END_NAMESPACE -- cgit v1.2.3