QFileInfoGatherer: fix race conditions pt.1: abort

Fix a race on the 'abort' variable. While there was a mutex lock around
the code that sets the variable in ~QFileInfoGatherer, there was no
protection in getFileInfos(), where it is read:

   // T:this->thread()  // T:*this
                        // in getFileInfos(), after last mutex.unlock()
   mutex.lock();
   abort = true;        while (!abort...
   // ...               // ...

Fix by making 'abort' an atomic. This means that we can drop the mutex
locker in the destructor, too. We still mostly access 'abort' under
protection of the mutex, because we need to protect other variables that
just happen to be accessed together with 'abort', but we avoid the mutex
lock/unlock on each iteration of the while loop in getFileInfos().

Also cleaned up the logic in run():

- by using the canonical form of condition.wait() (in a loop that
  checks the condition), we can ensure that !path.isEmpty() and avoid
  having to use the updateFiles boolean.
- by checking for abort.load() after we return from
  condition.wait(), we minimise the waiting time for thread
  exit.
- by using different local names, we avoid having to this->qualify members.

Also changed one condition.wakeOne() to wakeAll() for consistency
with fetchExtendedInformation().

Change-Id: If35f338fe774546616ec287c1c37e2c32ed05f1a
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: David Faure <faure@kde.org>
Reviewed-by: Marc Mutz <marc.mutz@kdab.com>

2 files changed, 15 insertions, 24 deletions

diff --git a/src/widgets/dialogs/qfileinfogatherer.cpp b/src/widgets/dialogs/qfileinfogatherer.cpp
index 03935654b8..6d9c0cac6b 100644
--- a/src/widgets/dialogs/qfileinfogatherer.cpp
+++ b/src/widgets/dialogs/qfileinfogatherer.cpp
@@ -96,10 +96,8 @@ QFileInfoGatherer::QFileInfoGatherer(QObject *parent)
 */
 QFileInfoGatherer::~QFileInfoGatherer()
 {
-    QMutexLocker locker(&mutex);
-    abort = true;
-    condition.wakeOne();
-    locker.unlock();
+    abort.store(true);
+    condition.wakeAll();
     wait();
 }
 
@@ -204,25 +202,18 @@ void QFileInfoGatherer::list(const QString &directoryPath)
 void QFileInfoGatherer::run()
 {
     forever {
-        bool updateFiles = false;
         QMutexLocker locker(&mutex);
-        if (abort) {
-            return;
-        }
-        if (this->path.isEmpty())
+        while (!abort.load() && path.isEmpty())
             condition.wait(&mutex);
-        QString path;
-        QStringList list;
-        if (!this->path.isEmpty()) {
-            path = this->path.first();
-            list = this->files.first();
-            this->path.pop_front();
-            this->files.pop_front();
-            updateFiles = true;
-        }
+        if (abort.load())
+            return;
+        const QString thisPath = path.front();
+        path.pop_front();
+        const QStringList thisList = files.front();
+        files.pop_front();
         locker.unlock();
-        if (updateFiles)
-            getFileInfos(path, list);
+
+        getFileInfos(thisPath, thisList);
     }
 }
 
@@ -316,7 +307,7 @@ void QFileInfoGatherer::getFileInfos(const QString &path, const QStringList &fil
     QString itPath = QDir::fromNativeSeparators(files.isEmpty() ? path : QLatin1String(""));
     QDirIterator dirIt(itPath, QDir::AllEntries | QDir::System | QDir::Hidden);
     QStringList allFiles;
-    while(!abort && dirIt.hasNext()) {
+    while (!abort.load() && dirIt.hasNext()) {
         dirIt.next();
         fileInfo = dirIt.fileInfo();
         allFiles.append(fileInfo.fileName());
@@ -326,7 +317,7 @@ void QFileInfoGatherer::getFileInfos(const QString &path, const QStringList &fil
         emit newListOfFiles(path, allFiles);
 
     QStringList::const_iterator filesIt = filesToCheck.constBegin();
-    while(!abort && filesIt != filesToCheck.constEnd()) {
+    while (!abort.load() && filesIt != filesToCheck.constEnd()) {
         fileInfo.setFile(path + QDir::separator() + *filesIt);
         ++filesIt;
         fetch(fileInfo, base, firstTime, updatedFiles, path);
diff --git a/src/widgets/dialogs/qfileinfogatherer_p.h b/src/widgets/dialogs/qfileinfogatherer_p.h
index c07c908a55..cc03e99e08 100644
--- a/src/widgets/dialogs/qfileinfogatherer_p.h
+++ b/src/widgets/dialogs/qfileinfogatherer_p.h
@@ -181,9 +181,9 @@ private:
     void fetch(const QFileInfo &info, QElapsedTimer &base, bool &firstTime, QList<QPair<QString, QFileInfo> > &updatedFiles, const QString &path);
     QString translateDriveName(const QFileInfo &drive) const;
 
-    QMutex mutex;
+    mutable QMutex mutex;
     QWaitCondition condition;
-    volatile bool abort;
+    QAtomicInt abort;
 
     QStack<QString> path;
     QStack<QStringList> files;