Adds a better state transition support to the BluetoothModel

When the model changes quickly from one discovery mode to the next, the
public device discovery agent and the internal device discovery agent
inside QBlutoothServiceDiscoveryAgent interact with each other. One agent
is shutting down while the other is starting. Since both instances
manage the same hardware, the resulting signals hit the agents in
unexpected states which even leads to random crashes.

The fix is to let one agent stop and only start the next one once the
first has properly shut down. Unfortunately the public BluetoothModel
API acts synchronously via its running property. Therefore the
synchronous running property must be mapped to asynchronous state changes
inside the agents. To achieve this the model uses an internal state
transition table which determines how the next setRunning(bool) call
is to be processed.

As a consequence it is possible to have a non-running BluetoothModel
but the internal agent is still shutting down. Another scenario
might exhibit a running model whereas one internal agent is still
finishing up and the next is waiting to start.

Task-number: QTBUG-51307
Change-Id: I0a471b913b8784d2218a797442cee7ee4d00edf3
Reviewed-by: Christian Kandeler <christian.kandeler@theqtcompany.com>
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@theqtcompany.com>
Reviewed-by: Oliver Wolff <oliver.wolff@qt.io>

2 files changed, 211 insertions, 50 deletions

diff --git a/src/imports/bluetooth/qdeclarativebluetoothdiscoverymodel.cpp b/src/imports/bluetooth/qdeclarativebluetoothdiscoverymodel.cpp
index c9559296..1f5f5f01 100644
--- a/src/imports/bluetooth/qdeclarativebluetoothdiscoverymodel.cpp
+++ b/src/imports/bluetooth/qdeclarativebluetoothdiscoverymodel.cpp
@@ -96,7 +96,10 @@ public:
         m_discoveryMode(QDeclarativeBluetoothDiscoveryModel::MinimalServiceDiscovery),
         m_running(false),
         m_runningRequested(true),
-        m_componentCompleted(false)
+        m_componentCompleted(false),
+        m_currentState(QDeclarativeBluetoothDiscoveryModel::IdleAction),
+        m_nextState(QDeclarativeBluetoothDiscoveryModel::IdleAction),
+        m_wasDirectDeviceAgentCancel(false)
     {
     }
     ~QDeclarativeBluetoothDiscoveryModelPrivate()
@@ -122,12 +125,33 @@ public:
     bool m_runningRequested;
     bool m_componentCompleted;
     QString m_remoteAddress;
+
+    QDeclarativeBluetoothDiscoveryModel::Action m_currentState;
+    QDeclarativeBluetoothDiscoveryModel::Action m_nextState;
+    bool m_wasDirectDeviceAgentCancel;
 };
 
 QDeclarativeBluetoothDiscoveryModel::QDeclarativeBluetoothDiscoveryModel(QObject *parent) :
     QAbstractListModel(parent),
     d(new QDeclarativeBluetoothDiscoveryModelPrivate)
 {
+    d->m_deviceAgent = new QBluetoothDeviceDiscoveryAgent(this);
+    connect(d->m_deviceAgent, SIGNAL(deviceDiscovered(QBluetoothDeviceInfo)),
+            this, SLOT(deviceDiscovered(QBluetoothDeviceInfo)));
+    connect(d->m_deviceAgent, SIGNAL(finished()), this, SLOT(finishedDiscovery()));
+    connect(d->m_deviceAgent, SIGNAL(canceled()), this, SLOT(finishedDiscovery()));
+    connect(d->m_deviceAgent, SIGNAL(error(QBluetoothDeviceDiscoveryAgent::Error)),
+            this, SLOT(errorDeviceDiscovery(QBluetoothDeviceDiscoveryAgent::Error)));
+    d->m_deviceAgent->setObjectName("DeviceDiscoveryAgent");
+
+    d->m_serviceAgent = new QBluetoothServiceDiscoveryAgent(this);
+    connect(d->m_serviceAgent, SIGNAL(serviceDiscovered(QBluetoothServiceInfo)),
+            this, SLOT(serviceDiscovered(QBluetoothServiceInfo)));
+    connect(d->m_serviceAgent, SIGNAL(finished()), this, SLOT(finishedDiscovery()));
+    connect(d->m_serviceAgent, SIGNAL(canceled()), this, SLOT(finishedDiscovery()));
+    connect(d->m_serviceAgent, SIGNAL(error(QBluetoothServiceDiscoveryAgent::Error)),
+            this, SLOT(errorDiscovery(QBluetoothServiceDiscoveryAgent::Error)));
+    d->m_serviceAgent->setObjectName("ServiceDiscoveryAgent");
 
     QHash<int, QByteArray> roleNames;
     roleNames = QAbstractItemModel::roleNames();
@@ -174,8 +198,8 @@ void QDeclarativeBluetoothDiscoveryModel::errorDeviceDiscovery(QBluetoothDeviceD
 
     //QBluetoothDeviceDiscoveryAgent::finished() signal is not emitted in case of an error
     //Note that this behavior is different from QBluetoothServiceDiscoveryAgent.
-    //This reset the models running flag.
-    setRunning(false);
+    //This resets the models running flag.
+    finishedDiscovery();
 }
 
 void QDeclarativeBluetoothDiscoveryModel::clearModel()
@@ -331,7 +355,35 @@ void QDeclarativeBluetoothDiscoveryModel::deviceDiscovered(const QBluetoothDevic
 
 void QDeclarativeBluetoothDiscoveryModel::finishedDiscovery()
 {
-    setRunning(false);
+    QDeclarativeBluetoothDiscoveryModel::Action previous = d->m_currentState;
+    d->m_currentState = IdleAction;
+
+    switch (previous) {
+    case IdleAction:
+        // last transition didn't even start
+        // can happen when start() or stop() immediately returned
+        // usually this happens within a current transitionToNextAction call
+        break;
+    case StopAction:
+        qCDebug(QT_BT_QML) << "Agent cancel detected";
+        transitionToNextAction();
+        break;
+    default: // all other
+        qCDebug(QT_BT_QML) << "Discovery finished" << sender()->objectName();
+
+        //TODO Qt6 This hack below is once again due to the pendingCancel logic
+        //         because QBluetoothDeviceDiscoveryAgent::isActive() is not reliable.
+        //         In toggleStartStop() we need to know whether the stop() is delayed or immediate.
+        //         isActive() cannot be used. Hence we have to wait for the canceled() signal.
+        //         Android, WinRT and Bluez5 are immediate, Bluez4 is always delayed.
+        //         The immediate case is what we catch here.
+        if (sender() == d->m_deviceAgent && d->m_nextState == StopAction) {
+            d->m_wasDirectDeviceAgentCancel = true;
+            return;
+        }
+        setRunning(false);
+        break;
+    }
 }
 
 /*!
@@ -361,6 +413,131 @@ void QDeclarativeBluetoothDiscoveryModel::setDiscoveryMode(DiscoveryMode discove
     emit discoveryModeChanged();
 }
 
+
+void QDeclarativeBluetoothDiscoveryModel::updateNextAction(Action action)
+{
+    qCDebug(QT_BT_QML) << "New action queue:"
+                       << d->m_currentState << d->m_nextState << action;
+
+    if (action == IdleAction)
+        return;
+
+    switch (d->m_nextState) {
+    case IdleAction:
+        d->m_nextState = action;
+        return;
+    case StopAction:
+        qWarning() << "Invalid Stop state when processing new action" << action;
+        return;
+    case DeviceDiscoveryAction:
+    case MinimalServiceDiscoveryAction:
+    case FullServiceDiscoveryAction:
+        if (action == StopAction) // cancel out previous start call
+            d->m_nextState = IdleAction;
+        else
+            qWarning() << "Ignoring new DMF state while another DMF state is scheduled.";
+        return;
+    }
+}
+
+void QDeclarativeBluetoothDiscoveryModel::transitionToNextAction()
+{
+    qCDebug(QT_BT_QML) << "Before transition change:" << d->m_currentState << d->m_nextState;
+    bool isRunning;
+    switch (d->m_currentState) {
+    case IdleAction:
+        switch (d->m_nextState) {
+        case IdleAction: break; // nothing to do
+        case StopAction: d->m_nextState = IdleAction; break; // clear, nothing to do
+        case DeviceDiscoveryAction:
+        case MinimalServiceDiscoveryAction:
+        case FullServiceDiscoveryAction:
+            Action temp = d->m_nextState;
+            clearModel();
+            isRunning = toggleStartStop(d->m_nextState);
+            d->m_nextState = IdleAction;
+            if (isRunning) {
+                d->m_currentState = temp;
+            } else {
+                if (temp != DeviceDiscoveryAction )
+                    errorDiscovery(d->m_serviceAgent->error());
+                d->m_running = false;
+            }
+        }
+        break;
+    case StopAction:
+        break; // do nothing, StopAction cleared by finished()/cancelled()/error() handlers
+    case DeviceDiscoveryAction:
+    case MinimalServiceDiscoveryAction:
+    case FullServiceDiscoveryAction:
+        switch (d->m_nextState) {
+        case IdleAction: break;
+        case StopAction:
+            isRunning = toggleStartStop(StopAction);
+            (isRunning) ? d->m_currentState = StopAction : d->m_currentState = IdleAction;
+            d->m_nextState = IdleAction;
+            break;
+         default:
+            Q_ASSERT(false); // should never happen
+            break;
+        }
+
+        break;
+    }
+
+    qCDebug(QT_BT_QML) << "After transition change:" << d->m_currentState << d->m_nextState;
+}
+
+// Returns true if the agent is active
+// this can be used to detect whether the agent still needs time to
+// perform the requested action.
+bool QDeclarativeBluetoothDiscoveryModel::toggleStartStop(Action action)
+{
+    Q_ASSERT(action != IdleAction);
+    switch (action) {
+    case DeviceDiscoveryAction:
+        Q_ASSERT(!d->m_deviceAgent->isActive() && !d->m_serviceAgent->isActive());
+        d->m_deviceAgent->start();
+        return d->m_deviceAgent->isActive();
+    case MinimalServiceDiscoveryAction:
+    case FullServiceDiscoveryAction:
+        Q_ASSERT(!d->m_deviceAgent->isActive() && !d->m_serviceAgent->isActive());
+        d->m_serviceAgent->setRemoteAddress(QBluetoothAddress(d->m_remoteAddress));
+        d->m_serviceAgent->clear();
+
+        if (!d->m_uuid.isEmpty())
+            d->m_serviceAgent->setUuidFilter(QBluetoothUuid(d->m_uuid));
+
+        if (action == FullServiceDiscoveryAction)  {
+            qCDebug(QT_BT_QML) << "Full Discovery";
+            d->m_serviceAgent->start(QBluetoothServiceDiscoveryAgent::FullDiscovery);
+        } else {
+            qCDebug(QT_BT_QML) << "Minimal Discovery";
+            d->m_serviceAgent->start(QBluetoothServiceDiscoveryAgent::MinimalDiscovery);
+        }
+        return d->m_serviceAgent->isActive();
+    case StopAction:
+        Q_ASSERT(d->m_currentState != StopAction && d->m_currentState != IdleAction);
+        if (d->m_currentState == DeviceDiscoveryAction) {
+            d->m_deviceAgent->stop();
+
+            // TODO Qt6 Crude hack below
+            // cannot use isActive() below due to pendingCancel logic
+            // we always wait for canceled() signal coming through or check
+            // for directly invoked cancel() response caused by stop() above
+            bool stillActive = !d->m_wasDirectDeviceAgentCancel;
+            d->m_wasDirectDeviceAgentCancel = false;
+            return stillActive;
+        } else {
+            d->m_serviceAgent->stop();
+            return d->m_serviceAgent->isActive();
+        }
+    default:
+        return true;
+    }
+}
+
+
 /*!
   \qmlproperty bool BluetoothDiscoveryModel::running
 
@@ -386,55 +563,23 @@ void QDeclarativeBluetoothDiscoveryModel::setRunning(bool running)
 
     d->m_running = running;
 
-    if (!running) {
-        if (d->m_deviceAgent)
-            d->m_deviceAgent->stop();
-        if (d->m_serviceAgent)
-            d->m_serviceAgent->stop();
+    Action nextAction = IdleAction;
+    if (running) {
+        if (discoveryMode() == MinimalServiceDiscovery)
+            nextAction = MinimalServiceDiscoveryAction;
+        else if (discoveryMode() == FullServiceDiscovery)
+            nextAction = FullServiceDiscoveryAction;
+        else
+            nextAction = DeviceDiscoveryAction;
     } else {
-        clearModel();
-        d->m_error = NoError;
-        if (d->m_discoveryMode == DeviceDiscovery) {
-            if (!d->m_deviceAgent) {
-                d->m_deviceAgent = new QBluetoothDeviceDiscoveryAgent(this);
-                connect(d->m_deviceAgent, SIGNAL(deviceDiscovered(QBluetoothDeviceInfo)), this, SLOT(deviceDiscovered(QBluetoothDeviceInfo)));
-                connect(d->m_deviceAgent, SIGNAL(finished()), this, SLOT(finishedDiscovery()));
-                connect(d->m_deviceAgent, SIGNAL(canceled()), this, SLOT(finishedDiscovery()));
-                connect(d->m_deviceAgent, SIGNAL(error(QBluetoothDeviceDiscoveryAgent::Error)), this, SLOT(errorDeviceDiscovery(QBluetoothDeviceDiscoveryAgent::Error)));
-            }
-            d->m_deviceAgent->start();
-        } else {
-            if (!d->m_serviceAgent) {
-                d->m_serviceAgent = new QBluetoothServiceDiscoveryAgent(this);
-                connect(d->m_serviceAgent, SIGNAL(serviceDiscovered(QBluetoothServiceInfo)), this, SLOT(serviceDiscovered(QBluetoothServiceInfo)));
-                connect(d->m_serviceAgent, SIGNAL(finished()), this, SLOT(finishedDiscovery()));
-                connect(d->m_serviceAgent, SIGNAL(canceled()), this, SLOT(finishedDiscovery()));
-                connect(d->m_serviceAgent, SIGNAL(error(QBluetoothServiceDiscoveryAgent::Error)), this, SLOT(errorDiscovery(QBluetoothServiceDiscoveryAgent::Error)));
-            }
-
-            d->m_serviceAgent->setRemoteAddress(QBluetoothAddress(d->m_remoteAddress));
-            d->m_serviceAgent->clear();
-
-            if (!d->m_uuid.isEmpty())
-                d->m_serviceAgent->setUuidFilter(QBluetoothUuid(d->m_uuid));
-
-            if (discoveryMode() == FullServiceDiscovery)  {
-                //qDebug() << "Full Discovery";
-                d->m_serviceAgent->start(QBluetoothServiceDiscoveryAgent::FullDiscovery);
-            } else {
-                //qDebug() << "Minimal Discovery";
-                d->m_serviceAgent->start(QBluetoothServiceDiscoveryAgent::MinimalDiscovery);
-            }
-
-            // we could not start service discovery
-            if (!d->m_serviceAgent->isActive()) {
-                d->m_running = false;
-                errorDiscovery(d->m_serviceAgent->error());
-                return;
-            }
-        }
+        nextAction = StopAction;
     }
 
+    Q_ASSERT(nextAction != IdleAction);
+    updateNextAction(nextAction);
+    transitionToNextAction();
+
+    qCDebug(QT_BT_QML) << "Running state:" << d->m_running;
     emit runningChanged();
 }
 
diff --git a/src/imports/bluetooth/qdeclarativebluetoothdiscoverymodel_p.h b/src/imports/bluetooth/qdeclarativebluetoothdiscoverymodel_p.h
index a06d49c4..cebff1f2 100644
--- a/src/imports/bluetooth/qdeclarativebluetoothdiscoverymodel_p.h
+++ b/src/imports/bluetooth/qdeclarativebluetoothdiscoverymodel_p.h
@@ -111,6 +111,8 @@ public:
     DiscoveryMode discoveryMode() const;
     void setDiscoveryMode(DiscoveryMode discovery);
 
+    // TODO Qt 6 This property behaves synchronously but should really be
+    // asynchronous. The agents start/stop/restart is not immediate.
     bool running() const;
     void setRunning(bool running);
 
@@ -139,8 +141,22 @@ private slots:
 private:
     void clearModel();
 
+    enum Action {
+        IdleAction = 0,
+        StopAction,
+        DeviceDiscoveryAction,
+        MinimalServiceDiscoveryAction,
+        FullServiceDiscoveryAction
+    };
+
+    bool toggleStartStop(Action action);
+    void updateNextAction(Action action);
+    void transitionToNextAction();
+
 private:
     QDeclarativeBluetoothDiscoveryModelPrivate* d;
+    friend class QDeclarativeBluetoothDiscoveryModelPrivate;
+
 };
 
 #endif // QDECLARATIVECONTACTMODEL_P_H