Fix BT LE service addition timing issue on Android

If services were added in a tight loop sometimes the services are
created wrong. In practice this results in a situation where a client
reads a characteristic value from "Service A", but gets the value from
"Service B" - even if the client had no knowledge of "Service B".

The problem is that according to Android documentation, the
BluetoothGattServer::addService() must not be called before the prior
added service has received a
BluetoothGattServerCallback::onServiceAdded() -callback.

This commit serializes/queues the service additions.

Fixes: QTBUG-96742
Change-Id: I42c980600419787d4490d1a1059e3893597cb7cf
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>
(cherry picked from commit 4deb789fe67615ebfa99af6f1071d20b0265a2e9)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

2 files changed, 52 insertions, 4 deletions

diff --git a/src/android/bluetooth/src/org/qtproject/qt/android/bluetooth/QtBluetoothLEServer.java b/src/android/bluetooth/src/org/qtproject/qt/android/bluetooth/QtBluetoothLEServer.java
index 0e797e51..e3ddb095 100644
--- a/src/android/bluetooth/src/org/qtproject/qt/android/bluetooth/QtBluetoothLEServer.java
+++ b/src/android/bluetooth/src/org/qtproject/qt/android/bluetooth/QtBluetoothLEServer.java
@@ -84,6 +84,11 @@ public class QtBluetoothLEServer {
     private BluetoothGattServer mGattServer = null;
     private BluetoothLeAdvertiser mLeAdvertiser = null;
 
+    // Note: service additions -list is accessed from different threads as it is manipulated
+    // from Qt/JNI as well as from Android callbacks => synchronize the access
+    private ArrayList<BluetoothGattService> mPendingServiceAdditions =
+                      new ArrayList<BluetoothGattService>();
+
     private String mRemoteName = "";
     public String remoteName() { return mRemoteName; }
 
@@ -343,6 +348,10 @@ public class QtBluetoothLEServer {
             // If last client disconnected, close down the server
             if (qtControllerState == 0) { // QLowEnergyController::UnconnectedState
                 mGattServer.close();
+                synchronized (mPendingServiceAdditions)
+                {
+                    mPendingServiceAdditions.clear();
+                }
                 mGattServer = null;
                 mRemoteName = "";
                 mRemoteAddress = "";
@@ -364,6 +373,31 @@ public class QtBluetoothLEServer {
         @Override
         public void onServiceAdded(int status, BluetoothGattService service) {
             super.onServiceAdded(status, service);
+            Log.d(TAG, "Service " + service.getUuid().toString() + " addition result: " + status);
+
+            // Remove the indicated service from the pending queue
+            synchronized (mPendingServiceAdditions)
+            {
+                ListIterator<BluetoothGattService> iterator = mPendingServiceAdditions.listIterator();
+                while (iterator.hasNext()) {
+                    if (iterator.next().getUuid().equals(service.getUuid())) {
+                            iterator.remove();
+                            break;
+                    }
+                }
+
+                // If there are more services in the queue, add the next whose add initiation succeeds
+                iterator = mPendingServiceAdditions.listIterator();
+                while (iterator.hasNext()) {
+                    BluetoothGattService nextService = iterator.next();
+                    if (mGattServer.addService(nextService)) {
+                        break;
+                    } else {
+                        Log.w(TAG, "Adding service " + nextService.getUuid().toString() + " failed");
+                        iterator.remove();
+                    }
+                }
+            }
         }
 
         @Override
@@ -605,6 +639,10 @@ public class QtBluetoothLEServer {
             return;
 
         clearPendingPreparedWrites(null);
+        synchronized (mPendingServiceAdditions)
+        {
+            mPendingServiceAdditions.clear();
+        }
         mGattServer.close();
         mGattServer = null;
 
@@ -646,8 +684,20 @@ public class QtBluetoothLEServer {
             return;
         }
 
-        boolean success = mGattServer.addService(service);
-        Log.w(TAG, "Services successfully added: " + success);
+        // When we add a service, we must wait for onServiceAdded callback before adding the
+        // next one. If the pending service queue is empty it means that there are no ongoing
+        // service additions => add the service to the server. If there are services in the
+        // queue it means there is an initiated addition ongoing, and we only add to the queue.
+        synchronized (mPendingServiceAdditions) {
+            if (mPendingServiceAdditions.isEmpty()) {
+                if (mGattServer.addService(service))
+                    mPendingServiceAdditions.add(service);
+                else
+                    Log.w(TAG, "Adding service " + service.getUuid().toString() + " failed.");
+            } else {
+                mPendingServiceAdditions.add(service);
+            }
+        }
     }
 
     /*
diff --git a/tests/bluetoothtestdevice/bluetoothtestdevice.cpp b/tests/bluetoothtestdevice/bluetoothtestdevice.cpp
index ef3a15fc..4fe504b5 100644
--- a/tests/bluetoothtestdevice/bluetoothtestdevice.cpp
+++ b/tests/bluetoothtestdevice/bluetoothtestdevice.cpp
@@ -279,7 +279,6 @@ int main(int argc, char *argv[])
 
     for (const auto &serviceData : serviceDefinitions) {
         services.emplaceBack(leController->addService(serviceData));
-        std::this_thread::sleep_for (std::chrono::seconds(1));
     }
 
     leController->startAdvertising(QLowEnergyAdvertisingParameters(), advertisingData,
@@ -290,7 +289,6 @@ int main(int argc, char *argv[])
         connectioncount++;
         for (int i = 0; i < services.size(); ++i) {
             services[i].reset(leController->addService(serviceDefinitions[i]));
-            std::this_thread::sleep_for (std::chrono::seconds(1));
         }