TSAN: fix data races in PackageManagerv6.7.0-beta1

These have been there for years and have never triggered, but now that
TSAN found the races, it's time to fix them.

Change-Id: I31fc3e9254d1a2145198621d55c812cd7226510b
Pick-to: 6.6 6.5
Reviewed-by: Dominik Holland <dominik.holland@qt.io>
(cherry picked from commit 4cf77c5cc9dd569aee4a92a6b540d10cb928da82)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

6 files changed, 28 insertions, 22 deletions

diff --git a/src/manager-lib/asynchronoustask.cpp b/src/manager-lib/asynchronoustask.cpp
index 17871da8..d750946f 100644
--- a/src/manager-lib/asynchronoustask.cpp
+++ b/src/manager-lib/asynchronoustask.cpp
@@ -25,29 +25,29 @@ QString AsynchronousTask::id() const
 
 AsynchronousTask::TaskState AsynchronousTask::state() const
 {
+    QMutexLocker locker(&m_mutex);
     return m_state;
 }
 
 void AsynchronousTask::setState(AsynchronousTask::TaskState state)
 {
+    QMutexLocker locker(&m_mutex);
     if (m_state != state) {
         m_state = state;
-        emit stateChanged(m_state);
+        locker.unlock();
+        emit stateChanged(state);
     }
 }
 
-bool AsynchronousTask::hasFailed() const
-{
-    return (m_state == Failed);
-}
-
 Error AsynchronousTask::errorCode() const
 {
+    QMutexLocker locker(&m_mutex);
     return m_errorCode;
 }
 
 QString AsynchronousTask::errorString() const
 {
+    QMutexLocker locker(&m_mutex);
     return m_errorString;
 }
 
@@ -59,7 +59,7 @@ bool AsynchronousTask::cancel()
 
 bool AsynchronousTask::forceCancel()
 {
-    if (m_state == Queued) {
+    if (state() == Queued) {
         setError(Error::Canceled, qSL("canceled"));
         return true;
     }
@@ -68,6 +68,7 @@ bool AsynchronousTask::forceCancel()
 
 QString AsynchronousTask::packageId() const
 {
+    QMutexLocker locker(&m_mutex);
     return m_packageId;
 }
 
@@ -83,8 +84,12 @@ bool AsynchronousTask::postExecute()
 
 void AsynchronousTask::setError(Error errorCode, const QString &errorString)
 {
-    m_errorCode = errorCode;
-    m_errorString = errorString;
+    {
+        // setState() also locks
+        QMutexLocker locker(&m_mutex);
+        m_errorCode = errorCode;
+        m_errorString = errorString;
+    }
     setState(Failed);
 }
 
diff --git a/src/manager-lib/asynchronoustask.h b/src/manager-lib/asynchronoustask.h
index b3e3d678..b8b8f611 100644
--- a/src/manager-lib/asynchronoustask.h
+++ b/src/manager-lib/asynchronoustask.h
@@ -39,7 +39,6 @@ public:
     TaskState state() const;
     void setState(TaskState state);
 
-    bool hasFailed() const;
     Error errorCode() const;
     QString errorString() const;
 
@@ -61,7 +60,7 @@ protected:
     void run() override final;
 
 protected:
-    QMutex m_mutex;
+    mutable QMutex m_mutex;
 
     QString m_id;
     QString m_packageId;
diff --git a/src/manager-lib/deinstallationtask.cpp b/src/manager-lib/deinstallationtask.cpp
index 7f0b778b..9b4f2d31 100644
--- a/src/manager-lib/deinstallationtask.cpp
+++ b/src/manager-lib/deinstallationtask.cpp
@@ -29,6 +29,7 @@ DeinstallationTask::DeinstallationTask(const QString &packageId, const QString &
 
 bool DeinstallationTask::cancel()
 {
+    QMutexLocker locker(&m_mutex);
     if (m_canBeCanceled)
         m_canceled = true;
     return m_canceled;
@@ -70,10 +71,12 @@ void DeinstallationTask::execute()
         while (!m_canceled && !package->areAllApplicationsStoppedDueToBlock())
             QThread::msleep(30);
 
-        // there's a small race condition here, but not doing a planned cancellation isn't harmful
-        m_canBeCanceled = false;
-        if (m_canceled)
-            throw Exception(Error::Canceled, "canceled");
+        {
+            QMutexLocker locker(&m_mutex);
+            m_canBeCanceled = false;
+            if (m_canceled)
+                throw Exception(Error::Canceled, "canceled");
+        }
 
         ScopedRenamer docDirRename;
         ScopedRenamer appDirRename;
diff --git a/src/manager-lib/deinstallationtask.h b/src/manager-lib/deinstallationtask.h
index 63e6613b..3159a3d1 100644
--- a/src/manager-lib/deinstallationtask.h
+++ b/src/manager-lib/deinstallationtask.h
@@ -30,7 +30,7 @@ private:
     QString m_documentPath;
     bool m_keepDocuments;
     bool m_canBeCanceled = true;
-    bool m_canceled = false;
+    QAtomicInteger<bool> m_canceled = false; // atomic for easy access in "busy" wait loop
 };
 
 QT_END_NAMESPACE_AM
diff --git a/src/manager-lib/installationtask.cpp b/src/manager-lib/installationtask.cpp
index 1bf965a5..cebfb9d2 100644
--- a/src/manager-lib/installationtask.cpp
+++ b/src/manager-lib/installationtask.cpp
@@ -263,7 +263,9 @@ void InstallationTask::checkExtractedFile(const QString &file) Q_DECL_NOEXCEPT_E
         if (m_iconFileName.isEmpty())
             throw Exception(Error::Package, "the 'icon' field in info.yaml cannot be empty or absent.");
 
+        m_mutex.lock();
         m_packageId = m_package->id();
+        m_mutex.unlock();
 
         m_foundInfo = true;
     } else if (m_extractedFileCount == 2) {
diff --git a/src/manager-lib/packagemanager.cpp b/src/manager-lib/packagemanager.cpp
index 7b73f5b0..30731f83 100644
--- a/src/manager-lib/packagemanager.cpp
+++ b/src/manager-lib/packagemanager.cpp
@@ -1254,9 +1254,7 @@ void PackageManager::executeNextTask()
 
     AsynchronousTask *task = d->incomingTaskList.takeFirst();
 
-    if (task->hasFailed()) {
-        task->setState(AsynchronousTask::Failed);
-
+    if (task->state() == AsynchronousTask::Failed) {
         handleFailure(task);
 
         task->deleteLater();
@@ -1285,11 +1283,10 @@ void PackageManager::executeNextTask()
     });
 
     connect(task, &AsynchronousTask::finished, this, [this, task]() {
-        task->setState(task->hasFailed() ? AsynchronousTask::Failed : AsynchronousTask::Finished);
-
-        if (task->hasFailed()) {
+        if (task->state() == AsynchronousTask::Failed) {
             handleFailure(task);
         } else {
+            task->setState(AsynchronousTask::Finished);
             qCDebug(LogInstaller) << "emit finished" << task->id();
             emit taskFinished(task->id());
         }