Crash when Disconnect()ing event handler without specifying function

[cwalther]

After this sequence

source->Connect(wxEVT_SOMETHING, wxSomethingEventHandler(MySink::OnSomething), NULL, sink);
source->Disconnect(wxID_ANY, wxEVT_SOMETHING));
delete source;

destruction of sink crashes because wxEventConnectionRef::OnObjectDestroy tries to call a method on the already destroyed source.

The cause is that calling Disconnect() without specifying the function (letting it figure it out on its own, taking the first match for wxEVT_SOMETHING) fails to call DecRef() on the wxEventConnectionRef connecting sink and source, so that it erroneously still exists at the time sink is destroyed.

I realize this is a somewhat exotic situation, as the problem can be easily worked around by passing the function to Disconnect() too, and Connect/Disconnect is supposed to be replaced by Bind/Unbind now, but as far as I understand the documentation, this should work. I just ran into this problem while porting an application to wxWidgets 2.9 that worked fine in 2.6.

To reproduce: apply the attached disconnectcrash-unittest.patch to the events/evthandler unit test and run test EvtHandlerTestCase. Expected result: tests should pass. Actual result: EvtHandlerTestCase::DisconnectWildcard() crashes on line evthandler.cpp:265 (at least when run on a C library that overwrites freed memory, such as in a debug build on Windows (MSVC10) - it didn’t crash for me on Linux, but even there stepping through with a debugger clearly shows that a method is being called on a destroyed object).

It seems to me this could be fixed by the attached disconnectcrash.patch, but I am completely unfamiliar with wxEventConnectionRef etc., so I’d appreciate a thorough review. In addition to fixing the above crash, it also makes sure that wxEventConnectionRef::DecRef() is not called when the loop over m_dynamicEvents doesn’t find anything to disconnect (Disconnect() will return false). Otherwise, such unmatching Disconnect() calls could cause OnSinkDestroyed not to be called. I added a test case for that too (EvtHandlerTestCase::AutoDisconnect).

[cwalther]

unit tests that currently fail/crash

[cwalther]

proposed fix

[vadz]

Thanks for the patches, I don't have time to look at them right now but we definitely should do something about this and I'll try to do it before 2.9.5.

If anybody else can review this, it would be very welcome too, of course!

[vadz]

The trouble is that I still get bug reports from valgrind even after applying the patch:

% valgrind ./test EvtHandler
==11042== Memcheck, a memory error detector
==11042== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==11042== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==11042== Command: ./test EvtHandler
==11042==
Test program for wxWidgets non-GUI features
build: 2.9.5 (wchar_t,compiler with C++ ABI 1002,wx containers,compatible with 2.8)
...==11042== Invalid read of size 8
==11042==    at 0x577C962: wxListBase::GetFirst() const (list.h:551)
==11042==    by 0x577CA3B: wxObjectList::GetFirst() const (list.h:1194)
==11042==    by 0x58A1BD5: wxEvtHandler::OnSinkDestroyed(wxEvtHandler*) (event.cpp:1799)
==11042==    by 0x58A2BFB: wxEventConnectionRef::OnObjectDestroy() (event.h:3675)
==11042==    by 0x58A227C: wxTrackable::~wxTrackable() (tracker.h:84)
==11042==    by 0x589FC8F: wxEvtHandler::~wxEvtHandler() (event.cpp:1152)
==11042==    by 0x4F7C54: (anonymous namespace)::MyHandler::~MyHandler() (evthandler.cpp:101)
==11042==    by 0x4F8795: EvtHandlerTestCase::DisconnectWildcard() (evthandler.cpp:262)
==11042==    by 0x501753: CppUnit::TestCaller<EvtHandlerTestCase>::runTest() (TestCaller.h:166)
==11042==    by 0x4E54406: CppUnit::TestCaseMethodFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4277EF: wxUnitTestProtector::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) (test.cpp:202)
==11042==    by 0x4E50278: CppUnit::ProtectorChain::ProtectFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==  Address 0x6d24328 is 24 bytes inside a block of size 48 free'd
==11042==    at 0x4C23E0F: operator delete(void*) (vg_replace_malloc.c:387)
==11042==    by 0x50A8A2C: wxList::~wxList() (list.h:1208)
==11042==    by 0x589FEA2: wxEvtHandler::~wxEvtHandler() (event.cpp:1140)
==11042==    by 0x4F8746: EvtHandlerTestCase::DisconnectWildcard() (evthandler.cpp:262)
==11042==    by 0x501753: CppUnit::TestCaller<EvtHandlerTestCase>::runTest() (TestCaller.h:166)
==11042==    by 0x4E54406: CppUnit::TestCaseMethodFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4277EF: wxUnitTestProtector::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) (test.cpp:202)
==11042==    by 0x4E50278: CppUnit::ProtectorChain::ProtectFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4E467D3: CppUnit::DefaultProtector::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4E50278: CppUnit::ProtectorChain::ProtectFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4E4FFBB: CppUnit::ProtectorChain::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4E5BD9F: CppUnit::TestResult::protect(CppUnit::Functor const&, CppUnit::Test*, std::string const&) (in /usr/lib/libcppunit-1.12.so.1.0.0)

I didn't yet have time to look at this though, if you have any idea about why am I still getting this, it would be more than welcome.

TIA!

[vadz]

I have no idea what happened but I can't reproduce the problem under valgrind any more. I also ran it under Intel Inspector and it didn't find anything wrong neither so it looks like I had done something wrong the last time, so I'll just commit your patches now.

Thanks again for fixing the bug and providing the unit tests for it!

[VZ]

(In [72943]) Fix crashes after using "wildcard" wxEvtHandler::Disconnect().

When not specifying the function to disconnect, the associated event sink was
destroyed too early resulting in crashes later. Fix this and add unit tests
verifying that things work as expected and at least don't crash.

Closes #14563.

[cwalther]

Thanks! As you may have guessed from my inactivity, I haven’t gotten around to looking into your valgrind error. If it was just a mistake on your part, all the better. My guess would be that you used a build of the library that didn’t have the patch applied.