Fix deadlocks in ICD and NetworkManager engines.

Ensure that locks are not held when signals are emitted.

Task-number: QTBUG-12631

4 files changed, 41 insertions, 46 deletions

diff --git a/src/plugins/bearer/icd/qicdengine.cpp b/src/plugins/bearer/icd/qicdengine.cpp
index 6060a09..0900329 100644
--- a/src/plugins/bearer/icd/qicdengine.cpp
+++ b/src/plugins/bearer/icd/qicdengine.cpp
@@ -217,7 +217,7 @@ void IapMonitor::iapRemoved(const QString &iap_id)
 }
 
 QIcdEngine::QIcdEngine(QObject *parent)
-:   QBearerEngine(parent), iapMonitor(new IapMonitor), m_dbusInterface(0),
+:   QBearerEngine(parent), iapMonitor(0), m_dbusInterface(0),
     firstUpdate(true), m_scanGoingOn(false)
 {
 }
@@ -258,6 +258,7 @@ void QIcdEngine::initialize()
     startListeningStateSignalsForAllConnections();
 
     /* Turn on IAP add/remove monitoring */
+    iapMonitor = new IapMonitor;
     iapMonitor->setup(this);
 
     /* We create a default configuration which is a pseudo config */
@@ -520,8 +521,6 @@ void QIcdEngine::addConfiguration(QString& iap_id)
 
 void QIcdEngine::doRequestUpdate(QList<Maemo::IcdScanResult> scanned)
 {
-    QMutexLocker locker(&mutex);
-
     /* Contains all known iap_ids from storage */
     QList<QString> knownConfigs = accessPointConfigurations.keys();
 
@@ -587,9 +586,9 @@ void QIcdEngine::doRequestUpdate(QList<Maemo::IcdScanResult> scanned)
             QNetworkConfigurationPrivatePointer ptr(cpPriv);
             accessPointConfigurations.insert(iap_id, ptr);
 
-            locker.unlock();
+            mutex.unlock();
             emit configurationAdded(ptr);
-            locker.relock();
+            mutex.lock();
 
 #ifdef BEARER_MANAGEMENT_DEBUG
             qDebug("IAP: %s, name: %s, ssid: %s, added to known list",
@@ -642,9 +641,9 @@ void QIcdEngine::doRequestUpdate(QList<Maemo::IcdScanResult> scanned)
                     ptr->mutex.unlock();
 
                     if (changed) {
-                        locker.unlock();
+                        mutex.unlock();
                         emit configurationChanged(ptr);
-                        locker.relock();
+                        mutex.lock();
                     }
 
                     if (!ap.scan.network_type.startsWith(QLatin1String("WLAN")))
@@ -703,9 +702,9 @@ rescan_list:
                     QNetworkConfigurationPrivatePointer ptr(cpPriv);
                     accessPointConfigurations.insert(ptr->id, ptr);
 
-                    locker.unlock();
+                    mutex.unlock();
                     emit configurationAdded(ptr);
-                    locker.relock();
+                    mutex.lock();
                 } else {
                     knownConfigs.removeOne(scanned_ssid);
                 }
@@ -733,9 +732,9 @@ rescan_list:
                     ptr->state = QNetworkConfiguration::Defined;
 
                     configLocker.unlock();
-                    locker.unlock();
+                    mutex.unlock();
                     emit configurationChanged(ptr);
-                    locker.relock();
+                    mutex.lock();
                 }
             }
         }
@@ -744,9 +743,9 @@ rescan_list:
         foreach (const QString &oldIface, knownConfigs) {
             QNetworkConfigurationPrivatePointer ptr = accessPointConfigurations.take(oldIface);
             if (ptr) {
-                locker.unlock();
+                mutex.unlock();
                 emit configurationRemoved(ptr);
-                locker.relock();
+                mutex.lock();
                 //if we would have SNAP support we would have to remove the references
                 //from existing ServiceNetworks to the removed access point configuration
             }
@@ -762,9 +761,9 @@ rescan_list:
     }
 
     if (!firstUpdate) {
-        locker.unlock();
+        mutex.unlock();
         emit updateCompleted();
-        locker.relock();
+        mutex.lock();
     }
 
     if (firstUpdate)
@@ -781,8 +780,6 @@ QNetworkConfigurationPrivatePointer QIcdEngine::defaultConfiguration()
 
 void QIcdEngine::startListeningStateSignalsForAllConnections()
 {
-    QMutexLocker locker(&mutex);
-
     // Start listening ICD_DBUS_API_STATE_SIG signals
     m_dbusInterface->connection().connect(ICD_DBUS_API_INTERFACE,
                                           ICD_DBUS_API_PATH,
@@ -906,8 +903,6 @@ void QIcdEngine::requestUpdate()
 
 void QIcdEngine::cancelAsyncConfigurationUpdate()
 {
-    QMutexLocker locker(&mutex);
-
     if (!m_scanGoingOn) {
         return;
     }
@@ -947,7 +942,9 @@ void QIcdEngine::asyncUpdateConfigurationsSlot(QDBusMessage msg)
     if (icd_scan_status == ICD_SCAN_COMPLETE) {
         m_typesToBeScanned.removeOne(arguments[6].toString());
         if (!m_typesToBeScanned.count()) {
+            locker.unlock();
             finishAsyncConfigurationUpdate();
+            locker.relock();
         }
     } else {
         Maemo::IcdScanResult scanResult;
@@ -977,7 +974,8 @@ void QIcdEngine::cleanup()
         m_scanTimer.stop();
         m_dbusInterface->call(ICD_DBUS_API_SCAN_CANCEL);
     }
-    iapMonitor->cleanup();
+    if (iapMonitor)
+        iapMonitor->cleanup();
 }
 
 bool QIcdEngine::hasIdentifier(const QString &id)
diff --git a/src/plugins/bearer/icd/qicdengine.h b/src/plugins/bearer/icd/qicdengine.h
index 16dc979..a768d84 100644
--- a/src/plugins/bearer/icd/qicdengine.h
+++ b/src/plugins/bearer/icd/qicdengine.h
@@ -126,13 +126,13 @@ public:
         QMutexLocker locker(&mutex);
 
         accessPointConfigurations.insert(ptr->id, ptr);
+
+        locker.unlock();
         emit configurationAdded(ptr);
     }
 
     inline void changedSessionConfiguration(QNetworkConfigurationPrivatePointer ptr)
     {
-        QMutexLocker locker(&mutex);
-
         emit configurationChanged(ptr);
     }
 
@@ -144,14 +144,14 @@ Q_SIGNALS:
     void iapStateChanged(const QString& iapid, uint icd_connection_state);
 
 private Q_SLOTS:
-    void doRequestUpdate(QList<Maemo::IcdScanResult> scanned = QList<Maemo::IcdScanResult>());
-    void cancelAsyncConfigurationUpdate();
     void finishAsyncConfigurationUpdate();
     void asyncUpdateConfigurationsSlot(QDBusMessage msg);
     void connectionStateSignalsSlot(QDBusMessage msg);
 
 private:
     void startListeningStateSignalsForAllConnections();
+    void doRequestUpdate(QList<Maemo::IcdScanResult> scanned = QList<Maemo::IcdScanResult>());
+    void cancelAsyncConfigurationUpdate();
 
 private:
     IapMonitor *iapMonitor;
diff --git a/src/plugins/bearer/networkmanager/qnetworkmanagerengine.cpp b/src/plugins/bearer/networkmanager/qnetworkmanagerengine.cpp
index f3f693b..29445ce 100644
--- a/src/plugins/bearer/networkmanager/qnetworkmanagerengine.cpp
+++ b/src/plugins/bearer/networkmanager/qnetworkmanagerengine.cpp
@@ -104,14 +104,23 @@ void QNetworkManagerEngine::initialize()
     QMutexLocker locker(&mutex);
 
     // Get current list of access points.
-    foreach (const QDBusObjectPath &devicePath, interface->getDevices())
+    foreach (const QDBusObjectPath &devicePath, interface->getDevices()) {
+        locker.unlock();
         deviceAdded(devicePath);
+        locker.relock();
+    }
 
     // Get connections.
-    foreach (const QDBusObjectPath &settingsPath, systemSettings->listConnections())
+    foreach (const QDBusObjectPath &settingsPath, systemSettings->listConnections()) {
+        locker.unlock();
         newConnection(settingsPath, systemSettings);
-    foreach (const QDBusObjectPath &settingsPath, userSettings->listConnections())
+        locker.relock();
+    }
+    foreach (const QDBusObjectPath &settingsPath, userSettings->listConnections()) {
+        locker.unlock();
         newConnection(settingsPath, userSettings);
+        locker.relock();
+    }
 
     // Get active connections.
     foreach (const QDBusObjectPath &acPath, interface->activeConnections()) {
@@ -132,11 +141,6 @@ bool QNetworkManagerEngine::networkManagerAvailable() const
     return interface->isValid();
 }
 
-void QNetworkManagerEngine::doRequestUpdate()
-{
-    emit updateCompleted();
-}
-
 QString QNetworkManagerEngine::getInterfaceFromId(const QString &id)
 {
     QMutexLocker locker(&mutex);
@@ -233,9 +237,7 @@ void QNetworkManagerEngine::disconnectFromId(const QString &id)
 
 void QNetworkManagerEngine::requestUpdate()
 {
-    QMutexLocker locker(&mutex);
-
-    QTimer::singleShot(0, this, SLOT(doRequestUpdate()));
+    QMetaObject::invokeMethod(this, "updateCompleted", Qt::QueuedConnection);
 }
 
 void QNetworkManagerEngine::interfacePropertiesChanged(const QString &path,
@@ -361,13 +363,10 @@ void QNetworkManagerEngine::devicePropertiesChanged(const QString &path,
 
 void QNetworkManagerEngine::deviceAdded(const QDBusObjectPath &path)
 {
-    QMutexLocker locker(&mutex);
-
     QNetworkManagerInterfaceDevice device(path.path());
     if (device.deviceType() == DEVICE_TYPE_802_11_WIRELESS) {
         QNetworkManagerInterfaceDeviceWireless *wirelessDevice =
             new QNetworkManagerInterfaceDeviceWireless(device.connectionInterface()->path());
-        wirelessDevices.insert(path.path(), wirelessDevice);
 
         wirelessDevice->setConnections();
         connect(wirelessDevice, SIGNAL(accessPointAdded(QString,QDBusObjectPath)),
@@ -379,6 +378,10 @@ void QNetworkManagerEngine::deviceAdded(const QDBusObjectPath &path)
 
         foreach (const QDBusObjectPath &apPath, wirelessDevice->getAccessPoints())
             newAccessPoint(QString(), apPath);
+
+        mutex.lock();
+        wirelessDevices.insert(path.path(), wirelessDevice);
+        mutex.unlock();
     }
 }
 
@@ -685,8 +688,6 @@ QNetworkConfigurationPrivate *QNetworkManagerEngine::parseConnection(const QStri
                                                                      const QString &settingsPath,
                                                                      const QNmSettingsMap &map)
 {
-    QMutexLocker locker(&mutex);
-
     QNetworkConfigurationPrivate *cpPriv = new QNetworkConfigurationPrivate;
     cpPriv->name = map.value("connection").value("id").toString();
     cpPriv->isValid = true;
@@ -735,9 +736,9 @@ QNetworkConfigurationPrivate *QNetworkManagerEngine::parseConnection(const QStri
                     QNetworkConfigurationPrivatePointer ptr =
                         accessPointConfigurations.take(accessPointId);
 
-                    locker.unlock();
+                    mutex.unlock();
                     emit configurationRemoved(ptr);
-                    locker.relock();
+                    mutex.lock();
                 }
                 break;
             }
@@ -753,8 +754,6 @@ QNetworkConfigurationPrivate *QNetworkManagerEngine::parseConnection(const QStri
 
 QNetworkManagerSettingsConnection *QNetworkManagerEngine::connectionFromId(const QString &id) const
 {
-    QMutexLocker locker(&mutex);
-
     for (int i = 0; i < connections.count(); ++i) {
         QNetworkManagerSettingsConnection *connection = connections.at(i);
         const QString service = connection->connectionInterface()->service();
diff --git a/src/plugins/bearer/networkmanager/qnetworkmanagerengine.h b/src/plugins/bearer/networkmanager/qnetworkmanagerengine.h
index ffb8395..78ebb0a 100644
--- a/src/plugins/bearer/networkmanager/qnetworkmanagerengine.h
+++ b/src/plugins/bearer/networkmanager/qnetworkmanagerengine.h
@@ -116,8 +116,6 @@ private Q_SLOTS:
     void removeAccessPoint(const QString &path, const QDBusObjectPath &objectPath);
     void updateAccessPoint(const QMap<QString, QVariant> &map);
 
-    void doRequestUpdate();
-
 private:
     QNetworkConfigurationPrivate *parseConnection(const QString &service,
                                                   const QString &settingsPath,