Darwin: correct state restore when FSEventsStream starting fails

The previous state was not restored completely when adding/removing
paths resulted in a stream start failure.

It also removes an autoreleasepool in restartStream, because both
stopStream and startStream do already create an autoreleasepool of their
own. (So, this pool will always be empty.)

Change-Id: Idc674e9c040f346703ab3ec256957e787a0ade73
Reviewed-by: Marc Mutz <marc.mutz@kdab.com>
Reviewed-by: Morten Johan Sørvig <morten.sorvig@qt.io>

2 files changed, 77 insertions, 45 deletions

diff --git a/src/corelib/io/qfilesystemwatcher_fsevents.mm b/src/corelib/io/qfilesystemwatcher_fsevents.mm
index 7656530a46..c6fa6c9846 100644
--- a/src/corelib/io/qfilesystemwatcher_fsevents.mm
+++ b/src/corelib/io/qfilesystemwatcher_fsevents.mm
@@ -81,7 +81,7 @@ bool QFseventsFileSystemWatcherEngine::checkDir(DirsByName::iterator &it)
     if (res == -1) {
         needsRestart |= derefPath(info.watchedPath);
         emit emitDirectoryChanged(info.origPath, true);
-        it = watchedDirectories.erase(it);
+        it = watchingState.watchedDirectories.erase(it);
     } else if (st.st_ctimespec != info.ctime || st.st_mode != info.mode) {
         info.ctime = st.st_ctimespec;
         info.mode = st.st_mode;
@@ -132,7 +132,8 @@ bool QFseventsFileSystemWatcherEngine::rescanDirs(const QString &path)
 {
     bool needsRestart = false;
 
-    for (DirsByName::iterator it = watchedDirectories.begin(); it != watchedDirectories.end(); ) {
+    for (DirsByName::iterator it = watchingState.watchedDirectories.begin();
+            it != watchingState.watchedDirectories.end(); ) {
         if (it.key().startsWith(path))
             needsRestart |= checkDir(it);
         else
@@ -171,11 +172,12 @@ bool QFseventsFileSystemWatcherEngine::rescanFiles(const QString &path)
 {
     bool needsRestart = false;
 
-    for (FilesByPath::iterator i = watchedFiles.begin(); i != watchedFiles.end(); ) {
+    for (FilesByPath::iterator i = watchingState.watchedFiles.begin();
+            i != watchingState.watchedFiles.end(); ) {
         if (i.key().startsWith(path)) {
             needsRestart |= rescanFiles(i.value());
             if (i.value().isEmpty()) {
-                i = watchedFiles.erase(i);
+                i = watchingState.watchedFiles.erase(i);
                 continue;
             }
         }
@@ -226,8 +228,8 @@ void QFseventsFileSystemWatcherEngine::processEvent(ConstFSEventStreamRef stream
 
         if (eFlags & kFSEventStreamEventFlagRootChanged) {
             // re-check everything:
-            DirsByName::iterator dirIt = watchedDirectories.find(path);
-            if (dirIt != watchedDirectories.end())
+            DirsByName::iterator dirIt = watchingState.watchedDirectories.find(path);
+            if (dirIt != watchingState.watchedDirectories.end())
                 needsRestart |= checkDir(dirIt);
             needsRestart |= rescanFiles(path);
             continue;
@@ -237,13 +239,13 @@ void QFseventsFileSystemWatcherEngine::processEvent(ConstFSEventStreamRef stream
             needsRestart |= rescanDirs(path);
 
         // check watched directories:
-        DirsByName::iterator dirIt = watchedDirectories.find(path);
-        if (dirIt != watchedDirectories.end())
+        DirsByName::iterator dirIt = watchingState.watchedDirectories.find(path);
+        if (dirIt != watchingState.watchedDirectories.end())
             needsRestart |= checkDir(dirIt);
 
         // check watched files:
-        FilesByPath::iterator pIt = watchedFiles.find(path);
-        if (pIt != watchedFiles.end())
+        FilesByPath::iterator pIt = watchingState.watchedFiles.find(path);
+        if (pIt != watchingState.watchedFiles.end())
             needsRestart |= rescanFiles(pIt.value());
     }
 
@@ -276,12 +278,11 @@ void QFseventsFileSystemWatcherEngine::doEmitDirectoryChanged(const QString &pat
     emit directoryChanged(path, removed);
 }
 
-void QFseventsFileSystemWatcherEngine::restartStream()
+bool QFseventsFileSystemWatcherEngine::restartStream()
 {
-    QMacAutoReleasePool pool;
     QMutexLocker locker(&lock);
     stopStream();
-    startStream();
+    return startStream();
 }
 
 QFseventsFileSystemWatcherEngine *QFseventsFileSystemWatcherEngine::create(QObject *parent)
@@ -311,6 +312,7 @@ QFseventsFileSystemWatcherEngine::~QFseventsFileSystemWatcherEngine()
 {
     QMacAutoReleasePool pool;
 
+    // Stop the stream in case we have to wait for the lock below to be acquired.
     if (stream)
         FSEventStreamStop(stream);
 
@@ -334,8 +336,10 @@ QStringList QFseventsFileSystemWatcherEngine::addPaths(const QStringList &paths,
 
     QMutexLocker locker(&lock);
 
+    bool wasRunning = stream != Q_NULLPTR;
     bool needsRestart = false;
 
+    WatchingState oldState = watchingState;
     QStringList p = paths;
     QMutableListIterator<QString> it(p);
     while (it.hasNext()) {
@@ -356,7 +360,7 @@ QStringList QFseventsFileSystemWatcherEngine::addPaths(const QStringList &paths,
 
         const bool isDir = S_ISDIR(st.st_mode);
         if (isDir) {
-            if (watchedDirectories.contains(realPath))
+            if (watchingState.watchedDirectories.contains(realPath))
                 continue;
             directories->append(origPath);
             watchedPath = realPath;
@@ -371,17 +375,18 @@ QStringList QFseventsFileSystemWatcherEngine::addPaths(const QStringList &paths,
             parentPath = watchedPath;
         }
 
-        for (PathRefCounts::const_iterator i = watchedPaths.begin(), ei = watchedPaths.end(); i != ei; ++i) {
+        for (PathRefCounts::const_iterator i = watchingState.watchedPaths.begin(),
+                ei = watchingState.watchedPaths.end(); i != ei; ++i) {
             if (watchedPath.startsWith(i.key())) {
                 watchedPath = i.key();
                 break;
             }
         }
 
-        PathRefCounts::iterator it = watchedPaths.find(watchedPath);
-        if (it == watchedPaths.end()) {
+        PathRefCounts::iterator it = watchingState.watchedPaths.find(watchedPath);
+        if (it == watchingState.watchedPaths.end()) {
             needsRestart = true;
-            watchedPaths.insert(watchedPath, 1);
+            watchingState.watchedPaths.insert(watchedPath, 1);
             DEBUG("Adding '%s' to watchedPaths", qPrintable(watchedPath));
         } else {
             ++it.value();
@@ -392,18 +397,25 @@ QStringList QFseventsFileSystemWatcherEngine::addPaths(const QStringList &paths,
             DirInfo dirInfo;
             dirInfo.dirInfo = info;
             dirInfo.entries = scanForDirEntries(realPath);
-            watchedDirectories.insert(realPath, dirInfo);
+            watchingState.watchedDirectories.insert(realPath, dirInfo);
             DEBUG("-- Also adding '%s' to watchedDirectories", qPrintable(realPath));
         } else {
-            watchedFiles[parentPath].insert(realPath, info);
+            watchingState.watchedFiles[parentPath].insert(realPath, info);
             DEBUG("-- Also adding '%s' to watchedFiles", qPrintable(realPath));
         }
     }
 
     if (needsRestart) {
         stopStream();
-        if (!startStream())
+        if (!startStream()) {
+            // ok, something went wrong, let's try to restore the previous state
+            watchingState = qMove(oldState);
+            // and because we don't know which path caused the issue (if any), fail on all of them
             p = paths;
+
+            if (wasRunning)
+                startStream();
+        }
     }
 
     return p;
@@ -419,6 +431,7 @@ QStringList QFseventsFileSystemWatcherEngine::removePaths(const QStringList &pat
 
     bool needsRestart = false;
 
+    WatchingState oldState = watchingState;
     QStringList p = paths;
     QMutableListIterator<QString> it(p);
     while (it.hasNext()) {
@@ -431,10 +444,10 @@ QStringList QFseventsFileSystemWatcherEngine::removePaths(const QStringList &pat
         realPath = fi.canonicalFilePath();
 
         if (fi.isDir()) {
-            DirsByName::iterator dirIt = watchedDirectories.find(realPath);
-            if (dirIt != watchedDirectories.end()) {
+            DirsByName::iterator dirIt = watchingState.watchedDirectories.find(realPath);
+            if (dirIt != watchingState.watchedDirectories.end()) {
                 needsRestart |= derefPath(dirIt->dirInfo.watchedPath);
-                watchedDirectories.erase(dirIt);
+                watchingState.watchedDirectories.erase(dirIt);
                 directories->removeAll(origPath);
                 it.remove();
                 DEBUG("Removed directory '%s'", qPrintable(realPath));
@@ -442,15 +455,15 @@ QStringList QFseventsFileSystemWatcherEngine::removePaths(const QStringList &pat
         } else {
             QFileInfo fi(realPath);
             QString parentPath = fi.path();
-            FilesByPath::iterator pIt = watchedFiles.find(parentPath);
-            if (pIt != watchedFiles.end()) {
+            FilesByPath::iterator pIt = watchingState.watchedFiles.find(parentPath);
+            if (pIt != watchingState.watchedFiles.end()) {
                 InfoByName &filesInDir = pIt.value();
                 InfoByName::iterator fIt = filesInDir.find(realPath);
                 if (fIt != filesInDir.end()) {
                     needsRestart |= derefPath(fIt->watchedPath);
                     filesInDir.erase(fIt);
                     if (filesInDir.isEmpty())
-                        watchedFiles.erase(pIt);
+                        watchingState.watchedFiles.erase(pIt);
                     files->removeAll(origPath);
                     it.remove();
                     DEBUG("Removed file '%s'", qPrintable(realPath));
@@ -461,26 +474,33 @@ QStringList QFseventsFileSystemWatcherEngine::removePaths(const QStringList &pat
 
     locker.unlock();
 
-    if (needsRestart)
-        restartStream();
+    if (needsRestart) {
+        if (!restartStream()) {
+            watchingState = qMove(oldState);
+            startStream();
+        }
+    }
 
     return p;
 }
 
+// Returns false if FSEventStream* calls failed for some mysterious reason, true if things got a
+// thumbs-up.
 bool QFseventsFileSystemWatcherEngine::startStream()
 {
     Q_ASSERT(stream == 0);
-    QMacAutoReleasePool pool;
-    if (stream) // This shouldn't happen, but let's be nice and handle it.
+    if (stream) // Ok, this really shouldn't happen, esp. not after the assert. But let's be nice in release mode and still handle it.
         stopStream();
 
-    if (watchedPaths.isEmpty())
-        return false;
+    QMacAutoReleasePool pool;
 
-    DEBUG() << "Starting stream with paths" << watchedPaths.keys();
+    if (watchingState.watchedPaths.isEmpty())
+        return true; // we succeeded in doing nothing
 
-    NSMutableArray *pathsToWatch = [NSMutableArray arrayWithCapacity:watchedPaths.size()];
-    for (PathRefCounts::const_iterator i = watchedPaths.begin(), ei = watchedPaths.end(); i != ei; ++i)
+    DEBUG() << "Starting stream with paths" << watchingState.watchedPaths.keys();
+
+    NSMutableArray *pathsToWatch = [NSMutableArray arrayWithCapacity:watchingState.watchedPaths.size()];
+    for (PathRefCounts::const_iterator i = watchingState.watchedPaths.begin(), ei = watchingState.watchedPaths.end(); i != ei; ++i)
         [pathsToWatch addObject:i.key().toNSString()];
 
     struct FSEventStreamContext callBackInfo = {
@@ -504,7 +524,7 @@ bool QFseventsFileSystemWatcherEngine::startStream()
                                  latency,
                                  FSEventStreamCreateFlags(0));
 
-    if (!stream) {
+    if (!stream) { // nope, no way to know what went wrong, so just fail
         DEBUG() << "Failed to create stream!";
         return false;
     }
@@ -514,7 +534,7 @@ bool QFseventsFileSystemWatcherEngine::startStream()
     if (FSEventStreamStart(stream)) {
         DEBUG() << "Stream started successfully with sinceWhen =" << lastReceivedEvent;
         return true;
-    } else {
+    } else { // again, no way to know what went wrong, so just clean up and fail
         DEBUG() << "Stream failed to start!";
         FSEventStreamInvalidate(stream);
         FSEventStreamRelease(stream);
@@ -525,6 +545,7 @@ bool QFseventsFileSystemWatcherEngine::startStream()
 
 void QFseventsFileSystemWatcherEngine::stopStream(bool isStopped)
 {
+    QMacAutoReleasePool pool;
     if (stream) {
         if (!isStopped)
             FSEventStreamStop(stream);
@@ -554,9 +575,9 @@ QFseventsFileSystemWatcherEngine::InfoByName QFseventsFileSystemWatcherEngine::s
 
 bool QFseventsFileSystemWatcherEngine::derefPath(const QString &watchedPath)
 {
-    PathRefCounts::iterator it = watchedPaths.find(watchedPath);
-    if (it != watchedPaths.end() && --it.value() < 1) {
-        watchedPaths.erase(it);
+    PathRefCounts::iterator it = watchingState.watchedPaths.find(watchedPath);
+    if (it != watchingState.watchedPaths.end() && --it.value() < 1) {
+        watchingState.watchedPaths.erase(it);
         DEBUG("Removing '%s' from watchedPaths.", qPrintable(watchedPath));
         return true;
     }
diff --git a/src/corelib/io/qfilesystemwatcher_fsevents_p.h b/src/corelib/io/qfilesystemwatcher_fsevents_p.h
index b4640afc4e..b99c026fad 100644
--- a/src/corelib/io/qfilesystemwatcher_fsevents_p.h
+++ b/src/corelib/io/qfilesystemwatcher_fsevents_p.h
@@ -81,7 +81,7 @@ Q_SIGNALS:
 private slots:
     void doEmitFileChanged(const QString &path, bool removed);
     void doEmitDirectoryChanged(const QString &path, bool removed);
-    void restartStream();
+    bool restartStream();
 
 private:
     struct Info {
@@ -112,6 +112,19 @@ private:
     typedef QHash<QString, DirInfo> DirsByName;
     typedef QHash<QString, qint64> PathRefCounts;
 
+    struct WatchingState {
+        // These fields go hand-in-hand. FSEvents watches paths, and there is no use in watching
+        // the same path multiple times. So, the "refcount" on a path is the number of watched
+        // files that have the same path, plus the number of directories that have the same path.
+        //
+        // If the stream fails to start after adding files/directories, the watcher will try to
+        // keep watching files/directories that it was already watching. It does that by restoring
+        // the previous WatchingState and restarting the stream.
+        FilesByPath watchedFiles;
+        DirsByName watchedDirectories;
+        PathRefCounts watchedPaths;
+    };
+
     QFseventsFileSystemWatcherEngine(QObject *parent);
     bool startStream();
     void stopStream(bool isStopped = false);
@@ -125,10 +138,8 @@ private:
     QMutex lock;
     dispatch_queue_t queue;
     FSEventStreamRef stream;
-    FilesByPath watchedFiles;
-    DirsByName watchedDirectories;
-    PathRefCounts watchedPaths;
     FSEventStreamEventId lastReceivedEvent;
+    WatchingState watchingState;
 };
 
 QT_END_NAMESPACE