Fix crash when unbinding event handlers from other handlers

Calling Unbind() on another handler from the currently executing handler which
had been bound after (and hence executed before) the handler being unbound
resulted in a crash previously as the iterators used in the loop over all
dynamic event handlers became invalid.

Fix this by storing the dynamic event table entries in a vector instead of a
list (which is also more memory and speed efficient anyhow) and null the
deleted entries instead of removing them to avoid invalidating the iterators
and only really remove them once we finish iterating.

Closes #17229.

include/wx/event.h

@@ -3587,8 +3587,6 @@ public:
                             userData);
     }
 
-    wxList* GetDynamicEventTable() const { return m_dynamicEvents ; }
-
     // User data can be associated with each wxEvtHandler
     void SetClientObject( wxClientData *data ) { DoSetClientObject(data); }
     wxClientData *GetClientObject() const { return DoGetClientObject(); }
@@ -3610,6 +3608,14 @@ public:
                                         wxEvtHandler *handler,
                                         wxEvent& event);
 
+    // Allow iterating over all connected dynamic event handlers: you must pass
+    // the same "cookie" to GetFirst() and GetNext() and call them until null
+    // is returned.
+    //
+    // These functions are for internal use only.
+    wxDynamicEventTableEntry* GetFirstDynamicEntry(size_t& cookie) const;
+    wxDynamicEventTableEntry* GetNextDynamicEntry(size_t& cookie) const;
+
     virtual bool SearchEventTable(wxEventTable& table, wxEvent& event);
     bool SearchDynamicEventTable( wxEvent& event );
 
@@ -3680,7 +3686,10 @@ protected:
 
     wxEvtHandler*       m_nextHandler;
     wxEvtHandler*       m_previousHandler;
-    wxList*             m_dynamicEvents;
+
+    typedef wxVector<wxDynamicEventTableEntry*> DynamicEvents;
+    DynamicEvents* m_dynamicEvents;
+
     wxList*             m_pendingEvents;
 
 #if wxUSE_THREADS

src/common/event.cpp

@@ -1122,13 +1122,11 @@ wxEvtHandler::~wxEvtHandler()
 
     if (m_dynamicEvents)
     {
-        for ( wxList::iterator it = m_dynamicEvents->begin(),
+        size_t cookie;
-                               end = m_dynamicEvents->end();
+        for ( wxDynamicEventTableEntry* entry = GetFirstDynamicEntry(cookie);
-              it != end;
+              entry;
-              ++it )
+              entry = GetNextDynamicEntry(cookie) )
         {
-            wxDynamicEventTableEntry *entry = (wxDynamicEventTableEntry*)*it;
-
             // Remove ourselves from sink destructor notifications
             // (this has usually been done, in wxTrackable destructor)
             wxEvtHandler *eventSink = entry->m_fn->GetEvtHandler();
@@ -1696,10 +1694,12 @@ void wxEvtHandler::DoBind(int id,
         new wxDynamicEventTableEntry(eventType, id, lastId, func, userData);
 
     if (!m_dynamicEvents)
-        m_dynamicEvents = new wxList;
+        m_dynamicEvents = new DynamicEvents;
 
-    // Insert at the front of the list so most recent additions are found first
+    // We prefer to push back the entry here and then iterate over the vector
-    m_dynamicEvents->Insert( (wxObject*) entry );
+    // in reverse direction in GetNextDynamicEntry() as it's more efficient
+    // than inserting the element at the front.
+    m_dynamicEvents->push_back(entry);
 
     // Make sure we get to know when a sink is destroyed
     wxEvtHandler *eventSink = func->GetEvtHandler();
@@ -1723,11 +1723,11 @@ wxEvtHandler::DoUnbind(int id,
     if (!m_dynamicEvents)
         return false;
 
-    wxList::compatibility_iterator node = m_dynamicEvents->GetFirst();
+    size_t cookie;
-    while (node)
+    for ( wxDynamicEventTableEntry* entry = GetFirstDynamicEntry(cookie);
+          entry;
+          entry = GetNextDynamicEntry(cookie) )
     {
-        wxDynamicEventTableEntry *entry = (wxDynamicEventTableEntry*)node->GetData();
-
         if ((entry->m_id == id) &&
             ((entry->m_lastId == lastId) || (lastId == wxID_ANY)) &&
             ((entry->m_eventType == eventType) || (eventType == wxEVT_NULL)) &&
@@ -1744,28 +1744,78 @@ wxEvtHandler::DoUnbind(int id,
             }
 
             delete entry->m_callbackUserData;
-            m_dynamicEvents->Erase( node );
+
+            // We can't delete the entry from the vector if we're currently
+            // iterating over it. As we don't know whether we're or not, just
+            // null it for now and we will really erase it when we do finish
+            // iterating over it the next time.
+            //
+            // 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] = NULL;
+
             delete entry;
             return true;
         }
-        node = node->GetNext();
     }
     return false;
 }
 
+wxDynamicEventTableEntry*
+wxEvtHandler::GetFirstDynamicEntry(size_t& cookie) const
+{
+    if ( !m_dynamicEvents )
+        return NULL;
+
+    // The handlers are in LIFO order, so we must start at the end.
+    cookie = m_dynamicEvents->size();
+    return GetNextDynamicEntry(cookie);
+}
+
+wxDynamicEventTableEntry*
+wxEvtHandler::GetNextDynamicEntry(size_t& cookie) const
+{
+    // On entry here cookie is one greater than the index of the entry to
+    // return, so if it is 0, it means that there are no more entries.
+    while ( cookie )
+    {
+        // Otherwise return the element at the previous index, skipping any
+        // null elements which indicate removed entries.
+        wxDynamicEventTableEntry* const entry = m_dynamicEvents->at(--cookie);
+        if ( entry )
+            return entry;
+    }
+
+    return NULL;
+}
+
 bool wxEvtHandler::SearchDynamicEventTable( wxEvent& event )
 {
     wxCHECK_MSG( m_dynamicEvents, false,
                  wxT("caller should check that we have dynamic events") );
 
-    wxList::compatibility_iterator node = m_dynamicEvents->GetFirst();
+    DynamicEvents& dynamicEvents = *m_dynamicEvents;
-    while (node)
-    {
-        wxDynamicEventTableEntry *entry = (wxDynamicEventTableEntry*)node->GetData();
 
-        // get next node before (maybe) calling the event handler as it could
+    bool processed = false;
-        // call Disconnect() invalidating the current node
+    bool needToPruneDeleted = false;
-        node = node->GetNext();
+
+    // 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-- )
+    {
+        wxDynamicEventTableEntry* const entry = dynamicEvents[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;
+            continue;
+        }
 
         if ( event.GetEventType() == entry->m_eventType )
         {
@@ -1773,11 +1823,27 @@ bool wxEvtHandler::SearchDynamicEventTable( wxEvent& event )
             if ( !handler )
                handler = this;
             if ( ProcessEventIfMatchesId(*entry, handler, event) )
-                return true;
+            {
+                processed = true;
+                break;
+            }
         }
     }
 
-    return false;
+    if ( needToPruneDeleted )
+    {
+        size_t nNew = 0;
+        for ( size_t n = 0; n != dynamicEvents.size(); n++ )
+        {
+            if ( dynamicEvents[n] )
+                dynamicEvents[nNew++] = dynamicEvents[n];
+        }
+
+        wxASSERT( nNew != dynamicEvents.size() );
+        dynamicEvents.resize(nNew);
+    }
+
+    return processed;
 }
 
 void wxEvtHandler::DoSetClientObject( wxClientData *data )
@@ -1844,19 +1910,20 @@ void wxEvtHandler::OnSinkDestroyed( wxEvtHandler *sink )
     wxASSERT(m_dynamicEvents);
 
     // remove all connections with this sink
-    wxList::compatibility_iterator node = m_dynamicEvents->GetFirst(), node_nxt;
+    size_t cookie;
-    while (node)
+    for ( wxDynamicEventTableEntry* entry = GetFirstDynamicEntry(cookie);
+          entry;
+          entry = GetNextDynamicEntry(cookie) )
     {
-        wxDynamicEventTableEntry *entry = (wxDynamicEventTableEntry*)node->GetData();
-        node_nxt = node->GetNext();
-
         if ( entry->m_fn->GetEvtHandler() == sink )
         {
             delete entry->m_callbackUserData;
-            m_dynamicEvents->Erase( node );
             delete entry;
+
+            // Just as in DoUnbind(), we use our knowledge of
+            // GetNextDynamicEntry() implementation here.
+            (*m_dynamicEvents)[cookie] = NULL;
         }
-        node = node_nxt;
     }
 }
 

src/common/xtistrm.cpp

@@ -119,34 +119,30 @@ void wxObjectWriter::FindConnectEntry(const wxEvtHandler * evSource,
                                 const wxObject* &sink, 
                                 const wxHandlerInfo *&handler)
 {
-    wxList *dynamicEvents = evSource->GetDynamicEventTable();
+    size_t cookie;
-
+    for ( wxDynamicEventTableEntry* entry = evSource->GetFirstDynamicEntry(cookie);
-    if ( dynamicEvents )
+          entry;
+          entry = evSource->GetNextDynamicEntry(cookie) )
     {
-        for ( wxList::const_iterator node = dynamicEvents->begin(); node != dynamicEvents->end(); ++node )
+        // find the match
+        if ( entry->m_fn &&
+            (dti->GetEventType() == entry->m_eventType) &&
+            (entry->m_id == -1 ) &&
+            (entry->m_fn->GetEvtHandler() != NULL ) )
         {
-            wxDynamicEventTableEntry *entry = (wxDynamicEventTableEntry*)(*node);
+            sink = entry->m_fn->GetEvtHandler();
-
+            const wxClassInfo* sinkClassInfo = sink->GetClassInfo();
-            // find the match
+            const wxHandlerInfo* sinkHandler = sinkClassInfo->GetFirstHandler();
-            if ( entry->m_fn &&
+            while ( sinkHandler )
-                (dti->GetEventType() == entry->m_eventType) &&
-                (entry->m_id == -1 ) &&
-                (entry->m_fn->GetEvtHandler() != NULL ) )
             {
-                sink = entry->m_fn->GetEvtHandler();
+                if ( sinkHandler->GetEventFunction() == entry->m_fn->GetEvtMethod() )
-                const wxClassInfo* sinkClassInfo = sink->GetClassInfo();
-                const wxHandlerInfo* sinkHandler = sinkClassInfo->GetFirstHandler();
-                while ( sinkHandler )
                 {
-                    if ( sinkHandler->GetEventFunction() == entry->m_fn->GetEvtMethod() )
+                    handler = sinkHandler;
-                    {
+                    break;
-                        handler = sinkHandler;
-                        break;
-                    }
-                    sinkHandler = sinkHandler->GetNext();
                 }
-                break;
+                sinkHandler = sinkHandler->GetNext();
             }
+            break;
         }
     }
 }

tests/events/evthandler.cpp

@@ -164,6 +164,7 @@ private:
         CPPUNIT_TEST( BindFunctionUsingBaseEvent );
         CPPUNIT_TEST( BindNonHandler );
         CPPUNIT_TEST( InvalidBind );
+        CPPUNIT_TEST( UnbindFromHandler );
     CPPUNIT_TEST_SUITE_END();
 
     void BuiltinConnect();
@@ -178,6 +179,7 @@ private:
     void BindFunctionUsingBaseEvent();
     void BindNonHandler();
     void InvalidBind();
+    void UnbindFromHandler();
 
 
     // these member variables exceptionally don't use "m_" prefix because
@@ -459,3 +461,48 @@ void EvtHandlerTestCase::InvalidBind()
     myHandler.Bind(MyEventType, &MyHandler::OnMyEvent, &mySink);
 #endif
 }
+
+void EvtHandlerTestCase::UnbindFromHandler()
+{
+    struct Handler1
+    {
+        void OnDontCall(MyEvent&)
+        {
+            // Although this handler is bound, the second one below is bound
+            // later and so will be called first and will disconnect this one
+            // before it has a chance to be called.
+            CPPUNIT_FAIL("shouldn't be called");
+        }
+    } h1;
+
+    handler.Bind(MyEventType, &Handler1::OnDontCall, &h1);
+
+    class Handler2
+    {
+    public:
+        Handler2(MyHandler& handler, Handler1& h1)
+        {
+            m_handler = &handler;
+            m_h1 = &h1;
+        }
+
+        void OnUnbind(MyEvent& e)
+        {
+            m_handler->Unbind(MyEventType, &Handler1::OnDontCall, m_h1);
+
+            // Check that the now disconnected first handler is not executed.
+            e.Skip();
+        }
+
+    private:
+        // Use pointers instead of references just to avoid warnings about the
+        // class being non-copyable even though these pointers don't change and
+        // are non-NULL.
+        MyHandler* m_handler;
+        Handler1* m_h1;
+    } h2(handler, h1);
+
+    handler.Bind(MyEventType, &Handler2::OnUnbind, &h2);
+
+    handler.ProcessEvent(e);
+}