From c8ef4889732011a8d7465bbcbda2574ef8046d76 Mon Sep 17 00:00:00 2001 From: Bill Su Date: Wed, 16 Apr 2025 23:36:33 -0400 Subject: [PATCH] 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 --- include/wx/event.h | 16 ++++++++++++++-- src/common/event.cpp | 42 ++++++++++++++++++++++++++++-------------- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/include/wx/event.h b/include/wx/event.h index 07a161e14d..3913e14ea9 100644 --- a/include/wx/event.h +++ b/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 DynamicEvents; - DynamicEvents* m_dynamicEvents; + struct DynamicEvents + { + wxVector 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 m_dynamicEvents; + // ensure new m_dynamicEvents has same layout as wxWidgets 3.2 m_dynamicEvents + static_assert(sizeof(wxSharedPtr) == sizeof(DynamicEvents*), "wxSharedPtr<> has wrong size"); + static_assert(alignof(wxSharedPtr) == alignof(DynamicEvents*), "wxSharedPtr<> has wrong alignment"); wxList* m_pendingEvents; diff --git a/src/common/event.cpp b/src/common/event.cpp index 6f49f73678..038203adae 100644 --- a/src/common/event.cpp +++ b/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 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; } } }