Fix Signal memory corruption issue

[DanRStevens]

Calling Signal::connect from within a signal callback called by Signal::emit would update the underlying std::vector, which would invalidate iterators. This was causing issues when control returned to Signal::emit and it tried to call subsequent callbacks, which would be done using the invalidated iterator on std::vector memory that had already been freed. Since that memory was already freed, it was subject to re-allocation and corruption.

Reference: OutpostUniverse/OPHD#1269

Valgrind for the win. It produced results much faster than my manual debugging attempts. Valgrind quickly pointed out use of already freed memory, and that this memory was both allocated and freed by the std::vector::push_back method, as used by SignalSource::connect. Note that the same line in SignalSource::connect calling push_back was responsible for both the memory allocation and deletion.

==21611== Invalid read of size 8
==21611== at 0x243366: GetClosureMemPtr (Delegate.h:297)
==21611== by 0x243366: operator() (Delegate.h:410)
==21611== by 0x243366: emit (Signal.h:46)
==21611== by 0x243366: operator() (Signal.h:50)
==21611== by 0x243366: NAS2D::EventHandler::pump() (EventHandler.cpp:681)
==21611== by 0x23F26B: NAS2D::StateManager::update() (StateManager.cpp:87)
==21611== by 0x1C9ED8: main (main.cpp:160)
==21611== Address 0x389d3030 is 112 bytes inside a block of size 384 free'd
==21611== at 0x484BB6F: operator delete(void*, unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==21611== by 0x15498B: deallocate (new_allocator.h:145)
==21611== by 0x15498B: deallocate (allocator.h:199)
==21611== by 0x15498B: deallocate (alloc_traits.h:496)
==21611== by 0x15498B: _M_deallocate (stl_vector.h:354)
==21611== by 0x15498B: void std::vector<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point >, std::allocator<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point > > >::_M_realloc_insert<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point > const&>(__gnu_cxx::__normal_iterator<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point >, std::vector<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point >, std::allocator<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point > > > >, NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point > const&) (vector.tcc:500)
==21611== by 0x1A2D17: push_back (stl_vector.h:1198)
==21611== by 0x1A2D17: connect (SignalSource.h:37)
==21611== by 0x1A2D17: connect<Window, Window> (SignalSource.h:42)
==21611== by 0x1A2D17: Window::Window(std::__cxx11::basic_string<char, std::char_traits, std::allocator >) (Window.cpp:34)
==21611== by 0x16E49D: CheatMenu::CheatMenu() (CheatMenu.cpp:38)
==21611== by 0x21DCE2: MapViewState::MapViewState(MainReportsUiState&, std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&) (MapViewState.cpp:160)
==21611== by 0x1DD8F3: MainMenuState::onFileIoAction(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&, FileIo::FileOperation) (MainMenuState.cpp:147)
==21611== by 0x150D5A: operator() (Delegate.h:410)
==21611== by 0x150D5A: emit (Signal.h:46)
==21611== by 0x150D5A: operator() (Signal.h:50)
==21611== by 0x150D5A: FileIo::onFileIo() (FileIo.cpp:182)
==21611== by 0x1AE73C: operator() (Delegate.h:410)
==21611== by 0x1AE73C: emit (Signal.h:46)
==21611== by 0x1AE73C: operator() (Signal.h:50)
==21611== by 0x1AE73C: Button::onMouseUp(NAS2D::EventHandler::MouseButton, NAS2D::Point) (Button.cpp:155)
==21611== by 0x24334E: operator() (Delegate.h:410)
==21611== by 0x24334E: emit (Signal.h:46)
==21611== by 0x24334E: operator() (Signal.h:50)
==21611== by 0x24334E: NAS2D::EventHandler::pump() (EventHandler.cpp:681)
==21611== by 0x23F26B: NAS2D::StateManager::update() (StateManager.cpp:87)
==21611== by 0x1C9ED8: main (main.cpp:160)
==21611== Block was alloc'd at
==21611== at 0x4849013: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==21611== by 0x154855: allocate (new_allocator.h:127)
==21611== by 0x154855: allocate (allocator.h:185)
==21611== by 0x154855: allocate (alloc_traits.h:464)
==21611== by 0x154855: _M_allocate (stl_vector.h:346)
==21611== by 0x154855: void std::vector<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point >, std::allocator<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point > > >::_M_realloc_insert<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point > const&>(__gnu_cxx::__normal_iterator<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point >, std::vector<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point >, std::allocator<NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point > > > >, NAS2D::DelegateX<void, NAS2D::EventHandler::MouseButton, NAS2D::Point > const&) (vector.tcc:440)
==21611== by 0x1AF8F9: push_back (stl_vector.h:1198)
==21611== by 0x1AF8F9: connect (SignalSource.h:37)
==21611== by 0x1AF8F9: connect<Button, Button> (SignalSource.h:42)
==21611== by 0x1AF8F9: Button::Button(std::__cxx11::basic_string<char, std::char_traits, std::allocator >) (Button.cpp:56)
==21611== by 0x1AFD22: Button::Button(std::__cxx11::basic_string<char, std::char_traits, std::allocator >, NAS2D::DelegateX) (Button.cpp:63)
==21611== by 0x1E0971: MainMenuState::MainMenuState() (MainMenuState.cpp:20)
==21611== by 0x2329B1: SplashState::skipSplash() (SplashState.cpp:79)
==21611== by 0x232A0C: SplashState::onKeyDown(NAS2D::EventHandler::KeyCode, NAS2D::EventHandler::KeyModifier, bool) (SplashState.cpp:163)
==21611== by 0x2430BC: operator() (Delegate.h:410)
==21611== by 0x2430BC: emit (Signal.h:46)
==21611== by 0x2430BC: operator() (Signal.h:50)
==21611== by 0x2430BC: NAS2D::EventHandler::pump() (EventHandler.cpp:660)
==21611== by 0x23F26B: NAS2D::StateManager::update() (StateManager.cpp:87)
==21611== by 0x1C9ED8: main (main.cpp:160)

[DanRStevens]

Copying the std::vector before iteration in Signal::emit is maybe not the most efficient thing to be doing, but it does ensure correctness. As it stands, signal handling isn't exactly very performance sensitive, so the copy overhead kind of doesn't matter. Plus, there's the plan to move to an event bubbling model, which would eliminate the need for a whole lot of the signal handling code in it's current form, so this is all kind of temporary anyway.

[ldicker83]

I had a feeling it was an invalidated iterator somewhere. Was thinking we'd need to do a lazy insert there somewhere. Good work :D

I'm not worried about the copy overhead. Connect/Disconnect calls aren't performance critical and aren't happening repeatedly each frame so a few copies should be negligible in the long run.

[DanRStevens]

Pretty sure I introduced the bug back in PR #1016. Switching to std::vector meant all pointers are invalidated calling erase. The situation is better with std::set::erase, which will only invalidate pointers to the erased item, and not to other items in the collection.

References and iterators to the erased elements are invalidated. Other references and iterators are not affected.

That and the reasons for switching to std::vector are perhaps questionable. I think that was prep work for switching to std::function or adding support for lambdas, but there were other blockers that prevented that from happening. Might still revisit that, though probably after looking into something like event bubbling, which would eliminate most of the need for Signal.