SearchDynamicEventTable(): handle recursion

SearchDynamicEventTable() tries to handle an event handler
that calls Unbind(), but previously failed to account for
SearchDynamicEventTable() being recursive when an event handler
processes a new event.

Use wxSharedPtr<> to ensure life of wxRecursionGuardFlag
extends beyond wxRecursionGuard.  This enables all recursive
call to SearchDynamicEventTable() to prune empty elements from
m_dynamicEvents, not just outermost recursive call.

more detailed explanation of need for wxSharedPtr<>

Co-authored-by: VZ <vz-github@zeitlins.org>

include/wx/event.h

@@ -30,6 +30,8 @@
 #include "wx/typeinfo.h"
 #include "wx/any.h"
 #include "wx/vector.h"
+#include "wx/recguard.h"
+#include "wx/sharedptr.h"
 
 #include "wx/meta/convertible.h"
 #include "wx/meta/removeref.h"
@@ -4066,8 +4068,18 @@ protected:
     wxEvtHandler*       m_nextHandler;
     wxEvtHandler*       m_previousHandler;
 
-    typedef wxVector<wxDynamicEventTableEntry*> DynamicEvents;
-    DynamicEvents* m_dynamicEvents;
+    struct DynamicEvents
+    {
+        wxVector<wxDynamicEventTableEntry*> m_entries;
+        wxRecursionGuardFlag m_flag = 0;
+    };
+    // use wxSharedPtr so that SearchDynamicEventTable() can use another
+    // instance of wxSharedPtr to extend the life of the wxRecursionGuardFlag
+    // to outlive wxRecursionGuard
+    wxSharedPtr<DynamicEvents> m_dynamicEvents;
+    // ensure new m_dynamicEvents has same layout as wxWidgets 3.2 m_dynamicEvents
+    static_assert(sizeof(wxSharedPtr<DynamicEvents>) == sizeof(DynamicEvents*), "wxSharedPtr<> has wrong size");
+    static_assert(alignof(wxSharedPtr<DynamicEvents>) == alignof(DynamicEvents*), "wxSharedPtr<> has wrong alignment");
 
     wxList*             m_pendingEvents;
 

src/common/event.cpp

@@ -1226,7 +1226,6 @@ wxEvtHandler::~wxEvtHandler()
             delete entry->m_callbackUserData;
             delete entry;
         }
-        delete m_dynamicEvents;
     }
 
     // Remove us from the list of the pending events if necessary.
@@ -1786,7 +1785,7 @@ void wxEvtHandler::DoBind(int id,
     // We prefer to push back the entry here and then iterate over the vector
     // in reverse direction in GetNextDynamicEntry() as it's more efficient
     // than inserting the element at the front.
-    m_dynamicEvents->push_back(entry);
+    m_dynamicEvents->m_entries.push_back(entry);
 
     // Make sure we get to know when a sink is destroyed
     wxEvtHandler *eventSink = func->GetEvtHandler();
@@ -1840,7 +1839,7 @@ wxEvtHandler::DoUnbind(int id,
             // Notice that we rely on "cookie" being just the index into the
             // vector, which is not guaranteed by our API, but here we can use
             // this implementation detail.
-            (*m_dynamicEvents)[cookie] = nullptr;
+            m_dynamicEvents->m_entries[cookie] = nullptr;
 
             delete entry;
             return true;
@@ -1856,7 +1855,7 @@ wxEvtHandler::GetFirstDynamicEntry(size_t& cookie) const
         return nullptr;
 
     // The handlers are in LIFO order, so we must start at the end.
-    cookie = m_dynamicEvents->size();
+    cookie = m_dynamicEvents->m_entries.size();
     return GetNextDynamicEntry(cookie);
 }
 
@@ -1869,7 +1868,7 @@ wxEvtHandler::GetNextDynamicEntry(size_t& cookie) const
     {
         // Otherwise return the element at the previous index, skipping any
         // null elements which indicate removed entries.
-        wxDynamicEventTableEntry* const entry = m_dynamicEvents->at(--cookie);
+        wxDynamicEventTableEntry* const entry = m_dynamicEvents->m_entries.at(--cookie);
         if ( entry )
             return entry;
     }
@@ -1882,24 +1881,39 @@ bool wxEvtHandler::SearchDynamicEventTable( wxEvent& event )
     wxCHECK_MSG( m_dynamicEvents, false,
                  wxT("caller should check that we have dynamic events") );
 
+    // Make sure that m_dynamicEvents remains valid until the end of this function,
+    // even if this object itself gets deleted by one of the event handlers called
+    // from here.
+    //
+    // We need to use m_dynamicEvents as we use its m_flag to record whether we're
+    // iterating over its entries and check it at the end, at which time this object
+    // itself might not exist any more.
+    //
+    // Note that we can't just use a local variable instead, as this function can
+    // also be called recursively, if an event handler dispatches another event while
+    // processing this one.
+    wxSharedPtr<DynamicEvents> localCopy(m_dynamicEvents);
     DynamicEvents& dynamicEvents = *m_dynamicEvents;
 
+    wxRecursionGuard guard(dynamicEvents.m_flag);
     bool needToPruneDeleted = false;
 
     // We can't use Get{First,Next}DynamicEntry() here as they hide the deleted
     // but not yet pruned entries from the caller, but here we do want to know
     // about them, so iterate directly. Remember to do it in the reverse order
     // to honour the order of handlers connection.
-    for ( size_t n = dynamicEvents.size(); n; n-- )
+    for ( size_t n = dynamicEvents.m_entries.size(); n; n-- )
     {
-        wxDynamicEventTableEntry* const entry = dynamicEvents[n - 1];
+        wxDynamicEventTableEntry* const entry = dynamicEvents.m_entries[n - 1];
 
         if ( !entry )
         {
             // This entry must have been unbound at some time in the past, so
             // skip it now and really remove it from the vector below, once we
             // finish iterating.
-            needToPruneDeleted = true;
+            // N.B.:  If we are in a nested call, then we can't be done
+            // iterating during this call
+            needToPruneDeleted = !guard.IsInside();
             continue;
         }
 
@@ -1931,14 +1945,14 @@ bool wxEvtHandler::SearchDynamicEventTable( wxEvent& event )
     if ( needToPruneDeleted )
     {
         size_t nNew = 0;
-        for ( size_t n = 0; n != dynamicEvents.size(); n++ )
+        for ( size_t n = 0; n != dynamicEvents.m_entries.size(); n++ )
         {
-            if ( dynamicEvents[n] )
-                dynamicEvents[nNew++] = dynamicEvents[n];
+            if ( dynamicEvents.m_entries[n] )
+                dynamicEvents.m_entries[nNew++] = dynamicEvents.m_entries[n];
         }
 
-        wxASSERT( nNew != dynamicEvents.size() );
-        dynamicEvents.resize(nNew);
+        wxASSERT( nNew != dynamicEvents.m_entries.size() );
+        dynamicEvents.m_entries.resize(nNew);
     }
 
     return false;
@@ -2019,7 +2033,7 @@ void wxEvtHandler::OnSinkDestroyed( wxEvtHandler *sink )
 
             // Just as in DoUnbind(), we use our knowledge of
             // GetNextDynamicEntry() implementation here.
-            (*m_dynamicEvents)[cookie] = nullptr;
+            m_dynamicEvents->m_entries[cookie] = nullptr;
         }
     }
 }