From 336bd4ff9edfdb3e5607a6e74af663b409e0c852 Mon Sep 17 00:00:00 2001 From: Juha Vuolle Date: Fri, 13 May 2022 07:48:40 +0300 Subject: Close socket descriptor when QBluetoothSocketBluez is destroyed There are two private QBluetoothSocket backends on Linux: - QBluetoothSocketBluez is native linux socket implementation It is always used by the linux QBluetoothServer, and by QBluetoothSocket if Bluez version is < 5.46 - QBluetoothSocketBluezDbus used by QBluetoothSocket when Bluez >= 5.46 Leaving the native socket unclosed leaks the resource and eventually we may run out of descriptors. This is reproducible by creating and destroying QBluetoothServer instances in a loop. As a related drive-by: - Fix bluetooth socket autotest version check. DBus socket is used with bluez 5.46+ (for clarity: DBus lowenergycontroller is used with bluez 5.42+). This is needed for the test to pass with Bluez < 5.46 - Add a clarifying comment on socket close() Fixes: QTBUG-103067 Change-Id: Idc38c743be09e559ea82bf09c2f9e44e4b80d666 Reviewed-by: Ivan Solovev Reviewed-by: Alex Blasche (cherry picked from commit 3aafe9d5ce117858143dbb527f742cf875aa83e8) --- src/bluetooth/qbluetoothsocket.cpp | 3 +++ src/bluetooth/qbluetoothsocket_bluez.cpp | 6 ++++++ 2 files changed, 9 insertions(+) (limited to 'src/bluetooth') diff --git a/src/bluetooth/qbluetoothsocket.cpp b/src/bluetooth/qbluetoothsocket.cpp index 864b9cab..5499c7d7 100644 --- a/src/bluetooth/qbluetoothsocket.cpp +++ b/src/bluetooth/qbluetoothsocket.cpp @@ -791,6 +791,9 @@ void QBluetoothSocket::close() Set the socket to use \a socketDescriptor with a type of \a socketType, which is in state, \a socketState, and mode, \a openMode. + The set socket descriptor is considered owned by the QBluetoothSocket + and may be e.g. closed once finished. + Returns true on success */ diff --git a/src/bluetooth/qbluetoothsocket_bluez.cpp b/src/bluetooth/qbluetoothsocket_bluez.cpp index f516629e..0240edbf 100644 --- a/src/bluetooth/qbluetoothsocket_bluez.cpp +++ b/src/bluetooth/qbluetoothsocket_bluez.cpp @@ -75,6 +75,10 @@ QBluetoothSocketPrivateBluez::~QBluetoothSocketPrivateBluez() readNotifier = nullptr; delete connectWriteNotifier; connectWriteNotifier = nullptr; + + // If the socket wasn't closed/aborted make sure we free the socket file descriptor + if (socket != -1) + QT_CLOSE(socket); } bool QBluetoothSocketPrivateBluez::ensureNativeSocket(QBluetoothServiceInfo::Protocol type) @@ -658,6 +662,8 @@ qint64 QBluetoothSocketPrivateBluez::readData(char *data, qint64 maxSize) void QBluetoothSocketPrivateBluez::close() { + // If we have pending data on the write buffer, wait until it has been written, + // after which this close() will be called again if (txBuffer.size() > 0) connectWriteNotifier->setEnabled(true); else -- cgit v1.2.3 From 095717ec21c0afa91f9472803ad971cde43f5f44 Mon Sep 17 00:00:00 2001 From: Andreas Buhr Date: Mon, 28 Feb 2022 18:27:47 +0100 Subject: Repair tst_QBluetoothDeviceDiscoveryAgent unit test on Android tst_QBluetoothDeviceDiscoveryAgent::tst_startStopDeviceDiscoveries was broken on Android. The sequence QBluetoothDeviceDiscoveryAgent::start() QBluetoothDeviceDiscoveryAgent::stop() QBluetoothDeviceDiscoveryAgent::start() QBluetoothDeviceDiscoveryAgent::stop() is called rather quickly. The first stop() results in the state pendingCancel=true, pendingStart=false. The second start() results in the state pendingCancel=true, pendingStart=true. The second stop() then did nothing because pendingCancel=true. Then, after the whole sequence, discovery started because pendingStart=true. This patch repairs it by setting pendingStart=false in the stop() method. Change-Id: I55486b5b494265c90149e72461a1d0529adaa2f0 Reviewed-by: Alex Blasche (cherry picked from commit 32b859dc9c83c7fb440e53335917021ef7eab15d) Reviewed-by: Qt Cherry-pick Bot --- src/bluetooth/qbluetoothdevicediscoveryagent_android.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'src/bluetooth') diff --git a/src/bluetooth/qbluetoothdevicediscoveryagent_android.cpp b/src/bluetooth/qbluetoothdevicediscoveryagent_android.cpp index 21187e5e..03ef0d21 100644 --- a/src/bluetooth/qbluetoothdevicediscoveryagent_android.cpp +++ b/src/bluetooth/qbluetoothdevicediscoveryagent_android.cpp @@ -285,15 +285,20 @@ void QBluetoothDeviceDiscoveryAgentPrivate::stop() { Q_Q(QBluetoothDeviceDiscoveryAgent); + pendingStart = false; + if (m_active == NoScanActive) return; if (m_active == SDPScanActive) { - if (pendingCancel) + if (pendingCancel) { + // If we had both a pending cancel and a pending start, + // we now have only a pending cancel. + // The pending start was canceled above. return; + } pendingCancel = true; - pendingStart = false; bool success = adapter.callMethod("cancelDiscovery"); if (!success) { lastError = QBluetoothDeviceDiscoveryAgent::InputOutputError; -- cgit v1.2.3 From 34b2849bbf0a8cf8dd712ba235e0ff1c6ac133ec Mon Sep 17 00:00:00 2001 From: Juha Vuolle Date: Mon, 13 Jun 2022 21:17:15 +0300 Subject: Add a timeout guard for Android BT device discovery not starting In some bluetooth environments, after a full SDP service discovery, the device discovery can get stuck indefinitely. Everything seems to be starting fine but we do not get the DISCOVERY_STARTED action. In the normal case this action is received in < 1 second. This commit adds a timeout guard for this. If we don't get the DISCOVERY_STARTED action in time, we silently restart the query. Typically the discovery starts working after 10..20 seconds. Task-number: QTBUG-101066 Change-Id: Id6032ebeec97d1ad9eec07a946bc623c92500fd5 Reviewed-by: Ivan Solovev (cherry picked from commit eedaaca9634d56dce27601749049c81b201ab625) --- .../android/devicediscoverybroadcastreceiver.cpp | 2 +- .../android/devicediscoverybroadcastreceiver_p.h | 1 + .../qbluetoothdevicediscoveryagent_android.cpp | 115 ++++++++++++++++----- src/bluetooth/qbluetoothdevicediscoveryagent_p.h | 4 + .../qbluetoothservicediscoveryagent_android.cpp | 2 +- 5 files changed, 96 insertions(+), 28 deletions(-) (limited to 'src/bluetooth') diff --git a/src/bluetooth/android/devicediscoverybroadcastreceiver.cpp b/src/bluetooth/android/devicediscoverybroadcastreceiver.cpp index 1af131f5..9663bc2b 100644 --- a/src/bluetooth/android/devicediscoverybroadcastreceiver.cpp +++ b/src/bluetooth/android/devicediscoverybroadcastreceiver.cpp @@ -421,7 +421,7 @@ void DeviceDiscoveryBroadcastReceiver::onReceive(JNIEnv *env, jobject context, j emit finished(); } else if (action == valueForStaticField(JavaNames::BluetoothAdapter, JavaNames::ActionDiscoveryStarted).toString()) { - + emit discoveryStarted(); } else if (action == valueForStaticField(JavaNames::BluetoothDevice, JavaNames::ActionFound).toString()) { //get BluetoothDevice diff --git a/src/bluetooth/android/devicediscoverybroadcastreceiver_p.h b/src/bluetooth/android/devicediscoverybroadcastreceiver_p.h index 75019ab1..150e01cb 100644 --- a/src/bluetooth/android/devicediscoverybroadcastreceiver_p.h +++ b/src/bluetooth/android/devicediscoverybroadcastreceiver_p.h @@ -70,6 +70,7 @@ public: signals: void deviceDiscovered(const QBluetoothDeviceInfo &info, bool isLeScanResult); + void discoveryStarted(); void finished(); private: diff --git a/src/bluetooth/qbluetoothdevicediscoveryagent_android.cpp b/src/bluetooth/qbluetoothdevicediscoveryagent_android.cpp index 03ef0d21..333cc4d5 100644 --- a/src/bluetooth/qbluetoothdevicediscoveryagent_android.cpp +++ b/src/bluetooth/qbluetoothdevicediscoveryagent_android.cpp @@ -58,6 +58,9 @@ enum { BtleScanActive = 2 }; +static constexpr auto deviceDiscoveryStartTimeLimit = std::chrono::seconds{5}; +static constexpr short deviceDiscoveryStartMaxAttempts = 6; + QBluetoothDeviceDiscoveryAgentPrivate::QBluetoothDeviceDiscoveryAgentPrivate( const QBluetoothAddress &deviceAdapter, QBluetoothDeviceDiscoveryAgent *parent) : inquiryType(QBluetoothDeviceDiscoveryAgent::GeneralUnlimitedInquiry), @@ -66,6 +69,7 @@ QBluetoothDeviceDiscoveryAgentPrivate::QBluetoothDeviceDiscoveryAgentPrivate( m_adapterAddress(deviceAdapter), m_active(NoScanActive), leScanTimeout(0), + deviceDiscoveryStartAttemptsLeft(deviceDiscoveryStartMaxAttempts), pendingCancel(false), pendingStart(false), lowEnergySearchTimeout(25000), @@ -112,9 +116,42 @@ QBluetoothDeviceDiscoveryAgent::DiscoveryMethods QBluetoothDeviceDiscoveryAgent: return (LowEnergyMethod | ClassicMethod); } +void QBluetoothDeviceDiscoveryAgentPrivate::classicDiscoveryStartFail() +{ + Q_Q(QBluetoothDeviceDiscoveryAgent); + lastError = QBluetoothDeviceDiscoveryAgent::InputOutputError; + errorString = QBluetoothDeviceDiscoveryAgent::tr("Classic Discovery cannot be started"); + emit q->error(lastError); +} + +// Sets & emits an error and returns true if bluetooth is off +bool QBluetoothDeviceDiscoveryAgentPrivate::setErrorIfPowerOff() +{ + Q_Q(QBluetoothDeviceDiscoveryAgent); + + const int state = adapter.callMethod("getState"); + if (state != 12) { // BluetoothAdapter.STATE_ON + lastError = QBluetoothDeviceDiscoveryAgent::PoweredOffError; + m_active = NoScanActive; + errorString = QBluetoothDeviceDiscoveryAgent::tr("Device is powered off"); + emit q->error(lastError); + return true; + } + return false; +} + +/* +The Classic/LE discovery method precedence is handled as follows: + +If only classic method is set => only classic method is used +If only LE method is set => only LE method is used +If both classic and LE methods are set, start classic scan first + If classic scan fails to start, start LE scan immediately in the start function + Otherwise start LE scan when classic scan completes +*/ + void QBluetoothDeviceDiscoveryAgentPrivate::start(QBluetoothDeviceDiscoveryAgent::DiscoveryMethods methods) { - //TODO Implement discovery method handling (see input parameter) requestedMethods = methods; if (pendingCancel) { @@ -142,13 +179,8 @@ void QBluetoothDeviceDiscoveryAgentPrivate::start(QBluetoothDeviceDiscoveryAgent return; } - const int state = adapter.callMethod("getState"); - if (state != 12) { // BluetoothAdapter.STATE_ON - lastError = QBluetoothDeviceDiscoveryAgent::PoweredOffError; - errorString = QBluetoothDeviceDiscoveryAgent::tr("Device is powered off"); - emit q->error(lastError); + if (setErrorIfPowerOff()) return; - } // check Android v23+ permissions // -> any device search requires android.permission.ACCESS_COARSE_LOCATION or android.permission.ACCESS_FINE_LOCATION @@ -247,16 +279,54 @@ void QBluetoothDeviceDiscoveryAgentPrivate::start(QBluetoothDeviceDiscoveryAgent const bool success = adapter.callMethod("startDiscovery"); if (!success) { qCDebug(QT_BT_ANDROID) << "Classic Discovery cannot be started"; + // Check if only classic discovery requested -> error out and return. + // Otherwise since LE was also requested => don't return but allow the + // function to continue to LE scanning if (requestedMethods == QBluetoothDeviceDiscoveryAgent::ClassicMethod) { - //only classic discovery requested -> error out - lastError = QBluetoothDeviceDiscoveryAgent::InputOutputError; - errorString = QBluetoothDeviceDiscoveryAgent::tr("Classic Discovery cannot be started"); - - emit q->error(lastError); + classicDiscoveryStartFail(); return; - } // else fall through to LE discovery + } } else { m_active = SDPScanActive; + if (!deviceDiscoveryStartTimeout) { + // In some bluetooth environments device discovery does not start properly + // if it is done shortly after (up to 20 seconds) a full service discovery. + // In that case we never get DISOVERY_STARTED action and device discovery never + // finishes. Here we use a small timeout to guard it; if we don't get the + // 'started' action in time, we restart the query. In the normal case the action + // is received in < 1 second. See QTBUG-101066 + deviceDiscoveryStartTimeout = new QTimer(this); + deviceDiscoveryStartTimeout->setInterval(deviceDiscoveryStartTimeLimit); + deviceDiscoveryStartTimeout->setSingleShot(true); + QObject::connect(receiver, &DeviceDiscoveryBroadcastReceiver::discoveryStarted, + deviceDiscoveryStartTimeout, &QTimer::stop); + QObject::connect(deviceDiscoveryStartTimeout, &QTimer::timeout, [this]() { + deviceDiscoveryStartAttemptsLeft -= 1; + qCWarning(QT_BT_ANDROID) << "Discovery start not received, attempts left:" + << deviceDiscoveryStartAttemptsLeft; + // Check that bluetooth is not switched off + if (setErrorIfPowerOff()) + return; + // If this was the last retry attempt, cancel the discovery just in case + // as a good cleanup practice + if (deviceDiscoveryStartAttemptsLeft <= 0) { + qCWarning(QT_BT_ANDROID) << "Classic device discovery failed to start"; + (void)adapter.callMethod("cancelDiscovery"); + } + // Restart the discovery and retry timer. + // The logic below is similar as in the start() + if (deviceDiscoveryStartAttemptsLeft > 0 && + adapter.callMethod("startDiscovery")) + deviceDiscoveryStartTimeout->start(); + else if (requestedMethods == QBluetoothDeviceDiscoveryAgent::ClassicMethod) + classicDiscoveryStartFail(); // No LE scan requested, scan is done + else + startLowEnergyScan(); // Continue to LE scan + }); + } + deviceDiscoveryStartAttemptsLeft = deviceDiscoveryStartMaxAttempts; + deviceDiscoveryStartTimeout->start(); + qCDebug(QT_BT_ANDROID) << "QBluetoothDeviceDiscoveryAgentPrivate::start() - Classic search successfully started."; return; @@ -264,9 +334,6 @@ void QBluetoothDeviceDiscoveryAgentPrivate::start(QBluetoothDeviceDiscoveryAgent } if (requestedMethods & QBluetoothDeviceDiscoveryAgent::LowEnergyMethod) { - // LE search only requested or classic discovery failed but lets try LE scan anyway - Q_ASSERT(requestedMethods & QBluetoothDeviceDiscoveryAgent::LowEnergyMethod); - if (QtAndroidPrivate::androidSdkVersion() < 18) { qCDebug(QT_BT_ANDROID) << "Skipping Bluetooth Low Energy device scan due to" "insufficient Android version."; @@ -276,7 +343,7 @@ void QBluetoothDeviceDiscoveryAgentPrivate::start(QBluetoothDeviceDiscoveryAgent emit q->error(lastError); return; } - + // LE search only requested or classic discovery failed but lets try LE scan anyway startLowEnergyScan(); } } @@ -287,6 +354,9 @@ void QBluetoothDeviceDiscoveryAgentPrivate::stop() pendingStart = false; + if (deviceDiscoveryStartTimeout) + deviceDiscoveryStartTimeout->stop(); + if (m_active == NoScanActive) return; @@ -330,16 +400,9 @@ void QBluetoothDeviceDiscoveryAgentPrivate::processSdpDiscoveryFinished() start(requestedMethods); } else { // check that it didn't finish due to turned off Bluetooth Device - const int state = adapter.callMethod("getState"); - if (state != 12) { // BluetoothAdapter.STATE_ON - m_active = NoScanActive; - lastError = QBluetoothDeviceDiscoveryAgent::PoweredOffError; - errorString = QBluetoothDeviceDiscoveryAgent::tr("Device is powered off"); - emit q->error(lastError); + if (setErrorIfPowerOff()) return; - } - - // no BTLE scan requested + // Since no BTLE scan requested and classic scan is done => finished() if (!(requestedMethods & QBluetoothDeviceDiscoveryAgent::LowEnergyMethod)) { m_active = NoScanActive; emit q->finished(); diff --git a/src/bluetooth/qbluetoothdevicediscoveryagent_p.h b/src/bluetooth/qbluetoothdevicediscoveryagent_p.h index 4fbaf734..6b9ec36a 100644 --- a/src/bluetooth/qbluetoothdevicediscoveryagent_p.h +++ b/src/bluetooth/qbluetoothdevicediscoveryagent_p.h @@ -164,6 +164,8 @@ private slots: private: void startLowEnergyScan(); + void classicDiscoveryStartFail(); + bool setErrorIfPowerOff(); DeviceDiscoveryBroadcastReceiver *receiver; QBluetoothAddress m_adapterAddress; @@ -171,6 +173,8 @@ private: QAndroidJniObject adapter; QAndroidJniObject leScanner; QTimer *leScanTimeout; + QTimer *deviceDiscoveryStartTimeout = nullptr; + short deviceDiscoveryStartAttemptsLeft; bool pendingCancel, pendingStart; #elif QT_CONFIG(bluez) diff --git a/src/bluetooth/qbluetoothservicediscoveryagent_android.cpp b/src/bluetooth/qbluetoothservicediscoveryagent_android.cpp index b42f2af3..e5024dfa 100644 --- a/src/bluetooth/qbluetoothservicediscoveryagent_android.cpp +++ b/src/bluetooth/qbluetoothservicediscoveryagent_android.cpp @@ -287,7 +287,7 @@ void QBluetoothServiceDiscoveryAgentPrivate::_q_processFetchedUuids( Q_Q(QBluetoothServiceDiscoveryAgent); QTimer::singleShot(4000, q, [this]() { this->_q_fetchUuidsTimeout(); - }); // will also call _q_seriveDiscoveryFinished() + }); // will also call _q_serviceDiscoveryFinished() } else { _q_serviceDiscoveryFinished(); } -- cgit v1.2.3 From 807c62ddee0749b68458654f7d37b28aceca6359 Mon Sep 17 00:00:00 2001 From: Juha Vuolle Date: Thu, 23 Jun 2022 15:34:22 +0300 Subject: Fix bluetooth service discovery not finishing on Android The service discovery finished signal is not emitted when the SDP cache is empty when last device inquiry of services finishes. This commit changes the logic so that the the inquiry is finished independent of whether actual services were discovered on (any) of the devices. As a related drive-by: - Document the role of sdpCache to ease understanding - Change raw timeout limits into a variable Fixes: QTBUG-104479 Change-Id: Ifc9e8587a66769a1fc7959a8154f2be72ffd7461 Reviewed-by: Alex Blasche (cherry picked from commit 7c7d860ca52dc19e994a1166b6e2d0f5fa869455) Reviewed-by: Ivan Solovev --- .../qbluetoothservicediscoveryagent_android.cpp | 29 +++++++++++++--------- src/bluetooth/qbluetoothservicediscoveryagent_p.h | 6 +++++ 2 files changed, 23 insertions(+), 12 deletions(-) (limited to 'src/bluetooth') diff --git a/src/bluetooth/qbluetoothservicediscoveryagent_android.cpp b/src/bluetooth/qbluetoothservicediscoveryagent_android.cpp index e5024dfa..ba09282d 100644 --- a/src/bluetooth/qbluetoothservicediscoveryagent_android.cpp +++ b/src/bluetooth/qbluetoothservicediscoveryagent_android.cpp @@ -57,6 +57,8 @@ QT_BEGIN_NAMESPACE Q_DECLARE_LOGGING_CATEGORY(QT_BT_ANDROID) +static constexpr auto uuidFetchTimeLimit = std::chrono::seconds{4}; + QBluetoothServiceDiscoveryAgentPrivate::QBluetoothServiceDiscoveryAgentPrivate( QBluetoothServiceDiscoveryAgent *qp, const QBluetoothAddress &deviceAdapter) : error(QBluetoothServiceDiscoveryAgent::NoError), @@ -285,7 +287,7 @@ void QBluetoothServiceDiscoveryAgentPrivate::_q_processFetchedUuids( if (address.isNull() || uuids.isEmpty()) { if (discoveredDevices.count() == 1) { Q_Q(QBluetoothServiceDiscoveryAgent); - QTimer::singleShot(4000, q, [this]() { + QTimer::singleShot(uuidFetchTimeLimit, q, [this]() { this->_q_fetchUuidsTimeout(); }); // will also call _q_serviceDiscoveryFinished() } else { @@ -304,7 +306,7 @@ void QBluetoothServiceDiscoveryAgentPrivate::_q_processFetchedUuids( qCDebug(QT_BT_ANDROID) << result; } - /* In general there are two uuid events per device. + /* In general there may be up-to two uuid events per device. * We'll wait for the second event to arrive before we process the UUIDs. * We utilize a timeout to catch cases when the second * event doesn't arrive at all. @@ -334,11 +336,10 @@ void QBluetoothServiceDiscoveryAgentPrivate::_q_processFetchedUuids( sdpCache.insert(address, pair); - //the discovery on the last device cannot immediately finish - //we have to grant the 2 seconds timeout delay - if (discoveredDevices.count() == 1) { + //we have to grant the timeout delay to allow potential second event to arrive + if (discoveredDevices.size() == 1) { Q_Q(QBluetoothServiceDiscoveryAgent); - QTimer::singleShot(4000, q, [this]() { + QTimer::singleShot(uuidFetchTimeLimit, q, [this]() { this->_q_fetchUuidsTimeout(); }); return; @@ -493,14 +494,18 @@ void QBluetoothServiceDiscoveryAgentPrivate::populateDiscoveredServices(const QB void QBluetoothServiceDiscoveryAgentPrivate::_q_fetchUuidsTimeout() { - if (sdpCache.isEmpty()) + // In practice if device list is empty, discovery has been stopped or bluetooth is offline + if (discoveredDevices.isEmpty()) return; - QPair > pair; - const QList keys = sdpCache.keys(); - for (const QBluetoothAddress &key : keys) { - pair = sdpCache.take(key); - populateDiscoveredServices(pair.first, pair.second); + // Process remaining services in the cache (these didn't get a second UUID event) + if (!sdpCache.isEmpty()) { + QPair > pair; + const QList keys = sdpCache.keys(); + for (const QBluetoothAddress &key : keys) { + pair = sdpCache.take(key); + populateDiscoveredServices(pair.first, pair.second); + } } Q_ASSERT(sdpCache.isEmpty()); diff --git a/src/bluetooth/qbluetoothservicediscoveryagent_p.h b/src/bluetooth/qbluetoothservicediscoveryagent_p.h index d163a80c..7fb57c0d 100644 --- a/src/bluetooth/qbluetoothservicediscoveryagent_p.h +++ b/src/bluetooth/qbluetoothservicediscoveryagent_p.h @@ -223,6 +223,12 @@ private: LocalDeviceBroadcastReceiver *localDeviceReceiver = nullptr; QAndroidJniObject btAdapter; + // The sdpCache caches service discovery results while it is running, and is + // cleared once finished. The cache is used as we may (or may not) get more accurate + // results after the first result. This temporary caching allows to send the + // serviceDiscovered() signal once per service and with the most accurate information. + // Partial cache clearing may occur already during the scan if the second (more accurate) + // scan result is received. QMap > > sdpCache; #endif -- cgit v1.2.3