pcbnew crashes when you try to draw a line more than a few segments

[Tim Trzepacz]

Pcbnew will frequently crash when I try to use the line tool to draw a line, especially if that is the first thing I do.
Often it happens within the first few segments I draw, especially if I am using Eco2.User, Edge.Cuts, or F.Cu layers.
Usually it happens immediately, especially if I draw in Eco2.User first thing, even if I am editing an empty file. If it doesn't happen in the first few lines, it might go for a long time without crashing, but still has a chance to crash.

About KiCAD: (I downloaded the latest daily.)
Application: KiCad
Version: (5.1.0-998-gf550ecaf1), release build
Libraries:
    wxWidgets 3.0.4
    libcurl/7.61.1 OpenSSL/1.1.1 (WinSSL) zlib/1.2.11 brotli/1.0.6 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.5) nghttp2/1.34.0
Platform: Windows 8 (build 9200), 64-bit edition, 64 bit, Little endian, wxMSW
Build Info:
    wxWidgets: 3.0.4 (wchar_t,wx containers,compatible with 2.8)
    Boost: 1.68.0
    OpenCASCADE Community Edition: 6.9.1
    Curl: 7.61.1
    Compiler: GCC 8.2.0 with C++ ABI 1013

Build settings:
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_PYTHON3=OFF
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_WXPYTHON_PHOENIX=OFF
    KICAD_SCRIPTING_ACTION_MENU=ON
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=ON
    KICAD_USE_OCC=OFF
    KICAD_SPICE=ON

My computer: Microsoft Windows 10 Home
Version: 10.0.17763 Build 17763
System Manufacturer: Sager/Clevo
System Model: P7xxDM(-G)
Processor: Intel Core i7-6700K CPU @ 4GHz 4001MHz 4 Cores, 8 Logical Processors
Bios: American Megatrends Inc. 1.05.03LS1, 9/23/2015

[Michael Kavanagh (michaelkavanagh)]

Please copy the full version information from About KiCad -> Copy Version Info.

[Tim Trzepacz (softegg)]

Ok, I copied the full version info. Can somebody look at the bug now?

[Seth Hillbrand (sethh)]

Hi Tim-

Thank you for the additional information. I suspect that this bug has already been fixed by Jeff. Please try tomorrow's nightly build to see if it still happens.

If you experience it again, please copy the same version information again (I know, repetitive, right? :) into a new comment. All the extra data really help us to fix things and are greatly appreciated.

Also, if you experience the crash again, it would be helpful to get the information from your Event log.

[Jeff Young (jeyjey)]

No, the bug is still there. It looks like memory corruption: I put in some defensive code but it just moves around....

[Seth Hillbrand (sethh)]

Jeff, did you ever get a backtrace?

[Jeff Young (jeyjey)]

The last couple I got died trying to look up the TOOL_STATE in the m_toolIdIndex map:

std::__1::__tree_node_base<void*>*& std::__1::__tree<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, std::__1::__map_value_compare<int, std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, std::__1::less<int>, true>, std::__1::allocator<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*> > >::__find_equal<int>(std::__1::__tree_end_node<std::__1::__tree_node_base<void*>*>*&, int const&) __functional_base:55
std::__1::__tree_node_base<void*>*& std::__1::__tree<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, std::__1::__map_value_compare<int, std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, std::__1::less<int>, true>, std::__1::allocator<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*> > >::__find_equal<int>(std::__1::__tree_end_node<std::__1::__tree_node_base<void*>*>*&, int const&) map:479
std::__1::__tree_node_base<void*>*& std::__1::__tree<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, std::__1::__map_value_compare<int, std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, std::__1::less<int>, true>, std::__1::allocator<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*> > >::__find_equal<int>(std::__1::__tree_end_node<std::__1::__tree_node_base<void*>*>*&, int const&) __tree:2007
std::__1::pair<std::__1::__tree_iterator<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, std::__1::__tree_node<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, void*>*, long>, bool> std::__1::__tree<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, std::__1::__map_value_compare<int, std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*>, std::__1::less<int>, true>, std::__1::allocator<std::__1::__value_type<int, TOOL_MANAGER::TOOL_STATE*> > >::__emplace_unique_key_args<int, std::__1::piecewise_construct_t const&, std::__1::tuple<int const&>, std::__1::tuple<> >(int const&, std::__1::piecewise_construct_t const&, std::__1::tuple<int const&>&&, std::__1::tuple<>&&) __tree:2131
std::__1::map<int, TOOL_MANAGER::TOOL_STATE*, std::__1::less<int>, std::__1::allocator<std::__1::pair<int const, TOOL_MANAGER::TOOL_STATE*> > >::operator[](int const&) map:1319
TOOL_MANAGER::dispatchInternal(TOOL_EVENT const&) tool_manager.cpp:522
TOOL_MANAGER::processEvent(TOOL_EVENT const&) tool_manager.cpp:920
TOOL_MANAGER::ProcessEvent(TOOL_EVENT const&) tool_manager.cpp:766
TOOL_DISPATCHER::handleMouseButton(wxEvent&, int, bool) tool_dispatcher.cpp:242
TOOL_DISPATCHER::DispatchWxEvent(wxEvent&) tool_dispatcher.cpp:354
EDA_DRAW_PANEL_GAL::OnEvent(wxEvent&) draw_panel_gal.cpp:442
wxAppConsoleBase::HandleEvent(wxEvtHandler*, void (wxEvtHandler::*)(wxEvent&), wxEvent&) const appbase.cpp:611
wxAppConsoleBase::CallEventHandler(wxEvtHandler*, wxEventFunctor&, wxEvent&) const appbase.cpp:623
wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&) event.cpp:1395
wxEvtHandler::SearchDynamicEventTable(wxEvent&) event.cpp:1754
wxEvtHandler::TryHereOnly(wxEvent&) event.cpp:1588
wxEvtHandler::TryBeforeAndHere(wxEvent&) event.h:3680
wxEvtHandler::ProcessEventLocally(wxEvent&) event.cpp:1525
wxEvtHandler::ProcessEv...

[Jeff Young (jeyjey)]

The last frame on the stack it's running the std::map's comparator. LLDB shows me:

(lldb) frame var
(const std::__1::less<int> *) this = 0x00007f9eaf078160
(const int &) __x = 0x900007f9eb36f7cd (&__x = <read memory from 0x900007f9eb36f7cd failed (0 of 4 bytes read)>)
(const int &) __y = 0x00007f9eaf0c2190 (&__y = 7)

Note that it looks like __x's pointer has been shifted one byte to the right?

[Seth Hillbrand (sethh)]

I see what is happening.

We are re-arranging the tool stack in runTool but iterating over the stack in dispatchInternal. The iterators are invalidated by the re-arrangement.

[Jeff Young (jeyjey)]

And it's exacerbated by the fact that the line drawing tool is unique in that it Activates itself again for each segment (so it's reshuffling the tool stack more often).

Still, it's hard to imagine this didn't bite us sooner?

[KiCad Janitor (kicad-janitor)]

Fixed in revision 552815d48617c08a6ec29f22c7ecdf1906988e11
https://git.launchpad.net/kicad/patch/?id=552815d48617c08a6ec29f22c7ecdf1906988e11

[Seth Hillbrand (sethh)]

It's certainly an unusual case. Other tools can't invalidate the stack because this is not a threaded operation and we don't execute a wait() anywhere. So the tool can only do this to itself. That is it gets activated, then erases itself from the stack and re-inserts itself, invalidating the iterator in the process without changing the stack.

[Jeff Young (jeyjey)]

@Seth, couldn't we still get into trouble if one of the tools on the stack activates or deactivates itself and then does a PassEvent() so that we keep iterating through the stack?

[Jeff Young (jeyjey)]

Well, this at least allowed me to dive deeper into the other hack. The issue was m_passEvent being effectively a global.

If a tool called something like clearSelection while processing a MOUSE_CLICK, the SELECTION_TOOL will pass the clearSelection COMMAND_EVENT because it handles it as a transition, not as an event. Because m_passEvent is effectively global, the tool manager would then interpret that as passing the MOUSE_CLICK and we'd end up processing it by multiple tools.

I've moved m_passEvent to the TOOL_EVENT.

[Seth Hillbrand (sethh)]

Nice catch Jeff! I think that fixed a number of other issues I was seeing.