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>

1 files changed, 44 insertions, 29 deletions

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);