From 06b3f5a1332a6a840cabfb61718ce5141d39fb76 Mon Sep 17 00:00:00 2001 From: Ivan Solovev Date: Mon, 29 Aug 2022 17:17:46 +0200 Subject: Windows device discovery: fix crash at discovery stop The crash is cannot be reproduced reliably. One of the possible scenarios is: * After finishing the scanning a last QWinRTBluetoothDeviceDiscoveryWorker::onAdvertisementDataReceived() slot is executed on worker's thread (thread A). * This slot starts an async BluetoothLEDevice::FromBluetoothAddressAsync() call, capturing shared_from_this() to make sure that the worker object is alive when the async call completes. The async call executes on some other thread B. * The async call completes on thread B with a failure (that happens with some LE devices), so we call invokeDecrementPendingDevicesCountAndCheckFinished(). This call schedules a decrementPendingDevicesCountAndCheckFinished() function call on thread A. * The lambda in thread B is completed, so shared pointer's counter is decremented. It is the last instance of the worker, so its destructor is called. * At the same time the decrementPendingDevicesCountAndCheckFinished() function is called on thread A. * It refers to an already deleted worker -> crash. This patch fixes it by passing the shared worker instance to the decrementPendingDevicesCountAndCheckFinished() function, making sure that the worker object is alive when this function is invoked. As a drive-by: get rid of the {invoke}IncrementPendingDevicesCount() functions, because we always increment the device count from the worker thread, so we can update the variable directly. This commit amends 2f560d044fec92e94e8438791aa5e4d9daced197 and 36dd802c964f97522d1f5a75c8fb7a67f3061a3d. Fixes: QTBUG-106029 Change-Id: I2d82c34b17c8cef873c9c61a92d874c377501edb Reviewed-by: Alex Blasche Reviewed-by: Qt CI Bot Reviewed-by: Oliver Wolff Reviewed-by: Juha Vuolle (cherry picked from commit e284887e0f6093767a5af16d497549860ca1770d) Reviewed-by: Qt Cherry-pick Bot --- .../qbluetoothdevicediscoveryagent_winrt.cpp | 53 +++++++++++----------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/src/bluetooth/qbluetoothdevicediscoveryagent_winrt.cpp b/src/bluetooth/qbluetoothdevicediscoveryagent_winrt.cpp index 637a9fa3..713ec12b 100644 --- a/src/bluetooth/qbluetoothdevicediscoveryagent_winrt.cpp +++ b/src/bluetooth/qbluetoothdevicediscoveryagent_winrt.cpp @@ -263,8 +263,8 @@ private: std::shared_ptr createAdvertisementWatcher(); // invokable methods for handling finish conditions - Q_INVOKABLE void incrementPendingDevicesCount(); - Q_INVOKABLE void decrementPendingDevicesCountAndCheckFinished(); + Q_INVOKABLE void decrementPendingDevicesCountAndCheckFinished( + std::shared_ptr worker); Q_SIGNALS: void deviceFound(const QBluetoothDeviceInfo &info); @@ -308,15 +308,13 @@ private: QTimer *m_leScanTimer = nullptr; }; -static void invokeIncrementPendingDevicesCount(QObject *context) +static void invokeDecrementPendingDevicesCountAndCheckFinished( + std::shared_ptr worker) { - QMetaObject::invokeMethod(context, "incrementPendingDevicesCount", Qt::QueuedConnection); -} - -static void invokeDecrementPendingDevicesCountAndCheckFinished(QObject *context) -{ - QMetaObject::invokeMethod(context, "decrementPendingDevicesCountAndCheckFinished", - Qt::QueuedConnection); + QMetaObject::invokeMethod(worker.get(), "decrementPendingDevicesCountAndCheckFinished", + Qt::QueuedConnection, + Q_ARG(std::shared_ptr, + worker)); } QWinRTBluetoothDeviceDiscoveryWorker::QWinRTBluetoothDeviceDiscoveryWorker( @@ -484,7 +482,7 @@ void QWinRTBluetoothDeviceDiscoveryWorker::onAdvertisementDataReceived( m_foundLEDevicesMap.insert(address, info); } } - invokeIncrementPendingDevicesCount(this); // as if we discovered a new LE device + ++m_pendingDevices; // as if we discovered a new LE device auto thisPtr = shared_from_this(); auto asyncOp = BluetoothLEDevice::FromBluetoothAddressAsync(address); asyncOp.Completed([thisPtr, address](auto &&op, AsyncStatus status) { @@ -499,7 +497,7 @@ void QWinRTBluetoothDeviceDiscoveryWorker::onAdvertisementDataReceived( // status != Completed or failed to extract result qCDebug(QT_BT_WINDOWS) << "Failed to get LE device from address" << QBluetoothAddress(address); - invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr.get()); + invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr); } }); } @@ -562,7 +560,7 @@ void QWinRTBluetoothDeviceDiscoveryWorker::getClassicDeviceFromId(const winrt::h } // status != Completed or failed to extract result qCDebug(QT_BT_WINDOWS) << "Failed to get Classic device from id"; - invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr.get()); + invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr); } }); } @@ -587,7 +585,7 @@ void QWinRTBluetoothDeviceDiscoveryWorker::handleClassicDevice(const BluetoothDe } // Failed to get services qCDebug(QT_BT_WINDOWS) << "Failed to get RFCOMM services for device" << btName; - invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr.get()); + invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr); } }); } @@ -598,8 +596,9 @@ void QWinRTBluetoothDeviceDiscoveryWorker::handleRfcommServices( const QString &name, uint32_t classOfDeviceInt) { // need to perform the check even if some of the operations fails - auto guard = qScopeGuard([this]() { - invokeDecrementPendingDevicesCountAndCheckFinished(this); + auto shared = shared_from_this(); + auto guard = qScopeGuard([shared]() { + invokeDecrementPendingDevicesCountAndCheckFinished(shared); }); Q_UNUSED(guard); // to suppress warning @@ -632,16 +631,15 @@ void QWinRTBluetoothDeviceDiscoveryWorker::handleRfcommServices( Q_ARG(QBluetoothDeviceInfo, info)); } -void QWinRTBluetoothDeviceDiscoveryWorker::incrementPendingDevicesCount() -{ - ++m_pendingDevices; -} - -void QWinRTBluetoothDeviceDiscoveryWorker::decrementPendingDevicesCountAndCheckFinished() +void QWinRTBluetoothDeviceDiscoveryWorker::decrementPendingDevicesCountAndCheckFinished( + std::shared_ptr worker) { --m_pendingDevices; if (isFinished()) finishDiscovery(); + // Worker is passed here simply to make sure that the object is still alive + // when we call this method via QObject::invoke(). + Q_UNUSED(worker) } // this function executes in main worker thread @@ -661,7 +659,7 @@ void QWinRTBluetoothDeviceDiscoveryWorker::getLowEnergyDeviceFromId(const winrt: } // status != Completed or failed to extract result qCDebug(QT_BT_WINDOWS) << "Failed to get LE device from id"; - invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr.get()); + invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr); } }); } @@ -693,7 +691,7 @@ void QWinRTBluetoothDeviceDiscoveryWorker::handleLowEnergyDevice(const Bluetooth // Use the services obtained from the advertisement data if the device is not paired if (!isPaired) { info.setServiceUuids(adInfo.services); - invokeDecrementPendingDevicesCountAndCheckFinished(this); + invokeDecrementPendingDevicesCountAndCheckFinished(shared_from_this()); invokeDeviceFoundWithDebug(info); } else { auto asyncOp = device.GetGattServicesAsync(); @@ -708,7 +706,7 @@ void QWinRTBluetoothDeviceDiscoveryWorker::handleLowEnergyDevice(const Bluetooth } // Failed to get services qCDebug(QT_BT_WINDOWS) << "Failed to get GATT services for device" << info.name(); - invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr.get()); + invokeDecrementPendingDevicesCountAndCheckFinished(thisPtr); }); } } @@ -718,8 +716,9 @@ void QWinRTBluetoothDeviceDiscoveryWorker::handleGattServices( const GattDeviceServicesResult &servicesResult, QBluetoothDeviceInfo &info) { // need to perform the check even if some of the operations fails - auto guard = qScopeGuard([this]() { - invokeDecrementPendingDevicesCountAndCheckFinished(this); + auto shared = shared_from_this(); + auto guard = qScopeGuard([shared]() { + invokeDecrementPendingDevicesCountAndCheckFinished(shared); }); Q_UNUSED(guard); // to suppress warning -- cgit v1.2.1