Fix wxFileSystemWatcher::Remove() in wxMSW.

Removing the path watched by wxFileSystemWatcher didn't do anything in wxMSW
implementation so we still continued getting events for the changes to this
path even after calling Remove().

Fix this by really implementing Remove() properly. Also add a unit test
checking that we don't get any events after calling Remove().

See #12847.

git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@67691 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

include/wx/msw/private/fswatcher.h

@@ -104,7 +104,6 @@ private:
     wxDECLARE_NO_COPY_CLASS(wxFSWatchEntryMSW);
 };
 
-
 // ============================================================================
 // wxFSWatcherImplMSW helper classes implementations
 // ============================================================================
@@ -156,6 +155,48 @@ public:
         return m_watches.insert(val).second;
     }
 
+    // Removes a watch we're currently using. Notice that this doesn't happen
+    // immediately, CompleteRemoval() must be called later when it's really
+    // safe to delete the watch, i.e. after completion of the IO operation
+    // using it.
+    bool ScheduleForRemoval(const wxSharedPtr<wxFSWatchEntryMSW>& watch)
+    {
+        wxCHECK_MSG( m_iocp != INVALID_HANDLE_VALUE, false, "IOCP not init" );
+        wxCHECK_MSG( watch->IsOk(), false, "Invalid watch" );
+
+        const wxString path = watch->GetPath();
+        wxFSWatchEntries::iterator it = m_watches.find(path);
+        wxCHECK_MSG( it != m_watches.end(), false,
+                     "Can't remove a watch we don't use" );
+
+        // We can't just delete the watch here as we can have pending events
+        // for it and if we destroyed it now, we could get a dangling (or,
+        // worse, reused to point to another object) pointer in ReadEvents() so
+        // just remember that this one should be removed when CompleteRemoval()
+        // is called later.
+        m_removedWatches.insert(wxFSWatchEntries::value_type(path, watch));
+        m_watches.erase(it);
+
+        return true;
+    }
+
+    // Really remove the watch previously passed to ScheduleForRemoval().
+    //
+    // It's ok to call this for a watch that hadn't been removed before, in
+    // this case we'll just return false and do nothing.
+    bool CompleteRemoval(wxFSWatchEntryMSW* watch)
+    {
+        wxFSWatchEntries::iterator it = m_removedWatches.find(watch->GetPath());
+        if ( it == m_removedWatches.end() )
+            return false;
+
+        // Removing the object from the map will result in deleting the watch
+        // itself as it's not referenced from anywhere else now.
+        m_removedWatches.erase(it);
+
+        return true;
+    }
+
     // post completion packet
     bool PostEmptyStatus()
     {
@@ -203,7 +244,13 @@ protected:
     }
 
     HANDLE m_iocp;
+
+    // The hash containing all the wxFSWatchEntryMSW objects currently being
+    // watched keyed by their paths.
     wxFSWatchEntries m_watches;
+
+    // Contains the watches which had been removed but are still pending.
+    wxFSWatchEntries m_removedWatches;
 };
 
 

src/msw/fswatcher.cpp

@@ -108,9 +108,9 @@ bool wxFSWatcherImplMSW::DoAdd(wxSharedPtr<wxFSWatchEntryMSW> watch)
 }
 
 bool
-wxFSWatcherImplMSW::DoRemove(wxSharedPtr<wxFSWatchEntryMSW> WXUNUSED(watch))
+wxFSWatcherImplMSW::DoRemove(wxSharedPtr<wxFSWatchEntryMSW> watch)
 {
-    return true;
+    return m_iocp.ScheduleForRemoval(watch);
 }
 
 // TODO ensuring that we have not already set watch for this handle/dir?
@@ -216,6 +216,11 @@ bool wxIOCPThread::ReadEvents()
     wxLogTrace( wxTRACE_FSWATCHER, "[iocp] Read entry: path='%s'",
                 watch->GetPath());
 
+    // First check if we're still interested in this watch, we could have
+    // removed it in the meanwhile.
+    if ( m_iocp->CompleteRemoval(watch) )
+        return true;
+
     // extract events from buffer info our vector container
     wxVector<wxEventProcessingData> events;
     const char* memory = static_cast<const char*>(watch->GetBuffer());

tests/fswatcher/fswatchertest.cpp

@@ -412,6 +412,8 @@ private:
         CPPUNIT_TEST( TestEventAccess );
 #endif // __WXMSW__
 #endif // !wxHAS_KQUEUE
+
+        CPPUNIT_TEST( TestNoEventsAfterRemove );
     CPPUNIT_TEST_SUITE_END();
 
     void TestEventCreate();
@@ -420,6 +422,8 @@ private:
     void TestEventModify();
     void TestEventAccess();
 
+    void TestNoEventsAfterRemove();
+
     DECLARE_NO_COPY_CLASS(FileSystemWatcherTestCase)
 };
 
@@ -605,3 +609,46 @@ void FileSystemWatcherTestCase::TestEventAccess()
     EventTester tester;
     tester.Run();
 }
+
+void FileSystemWatcherTestCase::TestNoEventsAfterRemove()
+{
+    class EventTester : public EventHandler,
+                        public wxTimer
+    {
+    public:
+        EventTester()
+        {
+            // We need to use an inactivity timer as we never get any file
+            // system events in this test, so we consider that the test is
+            // finished when this 1s timeout expires instead of, as usual,
+            // stopping after getting the file system events.
+            Start(1000, true);
+        }
+
+        virtual void GenerateEvent()
+        {
+            m_watcher->Remove(EventGenerator::GetWatchDir());
+            CPPUNIT_ASSERT(eg.CreateFile());
+        }
+
+        virtual void CheckResult()
+        {
+            CPPUNIT_ASSERT( m_events.empty() );
+        }
+
+        virtual wxFileSystemWatcherEvent ExpectedEvent()
+        {
+            CPPUNIT_FAIL( "Shouldn't be called" );
+
+            return wxFileSystemWatcherEvent(wxFSW_EVENT_ERROR);
+        }
+
+        virtual void Notify()
+        {
+            SendIdle();
+        }
+    };
+
+    EventTester tester;
+    tester.Run();
+}