Mac: make FSEvents-based QFileSystemWatcherEngine more robust.

- Record the last event ID with every callback. This event ID is passed
to FSEventStreamCreate when restarting the stream, so the watcher will
receive all events that occurred since invalidating the previous stream.

- Never start with kFSEventStreamEventIdSinceNow, because this will
generate a (bogus) soft-assert in FSEventStreamFlushSync in CarbonCore
when no event occurred since stream creation. The last globally
generated event ID is used instead to simulate the "now".

- Do not dispose and recreate the stream in the callback, but use a
queued signal-slot connection to schedule this on thread that owns the
watcher.

Change-Id: I02f5a845d9e27f9853ed97925ab9c7a5bc0dede1
Reviewed-by: Eike Ziller <eike.ziller@digia.com>
Reviewed-by: Morten Johan Sørvig <morten.sorvig@digia.com>

2 files changed, 105 insertions, 42 deletions

diff --git a/src/corelib/io/qfilesystemwatcher_fsevents.mm b/src/corelib/io/qfilesystemwatcher_fsevents.mm
index cb6ddd913e..bfcae2a3a2 100644
--- a/src/corelib/io/qfilesystemwatcher_fsevents.mm
+++ b/src/corelib/io/qfilesystemwatcher_fsevents.mm
@@ -55,7 +55,7 @@
 #include <qfileinfo.h>
 #include <qvarlengtharray.h>
 
-//#define FSEVENT_DEBUG
+#undef FSEVENT_DEBUG
 #ifdef FSEVENT_DEBUG
 #  define DEBUG if (true) qDebug
 #else
@@ -76,14 +76,16 @@ static void callBackFunction(ConstFSEventStreamRef streamRef,
     engine->processEvent(streamRef, numEvents, paths, eventFlags, eventIds);
 }
 
-void QFseventsFileSystemWatcherEngine::checkDir(DirsByName::iterator &it)
+bool QFseventsFileSystemWatcherEngine::checkDir(DirsByName::iterator &it)
 {
+    bool needsRestart = false;
+
     QT_STATBUF st;
     const QString &name = it.key();
     Info &info = it->dirInfo;
     const int res = QT_STAT(QFile::encodeName(name), &st);
     if (res == -1) {
-        derefPath(info.watchedPath);
+        needsRestart |= derefPath(info.watchedPath);
         emit emitDirectoryChanged(info.origPath, true);
         it = watchedDirectories.erase(it);
     } else if (st.st_ctimespec != info.ctime || st.st_mode != info.mode) {
@@ -127,26 +129,34 @@ void QFseventsFileSystemWatcherEngine::checkDir(DirsByName::iterator &it)
         if (dirChanged)
             emit emitDirectoryChanged(info.origPath, false);
     }
+
+    return needsRestart;
 }
 
-void QFseventsFileSystemWatcherEngine::rescanDirs(const QString &path)
+bool QFseventsFileSystemWatcherEngine::rescanDirs(const QString &path)
 {
+    bool needsRestart = false;
+
     for (DirsByName::iterator it = watchedDirectories.begin(); it != watchedDirectories.end(); ) {
         if (it.key().startsWith(path))
-            checkDir(it);
+            needsRestart |= checkDir(it);
         else
              ++it;
     }
+
+    return needsRestart;
 }
 
-void QFseventsFileSystemWatcherEngine::rescanFiles(InfoByName &filesInPath)
+bool QFseventsFileSystemWatcherEngine::rescanFiles(InfoByName &filesInPath)
 {
+    bool needsRestart = false;
+
     for (InfoByName::iterator it = filesInPath.begin(); it != filesInPath.end(); ) {
         QT_STATBUF st;
         QString name = it.key();
         const int res = QT_STAT(QFile::encodeName(name), &st);
         if (res == -1) {
-            derefPath(it->watchedPath);
+            needsRestart |= derefPath(it->watchedPath);
             emit emitFileChanged(it.value().origPath, true);
             it = filesInPath.erase(it);
             continue;
@@ -158,13 +168,17 @@ void QFseventsFileSystemWatcherEngine::rescanFiles(InfoByName &filesInPath)
 
         ++it;
     }
+
+    return needsRestart;
 }
 
-void QFseventsFileSystemWatcherEngine::rescanFiles(const QString &path)
+bool QFseventsFileSystemWatcherEngine::rescanFiles(const QString &path)
 {
+    bool needsRestart = false;
+
     for (FilesByPath::iterator i = watchedFiles.begin(); i != watchedFiles.end(); ) {
         if (i.key().startsWith(path)) {
-            rescanFiles(i.value());
+            needsRestart |= rescanFiles(i.value());
             if (i.value().isEmpty()) {
                 i = watchedFiles.erase(i);
                 continue;
@@ -173,6 +187,8 @@ void QFseventsFileSystemWatcherEngine::rescanFiles(const QString &path)
 
         ++i;
     }
+
+    return needsRestart;
 }
 
 void QFseventsFileSystemWatcherEngine::processEvent(ConstFSEventStreamRef streamRef,
@@ -184,11 +200,20 @@ void QFseventsFileSystemWatcherEngine::processEvent(ConstFSEventStreamRef stream
 #if defined(Q_OS_OSX) && MAC_OS_X_VERSION_MIN_REQUIRED > MAC_OS_X_VERSION_10_6
     Q_UNUSED(streamRef);
 
+    bool needsRestart = false;
+
     QMutexLocker locker(&lock);
 
     for (size_t i = 0; i < numEvents; ++i) {
         FSEventStreamEventFlags eFlags = eventFlags[i];
         DEBUG("Change %llu in %s, flags %x", eventIds[i], eventPaths[i], (unsigned int)eFlags);
+
+        if (eFlags & kFSEventStreamEventFlagEventIdsWrapped) {
+            DEBUG("\tthe event ids wrapped");
+            lastReceivedEvent = 0;
+        }
+        lastReceivedEvent = qMax(lastReceivedEvent, eventIds[i]);
+
         QString path = QFile::decodeName(eventPaths[i]);
         if (path.endsWith(QDir::separator()))
             path = path.mid(0, path.size() - 1);
@@ -199,38 +224,36 @@ void QFseventsFileSystemWatcherEngine::processEvent(ConstFSEventStreamRef stream
                 DEBUG("\t\t... user dropped.");
             if (eFlags & kFSEventStreamEventFlagKernelDropped)
                 DEBUG("\t\t... kernel dropped.");
-            rescanDirs(path);
-            rescanFiles(path);
+            needsRestart |= rescanDirs(path);
+            needsRestart |= rescanFiles(path);
             continue;
         }
 
-        if (eFlags & kFSEventStreamEventFlagEventIdsWrapped) {
-            DEBUG("\tthe event ids wrapped");
-            // TODO: verify if we need to do something
-        }
-
         if (eFlags & kFSEventStreamEventFlagRootChanged) {
             // re-check everything:
             DirsByName::iterator dirIt = watchedDirectories.find(path);
             if (dirIt != watchedDirectories.end())
-                checkDir(dirIt);
-            rescanFiles(path);
+                needsRestart |= checkDir(dirIt);
+            needsRestart |= rescanFiles(path);
             continue;
         }
 
         if ((eFlags & kFSEventStreamEventFlagItemIsDir) && (eFlags & kFSEventStreamEventFlagItemRemoved))
-            rescanDirs(path);
+            needsRestart |= rescanDirs(path);
 
         // check watched directories:
         DirsByName::iterator dirIt = watchedDirectories.find(path);
         if (dirIt != watchedDirectories.end())
-            checkDir(dirIt);
+            needsRestart |= checkDir(dirIt);
 
         // check watched files:
         FilesByPath::iterator pIt = watchedFiles.find(path);
         if (pIt != watchedFiles.end())
-            rescanFiles(pIt.value());
+            needsRestart |= rescanFiles(pIt.value());
     }
+
+    if (needsRestart)
+        emit scheduleStreamRestart();
 #else
     // This is a work-around for moc: when we put the version check at the top of the header file,
     // moc will still see the Q_OBJECT macro and generate a meta-object when compiling for 10.6,
@@ -248,14 +271,23 @@ void QFseventsFileSystemWatcherEngine::processEvent(ConstFSEventStreamRef stream
 
 void QFseventsFileSystemWatcherEngine::doEmitFileChanged(const QString path, bool removed)
 {
+    DEBUG() << "emitting fileChanged for" << path << "with removed =" << removed;
     emit fileChanged(path, removed);
 }
 
 void QFseventsFileSystemWatcherEngine::doEmitDirectoryChanged(const QString path, bool removed)
 {
+    DEBUG() << "emitting directoryChanged for" << path << "with removed =" << removed;
     emit directoryChanged(path, removed);
 }
 
+void QFseventsFileSystemWatcherEngine::restartStream()
+{
+    QMutexLocker locker(&lock);
+    stopStream();
+    startStream();
+}
+
 QFseventsFileSystemWatcherEngine *QFseventsFileSystemWatcherEngine::create(QObject *parent)
 {
     return new QFseventsFileSystemWatcherEngine(parent);
@@ -264,6 +296,7 @@ QFseventsFileSystemWatcherEngine *QFseventsFileSystemWatcherEngine::create(QObje
 QFseventsFileSystemWatcherEngine::QFseventsFileSystemWatcherEngine(QObject *parent)
     : QFileSystemWatcherEngine(parent)
     , stream(0)
+    , lastReceivedEvent(kFSEventStreamEventIdSinceNow)
 {
 
     // We cannot use signal-to-signal queued connections, because the
@@ -272,6 +305,8 @@ QFseventsFileSystemWatcherEngine::QFseventsFileSystemWatcherEngine(QObject *pare
             this, SLOT(doEmitDirectoryChanged(const QString, bool)), Qt::QueuedConnection);
     connect(this, SIGNAL(emitFileChanged(const QString, bool)),
             this, SLOT(doEmitFileChanged(const QString, bool)), Qt::QueuedConnection);
+    connect(this, SIGNAL(scheduleStreamRestart()),
+            this, SLOT(restartStream()), Qt::QueuedConnection);
 
     queue = dispatch_queue_create("org.qt-project.QFseventsFileSystemWatcherEngine", NULL);
 }
@@ -284,7 +319,7 @@ QFseventsFileSystemWatcherEngine::~QFseventsFileSystemWatcherEngine()
     // The assumption with the locking strategy is that this class cannot and will not be subclassed!
     QMutexLocker locker(&lock);
 
-    stopStream();
+    stopStream(true);
     dispatch_release(queue);
 }
 
@@ -292,12 +327,14 @@ QStringList QFseventsFileSystemWatcherEngine::addPaths(const QStringList &paths,
                                                        QStringList *files,
                                                        QStringList *directories)
 {
-    if (stream)
+    if (stream) {
+        DEBUG("Flushing, last id is %llu", FSEventStreamGetLatestEventId(stream));
         FSEventStreamFlushSync(stream);
+    }
 
     QMutexLocker locker(&lock);
 
-    bool newWatchPathsFound = false;
+    bool needsRestart = false;
 
     QStringList p = paths;
     QMutableListIterator<QString> it(p);
@@ -343,8 +380,9 @@ QStringList QFseventsFileSystemWatcherEngine::addPaths(const QStringList &paths,
 
         PathRefCounts::iterator it = watchedPaths.find(watchedPath);
         if (it == watchedPaths.end()) {
-            newWatchPathsFound = true;
+            needsRestart = true;
             watchedPaths.insert(watchedPath, 1);
+            DEBUG("Adding '%s' to watchedPaths", qPrintable(watchedPath));
         } else {
             ++it.value();
         }
@@ -355,12 +393,14 @@ QStringList QFseventsFileSystemWatcherEngine::addPaths(const QStringList &paths,
             dirInfo.dirInfo = info;
             dirInfo.entries = scanForDirEntries(realPath);
             watchedDirectories.insert(realPath, dirInfo);
+            DEBUG("-- Also adding '%s' to watchedDirectories", qPrintable(realPath));
         } else {
             watchedFiles[parentPath].insert(realPath, info);
+            DEBUG("-- Also adding '%s' to watchedFiles", qPrintable(realPath));
         }
     }
 
-    if (newWatchPathsFound) {
+    if (needsRestart) {
         stopStream();
         if (!startStream())
             p = paths;
@@ -375,6 +415,8 @@ QStringList QFseventsFileSystemWatcherEngine::removePaths(const QStringList &pat
 {
     QMutexLocker locker(&lock);
 
+    bool needsRestart = false;
+
     QStringList p = paths;
     QMutableListIterator<QString> it(p);
     while (it.hasNext()) {
@@ -389,10 +431,11 @@ QStringList QFseventsFileSystemWatcherEngine::removePaths(const QStringList &pat
         if (fi.isDir()) {
             DirsByName::iterator dirIt = watchedDirectories.find(realPath);
             if (dirIt != watchedDirectories.end()) {
-                derefPath(dirIt->dirInfo.watchedPath);
+                needsRestart |= derefPath(dirIt->dirInfo.watchedPath);
                 watchedDirectories.erase(dirIt);
                 directories->removeAll(origPath);
                 it.remove();
+                DEBUG("Removed directory '%s'", qPrintable(realPath));
             }
         } else {
             QFileInfo fi(realPath);
@@ -402,22 +445,32 @@ QStringList QFseventsFileSystemWatcherEngine::removePaths(const QStringList &pat
                 InfoByName &filesInDir = pIt.value();
                 InfoByName::iterator fIt = filesInDir.find(realPath);
                 if (fIt != filesInDir.end()) {
-                    derefPath(fIt->watchedPath);
+                    needsRestart |= derefPath(fIt->watchedPath);
                     filesInDir.erase(fIt);
                     if (filesInDir.isEmpty())
                         watchedFiles.erase(pIt);
                     files->removeAll(origPath);
                     it.remove();
+                    DEBUG("Removed file '%s'", qPrintable(realPath));
                 }
             }
         }
     }
 
+    locker.unlock();
+
+    if (needsRestart)
+        restartStream();
+
     return p;
 }
 
 bool QFseventsFileSystemWatcherEngine::startStream()
 {
+    Q_ASSERT(stream == 0);
+    if (stream) // This shouldn't happen, but let's be nice and handle it.
+        stopStream();
+
     if (watchedPaths.isEmpty())
         return false;
 
@@ -437,11 +490,15 @@ bool QFseventsFileSystemWatcherEngine::startStream()
     const CFAbsoluteTime latency = .5; // in seconds
     FSEventStreamCreateFlags flags = kFSEventStreamCreateFlagWatchRoot;
 
+    // Never start with kFSEventStreamEventIdSinceNow, because this will generate an invalid
+    // soft-assert in FSEventStreamFlushSync in CarbonCore when no event occurred.
+    if (lastReceivedEvent == kFSEventStreamEventIdSinceNow)
+        lastReceivedEvent = FSEventsGetCurrentEventId();
     stream = FSEventStreamCreate(NULL,
                                  &callBackFunction,
                                  &callBackInfo,
                                  reinterpret_cast<CFArrayRef>(pathsToWatch),
-                                 kFSEventStreamEventIdSinceNow,
+                                 lastReceivedEvent,
                                  latency,
                                  flags);
 
@@ -453,10 +510,13 @@ bool QFseventsFileSystemWatcherEngine::startStream()
     FSEventStreamSetDispatchQueue(stream, queue);
 
     if (FSEventStreamStart(stream)) {
-        DEBUG() << "Stream started successfully.";
+        DEBUG() << "Stream started successfully with sinceWhen =" << lastReceivedEvent;
         return true;
     } else {
         DEBUG() << "Stream failed to start!";
+        FSEventStreamInvalidate(stream);
+        FSEventStreamRelease(stream);
+        stream = 0;
         return false;
     }
 }
@@ -469,7 +529,7 @@ void QFseventsFileSystemWatcherEngine::stopStream(bool isStopped)
         FSEventStreamInvalidate(stream);
         FSEventStreamRelease(stream);
         stream = 0;
-        DEBUG() << "Stream stopped.";
+        DEBUG() << "Stream stopped. Last event ID:" << lastReceivedEvent;
     }
 }
 
@@ -490,16 +550,16 @@ QFseventsFileSystemWatcherEngine::InfoByName QFseventsFileSystemWatcherEngine::s
     return entries;
 }
 
-void QFseventsFileSystemWatcherEngine::derefPath(const QString &watchedPath)
+bool QFseventsFileSystemWatcherEngine::derefPath(const QString &watchedPath)
 {
     PathRefCounts::iterator it = watchedPaths.find(watchedPath);
-    if (it == watchedPaths.end())
-        return;
-    if (--it.value() < 1) {
+    if (it != watchedPaths.end() && --it.value() < 1) {
         watchedPaths.erase(it);
-        stopStream();
-        startStream();
+        DEBUG("Removing '%s' from watchedPaths.", qPrintable(watchedPath));
+        return true;
     }
+
+    return false;
 }
 
 #endif //QT_NO_FILESYSTEMWATCHER
diff --git a/src/corelib/io/qfilesystemwatcher_fsevents_p.h b/src/corelib/io/qfilesystemwatcher_fsevents_p.h
index c899c556c8..2de2bf4f18 100644
--- a/src/corelib/io/qfilesystemwatcher_fsevents_p.h
+++ b/src/corelib/io/qfilesystemwatcher_fsevents_p.h
@@ -84,10 +84,12 @@ public:
 Q_SIGNALS:
     void emitFileChanged(const QString path, bool removed);
     void emitDirectoryChanged(const QString path, bool removed);
+    void scheduleStreamRestart();
 
 private slots:
     void doEmitFileChanged(const QString path, bool removed);
     void doEmitDirectoryChanged(const QString path, bool removed);
+    void restartStream();
 
 private:
     struct Info {
@@ -122,11 +124,11 @@ private:
     bool startStream();
     void stopStream(bool isStopped = false);
     InfoByName scanForDirEntries(const QString &path);
-    void derefPath(const QString &watchedPath);
-    void checkDir(DirsByName::iterator &it);
-    void rescanDirs(const QString &path);
-    void rescanFiles(InfoByName &filesInPath);
-    void rescanFiles(const QString &path);
+    bool derefPath(const QString &watchedPath);
+    bool checkDir(DirsByName::iterator &it);
+    bool rescanDirs(const QString &path);
+    bool rescanFiles(InfoByName &filesInPath);
+    bool rescanFiles(const QString &path);
 
     QMutex lock;
     dispatch_queue_t queue;
@@ -134,6 +136,7 @@ private:
     FilesByPath watchedFiles;
     DirsByName watchedDirectories;
     PathRefCounts watchedPaths;
+    FSEventStreamEventId lastReceivedEvent;
 };
 
 QT_END_NAMESPACE