Try harder to clean up the file info gatherer without blocking

QFileSystemModel uses a QFileInfoGatherer, which is a QThread, to
populate itself with the data from the file system. Some of the system
calls we make for that might block, e.g. on Windows a disconnected
network can easily result in 10 minutes of timeout.

Destroying the QFileSystemModel destroyed the QFileInfoGatherer class
member instance, and the QFileInfoGatherer destructor, being a QThread,
had to wait on itself to avoid QThread asserting when being destroyed
while still running. This means that shutting down a Qt application that
was using a QFileSystemModel, e.g. through the non-native QFileDialog,
would hang practically indefinitely instead of shutting down.

To fix this, explicitly manage the life-time of the QFileInfoGatherer.
Manage it in QFileSystemModel through a std::unique_ptr instead of as a
direct class member so that we can control its destruction. In the
QFileSystemModel destructor, signal the gather to terminate and wait
one it for one second. If that fails, schedule the gatherer for later
deletion through deleteLater(), and release it from the unique_ptr.

The QFileInfoGatherer instance might then be alive with the
QFileSystemModel destroyed. When it receives the DeferredDelete event,
try again to abort the process and wait on the thread, this time for
5 seconds.

If that also fails, then we have to make a choice. We cannot destroy
the QThread object while it's still running, so we have to either
wait indefinitely, terminate the thread, or accept a memory leak by
ignoring the deferred delete event.

If the QCoreApplication is shutting down, then we terminate the thread.
To make this safe, explicitly control when termination is allowed in the
thread - disallow it while the thread is holding a mutex, allow it while
the thread is performing I/O operations.

If the QCoreApplication is not shutting down, then we connect to the
QThread::finished signal, and delete the object in response to that.
This might or might not happen, in which case we have to accept that
there's a memory leak.

Pick-to: 6.7 6.6
Fixes: QTBUG-120062
Change-Id: I1e4243265879d61cf406b4eeb0b11b9d59b006ad
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>

4 files changed, 95 insertions, 39 deletions

diff --git a/src/gui/itemmodels/qfileinfogatherer.cpp b/src/gui/itemmodels/qfileinfogatherer.cpp
index e0f2d3c41a..94981b482e 100644
--- a/src/gui/itemmodels/qfileinfogatherer.cpp
+++ b/src/gui/itemmodels/qfileinfogatherer.cpp
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: LicenseRef-Qt-Commercial OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only
 
 #include "qfileinfogatherer_p.h"
+#include <qcoreapplication.h>
 #include <qdebug.h>
 #include <qdiriterator.h>
 #include <private/qfileinfo_p.h>
@@ -57,11 +58,40 @@ QFileInfoGatherer::QFileInfoGatherer(QObject *parent)
 */
 QFileInfoGatherer::~QFileInfoGatherer()
 {
-    abort.storeRelaxed(true);
+    requestAbort();
+    wait();
+}
+
+bool QFileInfoGatherer::event(QEvent *event)
+{
+    if (event->type() == QEvent::DeferredDelete && isRunning()) {
+        // We have been asked to shut down later but were blocked,
+        // so the owning QFileSystemModel proceeded with its shut-down
+        // and deferred the destruction of the gatherer.
+        // If we are still blocked now, then we have three bad options:
+        // terminate, wait forever (preventing the process from shutting down),
+        // or accept a memory leak.
+        requestAbort();
+        if (!wait(5000)) {
+            // If the application is shutting down, then we terminate.
+            // Otherwise assume that sooner or later the thread will finish,
+            // and we delete it then.
+            if (QCoreApplication::closingDown())
+                terminate();
+            else
+                connect(this, &QThread::finished, this, [this]{ delete this; });
+            return true;
+        }
+    }
+
+    return QThread::event(event);
+}
+
+void QFileInfoGatherer::requestAbort()
+{
+    requestInterruption();
     QMutexLocker locker(&mutex);
     condition.wakeAll();
-    locker.unlock();
-    wait();
 }
 
 void QFileInfoGatherer::setResolveSymlinks(bool enable)
@@ -280,10 +310,13 @@ void QFileInfoGatherer::list(const QString &directoryPath)
 void QFileInfoGatherer::run()
 {
     forever {
+        // Disallow termination while we are holding a mutex or can be
+        // woken up cleanly.
+        setTerminationEnabled(false);
         QMutexLocker locker(&mutex);
-        while (!abort.loadRelaxed() && path.isEmpty())
+        while (!isInterruptionRequested() && path.isEmpty())
             condition.wait(&mutex);
-        if (abort.loadRelaxed())
+        if (isInterruptionRequested())
             return;
         const QString thisPath = std::as_const(path).front();
         path.pop_front();
@@ -291,6 +324,10 @@ void QFileInfoGatherer::run()
         files.pop_front();
         locker.unlock();
 
+        // Some of the system APIs we call when gathering file infomration
+        // might hang (e.g. waiting for network), so we explicitly allow
+        // termination now.
+        setTerminationEnabled(true);
         getFileInfos(thisPath, thisList);
     }
 }
@@ -369,7 +406,7 @@ void QFileInfoGatherer::getFileInfos(const QString &path, const QStringList &fil
     QStringList allFiles;
     if (files.isEmpty()) {
         QDirIterator dirIt(path, QDir::AllEntries | QDir::System | QDir::Hidden);
-        while (!abort.loadRelaxed() && dirIt.hasNext()) {
+        while (!isInterruptionRequested() && dirIt.hasNext()) {
             fileInfo = dirIt.nextFileInfo();
             fileInfo.stat();
             allFiles.append(fileInfo.fileName());
@@ -380,7 +417,7 @@ void QFileInfoGatherer::getFileInfos(const QString &path, const QStringList &fil
         emit newListOfFiles(path, allFiles);
 
     QStringList::const_iterator filesIt = filesToCheck.constBegin();
-    while (!abort.loadRelaxed() && filesIt != filesToCheck.constEnd()) {
+    while (!isInterruptionRequested() && filesIt != filesToCheck.constEnd()) {
         fileInfo.setFile(path + QDir::separator() + *filesIt);
         ++filesIt;
         fileInfo.stat();
diff --git a/src/gui/itemmodels/qfileinfogatherer_p.h b/src/gui/itemmodels/qfileinfogatherer_p.h
index dc81cdaee1..3d5f59c22e 100644
--- a/src/gui/itemmodels/qfileinfogatherer_p.h
+++ b/src/gui/itemmodels/qfileinfogatherer_p.h
@@ -149,6 +149,8 @@ public:
     QAbstractFileIconProvider *iconProvider() const;
     bool resolveSymlinks() const;
 
+    void requestAbort();
+
 public Q_SLOTS:
     void list(const QString &directoryPath);
     void fetchExtendedInformation(const QString &path, const QStringList &files);
@@ -160,6 +162,9 @@ private Q_SLOTS:
     void driveAdded();
     void driveRemoved();
 
+protected:
+    bool event(QEvent *event) override;
+
 private:
     void run() override;
     // called by run():
@@ -176,7 +181,6 @@ private:
     QStack<QString> path;
     QStack<QStringList> files;
     // end protected by mutex
-    QAtomicInt abort;
 
 #if QT_CONFIG(filesystemwatcher)
     QFileSystemWatcher *m_watcher = nullptr;
diff --git a/src/gui/itemmodels/qfilesystemmodel.cpp b/src/gui/itemmodels/qfilesystemmodel.cpp
index f8810366df..2000afc5b2 100644
--- a/src/gui/itemmodels/qfilesystemmodel.cpp
+++ b/src/gui/itemmodels/qfilesystemmodel.cpp
@@ -438,7 +438,7 @@ QFileSystemModelPrivate::QFileSystemNode *QFileSystemModelPrivate::node(const QS
             QFileSystemModelPrivate *p = const_cast<QFileSystemModelPrivate*>(this);
             node = p->addNode(parent, element,info);
 #if QT_CONFIG(filesystemwatcher)
-            node->populate(fileInfoGatherer.getInfo(info));
+            node->populate(fileInfoGatherer->getInfo(info));
 #endif
         } else {
             node = parent->children.value(element);
@@ -479,7 +479,7 @@ void QFileSystemModel::timerEvent(QTimerEvent *event)
         for (int i = 0; i < d->toFetch.size(); ++i) {
             const QFileSystemModelPrivate::QFileSystemNode *node = d->toFetch.at(i).node;
             if (!node->hasInformation()) {
-                d->fileInfoGatherer.fetchExtendedInformation(d->toFetch.at(i).dir,
+                d->fileInfoGatherer->fetchExtendedInformation(d->toFetch.at(i).dir,
                                                  QStringList(d->toFetch.at(i).file));
             } else {
                 // qDebug("yah!, you saved a little gerbil soul");
@@ -650,7 +650,7 @@ void QFileSystemModel::fetchMore(const QModelIndex &parent)
         return;
     indexNode->populatedChildren = true;
 #if QT_CONFIG(filesystemwatcher)
-    d->fileInfoGatherer.list(filePath(parent));
+    d->fileInfoGatherer->list(filePath(parent));
 #endif
 }
 
@@ -693,7 +693,7 @@ QVariant QFileSystemModel::myComputer(int role) const
         return QFileSystemModelPrivate::myComputer();
 #if QT_CONFIG(filesystemwatcher)
     case Qt::DecorationRole:
-        return d->fileInfoGatherer.iconProvider()->icon(QAbstractFileIconProvider::Computer);
+        return d->fileInfoGatherer->iconProvider()->icon(QAbstractFileIconProvider::Computer);
 #endif
     }
     return QVariant();
@@ -734,9 +734,9 @@ QVariant QFileSystemModel::data(const QModelIndex &index, int role) const
 #if QT_CONFIG(filesystemwatcher)
             if (icon.isNull()) {
                 if (d->node(index)->isDir())
-                    icon = d->fileInfoGatherer.iconProvider()->icon(QAbstractFileIconProvider::Folder);
+                    icon = d->fileInfoGatherer->iconProvider()->icon(QAbstractFileIconProvider::Folder);
                 else
-                    icon = d->fileInfoGatherer.iconProvider()->icon(QAbstractFileIconProvider::File);
+                    icon = d->fileInfoGatherer->iconProvider()->icon(QAbstractFileIconProvider::File);
             }
 #endif // filesystemwatcher
             return icon;
@@ -816,7 +816,7 @@ QString QFileSystemModelPrivate::name(const QModelIndex &index) const
     QFileSystemNode *dirNode = node(index);
     if (
 #if QT_CONFIG(filesystemwatcher)
-        fileInfoGatherer.resolveSymlinks() &&
+        fileInfoGatherer->resolveSymlinks() &&
 #endif
         !resolvedSymLinks.isEmpty() && dirNode->isSymLink(/* ignoreNtfsSymLinks = */ true)) {
         QString fullPath = QDir::fromNativeSeparators(filePath(index));
@@ -901,7 +901,7 @@ bool QFileSystemModel::setData(const QModelIndex &idx, const QVariant &value, in
         nodeToRename->fileName = newName;
         nodeToRename->parent = parentNode;
 #if QT_CONFIG(filesystemwatcher)
-        nodeToRename->populate(d->fileInfoGatherer.getInfo(QFileInfo(parentPath, newName)));
+        nodeToRename->populate(d->fileInfoGatherer->getInfo(QFileInfo(parentPath, newName)));
 #endif
         nodeToRename->isVisible = true;
         parentNode->children[newName] = nodeToRename.release();
@@ -1326,7 +1326,7 @@ void QFileSystemModel::setOptions(Options options)
 #if QT_CONFIG(filesystemwatcher)
     Q_D(QFileSystemModel);
     if (changed.testFlag(DontWatchForChanges))
-        d->fileInfoGatherer.setWatching(!options.testFlag(DontWatchForChanges));
+        d->fileInfoGatherer->setWatching(!options.testFlag(DontWatchForChanges));
 #endif
 
     if (changed.testFlag(DontUseCustomDirectoryIcons)) {
@@ -1347,7 +1347,7 @@ QFileSystemModel::Options QFileSystemModel::options() const
     result.setFlag(DontResolveSymlinks, !resolveSymlinks());
 #if QT_CONFIG(filesystemwatcher)
     Q_D(const QFileSystemModel);
-    result.setFlag(DontWatchForChanges, !d->fileInfoGatherer.isWatching());
+    result.setFlag(DontWatchForChanges, !d->fileInfoGatherer->isWatching());
 #else
     result.setFlag(DontWatchForChanges);
 #endif
@@ -1369,7 +1369,7 @@ QString QFileSystemModel::filePath(const QModelIndex &index) const
     QFileSystemModelPrivate::QFileSystemNode *dirNode = d->node(index);
     if (dirNode->isSymLink()
 #if QT_CONFIG(filesystemwatcher)
-        && d->fileInfoGatherer.resolveSymlinks()
+        && d->fileInfoGatherer->resolveSymlinks()
 #endif
         && d->resolvedSymLinks.contains(fullPath)
         && dirNode->isDir()) {
@@ -1431,7 +1431,7 @@ QModelIndex QFileSystemModel::mkdir(const QModelIndex &parent, const QString &na
     Q_ASSERT(parentNode->children.contains(name));
     QFileSystemModelPrivate::QFileSystemNode *node = parentNode->children[name];
 #if QT_CONFIG(filesystemwatcher)
-    node->populate(d->fileInfoGatherer.getInfo(QFileInfo(dir.absolutePath() + QDir::separator() + name)));
+    node->populate(d->fileInfoGatherer->getInfo(QFileInfo(dir.absolutePath() + QDir::separator() + name)));
 #endif
     d->addVisibleFiles(parentNode, QStringList(name));
     return d->index(node);
@@ -1499,7 +1499,7 @@ QModelIndex QFileSystemModel::setRootPath(const QString &newPath)
     if (!rootPath().isEmpty() && rootPath() != "."_L1) {
         //This remove the watcher for the old rootPath
 #if QT_CONFIG(filesystemwatcher)
-        d->fileInfoGatherer.removePath(rootPath());
+        d->fileInfoGatherer->removePath(rootPath());
 #endif
         //This line "marks" the node as dirty, so the next fetchMore
         //call on the path will ask the gatherer to install a watcher again
@@ -1555,7 +1555,7 @@ void QFileSystemModel::setIconProvider(QAbstractFileIconProvider *provider)
 {
     Q_D(QFileSystemModel);
 #if QT_CONFIG(filesystemwatcher)
-    d->fileInfoGatherer.setIconProvider(provider);
+    d->fileInfoGatherer->setIconProvider(provider);
 #endif
     d->root.updateIcon(provider, QString());
 }
@@ -1567,7 +1567,7 @@ QAbstractFileIconProvider *QFileSystemModel::iconProvider() const
 {
 #if QT_CONFIG(filesystemwatcher)
     Q_D(const QFileSystemModel);
-    return d->fileInfoGatherer.iconProvider();
+    return d->fileInfoGatherer->iconProvider();
 #else
     return 0;
 #endif
@@ -1623,7 +1623,7 @@ void QFileSystemModel::setResolveSymlinks(bool enable)
 {
 #if QT_CONFIG(filesystemwatcher)
     Q_D(QFileSystemModel);
-    d->fileInfoGatherer.setResolveSymlinks(enable);
+    d->fileInfoGatherer->setResolveSymlinks(enable);
 #else
     Q_UNUSED(enable);
 #endif
@@ -1633,7 +1633,7 @@ bool QFileSystemModel::resolveSymlinks() const
 {
 #if QT_CONFIG(filesystemwatcher)
     Q_D(const QFileSystemModel);
-    return d->fileInfoGatherer.resolveSymlinks();
+    return d->fileInfoGatherer->resolveSymlinks();
 #else
     return false;
 #endif
@@ -1738,7 +1738,7 @@ bool QFileSystemModel::event(QEvent *event)
 #if QT_CONFIG(filesystemwatcher)
     Q_D(QFileSystemModel);
     if (event->type() == QEvent::LanguageChange) {
-        d->root.retranslateStrings(d->fileInfoGatherer.iconProvider(), QString());
+        d->root.retranslateStrings(d->fileInfoGatherer->iconProvider(), QString());
         return true;
     }
 #endif
@@ -1752,7 +1752,7 @@ bool QFileSystemModel::rmdir(const QModelIndex &aindex)
 #if QT_CONFIG(filesystemwatcher)
     if (success) {
         QFileSystemModelPrivate * d = const_cast<QFileSystemModelPrivate*>(d_func());
-        d->fileInfoGatherer.removePath(path);
+        d->fileInfoGatherer->removePath(path);
     }
 #endif
     return success;
@@ -1924,7 +1924,7 @@ void QFileSystemModelPrivate::_q_fileSystemChanged(const QString &path,
     for (const auto &update : updates) {
         QString fileName = update.first;
         Q_ASSERT(!fileName.isEmpty());
-        QExtendedInformation info = fileInfoGatherer.getInfo(update.second);
+        QExtendedInformation info = fileInfoGatherer->getInfo(update.second);
         bool previouslyHere = parentNode->children.contains(fileName);
         if (!previouslyHere) {
             addNode(parentNode, fileName, info.fileInfo());
@@ -2055,22 +2055,37 @@ QStringList QFileSystemModelPrivate::unwatchPathsAt(const QModelIndex &index)
         return false;
     };
 
-    const QStringList &watchedFiles = fileInfoGatherer.watchedFiles();
+    const QStringList &watchedFiles = fileInfoGatherer->watchedFiles();
     std::copy_if(watchedFiles.cbegin(), watchedFiles.cend(),
                  std::back_inserter(result), filter);
 
-    const QStringList &watchedDirectories = fileInfoGatherer.watchedDirectories();
+    const QStringList &watchedDirectories = fileInfoGatherer->watchedDirectories();
     std::copy_if(watchedDirectories.cbegin(), watchedDirectories.cend(),
                  std::back_inserter(result), filter);
 
-    fileInfoGatherer.unwatchPaths(result);
+    fileInfoGatherer->unwatchPaths(result);
     return result;
 }
 #endif // filesystemwatcher && Q_OS_WIN
 
-QFileSystemModelPrivate::QFileSystemModelPrivate() = default;
+QFileSystemModelPrivate::QFileSystemModelPrivate()
+    : fileInfoGatherer(new QFileInfoGatherer)
+{
+}
 
-QFileSystemModelPrivate::~QFileSystemModelPrivate() = default;
+QFileSystemModelPrivate::~QFileSystemModelPrivate()
+{
+    fileInfoGatherer->requestInterruption();
+    if (!fileInfoGatherer->wait(1000)) {
+        // If the thread hangs, perhaps because the network was disconnected
+        // while the gatherer was stat'ing a remote file, then don't block
+        // shutting down the model (which might block a file dialog and the
+        // main thread). Schedule the gatherer for later deletion; it's
+        // destructor will wait for the thread to finish.
+        auto *rawGatherer = fileInfoGatherer.release();
+        rawGatherer->deleteLater();
+    }
+}
 
 /*!
     \internal
@@ -2083,13 +2098,13 @@ void QFileSystemModelPrivate::init()
 
     qRegisterMetaType<QList<std::pair<QString, QFileInfo>>>();
 #if QT_CONFIG(filesystemwatcher)
-    q->connect(&fileInfoGatherer, SIGNAL(newListOfFiles(QString,QStringList)),
+    q->connect(fileInfoGatherer.get(), SIGNAL(newListOfFiles(QString,QStringList)),
                q, SLOT(_q_directoryChanged(QString,QStringList)));
-    q->connect(&fileInfoGatherer, SIGNAL(updates(QString,QList<std::pair<QString,QFileInfo>>)), q,
+    q->connect(fileInfoGatherer.get(), SIGNAL(updates(QString,QList<std::pair<QString,QFileInfo>>)), q,
                SLOT(_q_fileSystemChanged(QString,QList<std::pair<QString,QFileInfo>>)));
-    q->connect(&fileInfoGatherer, SIGNAL(nameResolved(QString,QString)),
+    q->connect(fileInfoGatherer.get(), SIGNAL(nameResolved(QString,QString)),
             q, SLOT(_q_resolvedName(QString,QString)));
-    q->connect(&fileInfoGatherer, SIGNAL(directoryLoaded(QString)),
+    q->connect(fileInfoGatherer.get(), SIGNAL(directoryLoaded(QString)),
                q, SIGNAL(directoryLoaded(QString)));
 #endif // filesystemwatcher
     q->connect(&delayedSortTimer, SIGNAL(timeout()), q, SLOT(_q_performDelayedSort()), Qt::QueuedConnection);
diff --git a/src/gui/itemmodels/qfilesystemmodel_p.h b/src/gui/itemmodels/qfilesystemmodel_p.h
index b46be018a9..784c135357 100644
--- a/src/gui/itemmodels/qfilesystemmodel_p.h
+++ b/src/gui/itemmodels/qfilesystemmodel_p.h
@@ -259,9 +259,9 @@ public:
 #if QT_CONFIG(filesystemwatcher)
 #  ifdef Q_OS_WIN
     QStringList unwatchPathsAt(const QModelIndex &);
-    void watchPaths(const QStringList &paths) { fileInfoGatherer.watchPaths(paths); }
+    void watchPaths(const QStringList &paths) { fileInfoGatherer->watchPaths(paths); }
 #  endif // Q_OS_WIN
-    QFileInfoGatherer fileInfoGatherer;
+    std::unique_ptr<QFileInfoGatherer> fileInfoGatherer;
 #endif // filesystemwatcher
     QTimer delayedSortTimer;
     QHash<const QFileSystemNode*, bool> bypassFilters;