Fix remaining MSW wxFileSystemWatcher threading races

The worker thread checked whether a watch was still used by looking it up
in wxFSWatcherImpl::m_watches, which is modified by the main thread in
Add(), Remove() and RemoveAll() without any locking. And even with the
locking added in wxIOCPService, the watch could still be removed between
checking for its removal and starting a new read for it, leaving a pending
read on a watch nobody uses any more.

Check whether the watch was scheduled for removal and start the new read
while holding the wxIOCPService lock, and complete the removal instead if
it was.

Also register the watch with wxIOCPService before starting the first read
for it, so that the worker thread can't get a completion for a watch it
doesn't know about yet.

include/wx/msw/private/fswatcher.h

@@ -266,6 +266,23 @@ public:
         return false;
     }
 
+    // Forget a watch successfully passed to Add() if no read could be started
+    // for it: as there is no pending I/O using it, it can be dropped at once.
+    void CancelAdd(const wxSharedPtr<wxFSWatchEntryMSW>& watch)
+    {
+        wxCriticalSectionLocker lock(m_critsect);
+
+        const auto it = m_watches.find(watch->GetPath());
+
+        if ( it != m_watches.end() && it->second.get() == watch.get() )
+        {
+            m_watches.erase(it);
+            return;
+        }
+
+        wxFAIL_MSG("No watch to cancel");
+    }
+
     // post completion packet
     bool PostEmptyStatus()
     {
@@ -348,6 +365,7 @@ protected:
     HANDLE m_iocp;
 
     // Protects the watch lists shared by the user thread and IOCP thread.
+    // wxFSWatcherImplMSW::SetUpWatch() holds it while starting a new read.
     wxCriticalSection m_critsect;
 
     // The hash containing all the wxFSWatchEntryMSW objects currently being
@@ -357,6 +375,8 @@ protected:
     // Contains the watches which had been removed but are still pending.
     typedef wxVector< wxSharedPtr<wxFSWatchEntryMSW> > Watches;
     Watches m_removedWatches;
+
+    friend class wxFSWatcherImplMSW;
 };
 
 

src/msw/fswatcher.cpp

@@ -95,12 +95,20 @@ bool wxFSWatcherImplMSW::Init()
 // adds watch to be monitored for file system changes
 bool wxFSWatcherImplMSW::DoAdd(wxSharedPtr<wxFSWatchEntryMSW> watch)
 {
-    // setting up wait for directory changes
-    if (!DoSetUpWatch(*watch))
+    // Associate the handle with the completion port and register the watch
+    // before starting to wait for directory changes: otherwise the worker
+    // thread could get a completion for a watch it doesn't know about yet and
+    // would never start a new read for it.
+    if ( !m_iocp.Add(watch) )
         return false;
 
-    // associating handle with completion port
-    return m_iocp.Add(watch);
+    if ( !DoSetUpWatch(*watch) )
+    {
+        m_iocp.CancelAdd(watch);
+        return false;
+    }
+
+    return true;
 }
 
 bool
@@ -109,18 +117,21 @@ wxFSWatcherImplMSW::DoRemove(wxSharedPtr<wxFSWatchEntryMSW> watch)
     return m_iocp.ScheduleForRemoval(watch);
 }
 
-// TODO ensuring that we have not already set watch for this handle/dir?
+// This is called from the worker thread, so it must not use m_watches, which is
+// only used by the main thread, and asks wxIOCPService instead.
 bool wxFSWatcherImplMSW::SetUpWatch(wxFSWatchEntryMSW& watch)
 {
     wxCHECK_MSG( watch.IsOk(), false, "Invalid watch" );
-    if (m_watches.find(watch.GetPath()) == m_watches.end())
-    {
-        wxLogTrace(wxTRACE_FSWATCHER, "Path '%s' is not watched",
-                   watch.GetPath());
+
+    // Don't let the watch be removed between checking for it and starting a
+    // new read. CompleteRemoval() locks the same recursive critical section.
+    wxCriticalSectionLocker lock(m_iocp.m_critsect);
+
+    if ( m_iocp.CompleteRemoval(&watch) )
         return false;
-    }
 
     wxLogTrace(wxTRACE_FSWATCHER, "Setting up watch for file system changes...");
+
     return DoSetUpWatch(watch);
 }
 
@@ -306,10 +317,9 @@ bool wxIOCPThread::ReadEvents()
     // process events
     ProcessNativeEvents(events);
 
-    if ( m_iocp->CompleteRemoval(watch) )
-        return true;
-
-    // reissue the watch. ignore possible errors, we will return true anyway
+    // reissue the watch unless it was removed while processing the events, in
+    // which case this completes its removal. ignore possible errors, we will
+    // return true anyway
     (void) m_service->SetUpWatch(*watch);
 
     return true;