modedit crashes after deleting graphic zone corner

[Grzegorz Obuch]

1. In Footprint Editor import test.kicad_mod (simplest sample I found)
2. Select graphic zone
3. Right click on any corner
4 Remove Corner => crash

Application: kicad
Version: (5.1.0)-1, 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:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=OFF
    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

[Grzegorz Obuch (grzegorzobuch)]

I found something...

In modules generated from bitmap2component start and end point in fp_poly have the same coordinates.
After manually deleting the last point it works normally..

[Seth Hillbrand (sethh)]

Looks like this was fix committed at some point between 5.1 and master

[Grzegorz Obuch (grzegorzobuch)]

In the last nightly version it behaves the same for me.

Application: kicad
Version: (5.1.0-63-g2447933b5), 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:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=OFF
    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

[Seth Hillbrand (sethh)]

Can you add a screenshot of the crash? Does it grey out the window or is there a new window with an error?

[Wayne Stambaugh (stambaughw)]

I just tried testing the provided footprint by deleting each of the four zone corners and I could not crash the footprint editor. Maybe the crash is due to something else.

[Grzegorz Obuch (grzegorzobuch)]

I attach screenshot captured while it hang, it looks like it want show a mesage..

[Grzegorz Obuch (grzegorzobuch)]

bitmap2component still generates start and stop point of fpoly with the same coordinates - is it a bug?

For me it crash for all footprint generates from bitmap2component. For modules with bigger size sometimes time from hang to crash is longer.

In the attached movie it is all I do from start Footprint Editor

What can I test?

[Grzegorz Obuch (grzegorzobuch)]

Maybe it does not matter here but I noticed 3 different orders in the menu for the corner...

[Grzegorz Obuch (grzegorzobuch)]

Maybe I catch something.
I install KiCad on different location than default.
It work with different language than is selected see attachment
Before this installation i work in English.
Changing language not do anything... it always work in my native language.

I can not repeat that, but once I run Footprint Editor, in any attempt to delete any corner I get message in English:
"Self - intersecting polygons are not allowed" - in small grey box which is empty on "hang.png" attached before.
Any other text in Footprint Editor was then in Polish.

[jean-pierre charras (jp-charras)]

I do not have any crash or hang when trying to delete a corner.

For other things:
* "bitmap2component still generates start and stop point of fpoly with the same coordinates"
Not a bug, but this is a redundant point that should not really create issues.

* When changing the language, some strings are not modified: you have to quit Kicad and rerun it.

* Polygons generated by bitmap2component are sometimes *really* complex, and are not optimized to be edited by hand.
The only easy task is just delete them.

[Michael Kavanagh (michaelkavanagh)]

It happens for me on latest master.

KIGFX::VIEW_GROUP::ViewDraw(int, KIGFX::VIEW*) const view_group.cpp:125
KIGFX::VIEW::draw(KIGFX::VIEW_ITEM*, int, bool) view.cpp:1049
KIGFX::VIEW::drawItem::operator()(KIGFX::VIEW_ITEM*) view.cpp:980
bool RTree<KIGFX::VIEW_ITEM*, int, 2, double, 8, 4>::Search<KIGFX::VIEW::drawItem>(RTree<KIGFX::VIEW_ITEM*, int, 2, double, 8, 4>::Node*, RTree<KIGFX::VIEW_ITEM*, int, 2, double, 8, 4>::Rect*, KIGFX::VIEW::drawItem&, int&) rtree.h:499
int RTree<KIGFX::VIEW_ITEM*, int, 2, double, 8, 4>::Search<KIGFX::VIEW::drawItem>(int const*, int const*, KIGFX::VIEW::drawItem&) rtree.h:154
void KIGFX::VIEW_RTREE::Query<KIGFX::VIEW::drawItem>(BOX2<VECTOR2<int> > const&, KIGFX::VIEW::drawItem&) view_rtree.h:98
KIGFX::VIEW::redrawRect(BOX2<VECTOR2<int> > const&) view.cpp:1019
KIGFX::VIEW::Redraw() view.cpp:1163
EDA_DRAW_PANEL_GAL::onPaint(wxPaintEvent&) draw_panel_gal.cpp:208
wxEvtHandler::SearchDynamicEventTable(wxEvent&) 0x000000010eb819de
wxEvtHandler::ProcessEventLocally(wxEvent&) 0x000000010eb8175b
wxEvtHandler::ProcessEvent(wxEvent&) 0x000000010eb81604
wxEvtHandler::ProcessPendingEvents() 0x000000010eb811be
wxAppConsoleBase::ProcessPendingEvents() 0x000000010ea29e97
TOOL_MANAGER::ProcessEvent(TOOL_EVENT const&) tool_manager.cpp:786
CONTEXT_MENU::onMenuEvent(wxMenuEvent&) context_menu.cpp:393
wxEvtHandler::SearchDynamicEventTable(wxEvent&) 0x000000010eb819de
wxEvtHandler::ProcessEventLocally(wxEvent&) 0x000000010eb8175b
wxEvtHandler::ProcessEvent(wxEvent&) 0x000000010eb81604
wxEvtHandler::SafelyProcessEvent(wxEvent&) 0x000000010eb81a7c
wxMenuBase::SendEvent(int, int) 0x000000010e59eac6
wxMenu::HandleCommandProcess(wxMenuItem*, wxWindow*) 0x000000010e42bffb
-[wxNSMenuItem clickedAction:] 0x000000010e4c10e3
-[NSApplication(NSResponder) sendAction:to:from:] 0x00007fff4348ce80
-[NSMenuItem _corePerformAction] 0x00007fff434e906f
-[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:] 0x00007fff434e8de0
-[NSMenu performActionForItemAtIndex:] 0x00007fff4354cd93
-[NSMenu _internalPerformActionForItemAtIndex:] 0x00007fff4354cd00
-[NSCarbonMenuImpl _carbonCommandProcessEvent:handlerCallRef:] 0x00007fff4354cb32
NSSLMMenuEventHandler 0x00007fff434b7602
DispatchEventToHandlers(EventTargetRec*, OpaqueEventRef*, HandlerCallRec*) 0x00007fff44dfea6e
SendEventToEventTargetInternal(OpaqueEventRef*, OpaqueEventTargetRef*, HandlerCallRec*) 0x00007fff44dfde1f
SendEventToEventTarget 0x00007fff44e1afc6
SendHICommandEvent(unsigned int, HICommand const*, unsigned int, unsigned int, unsigned char, void const*, OpaqueEventTargetRef*, OpaqueEventTargetRef*, OpaqueEventRef**) 0x00007fff44e6d202
SendMenuCommandWithContextAndModifiers 0x00007fff44e9569e
SendMenuItemSelectedEvent 0x00007fff44e9564d
FinishMenuSelection(SelectionData*, MenuResult*, MenuResult*) 0x00007fff44e95535
PopUpMenuSelectCore(MenuData*, Point, double, Point, unsigned short, unsigned int, unsigned int, Rect const*, unsigned short, unsigned int, Rect const*, Rect const*, __CFDictionary const*, __CFString const*, OpaqueMenuRef**, unsigned short*) 0x00007fff44fa3ff1
_HandlePopUpMenuSelection8(OpaqueMenuRef*, OpaqueEventRef*, unsigned int, Point, unsigned short, unsigned int, unsi...

[Seth Hillbrand (sethh)]

@Michael- I can't recreate the issue so can you test the attached patch to see if it resolves the crash?

[Michael Kavanagh (michaelkavanagh)]

@Seth - it doesn't seem to help.

[Grzegorz Obuch (grzegorzobuch)]

Another way to reproduce it:
1. Create new footprint.
2. Draw simple polygon like in attachment.
3. Try to delete corner (marked with circle) that normally should generate only error :"Self - intersecting polygons are not allowed"
4 hang and crash...

This always happens for me if I do it right after starting Kicad.

Today I work some time with Footprint Editor and after that time trying the same ends normally, but that was only one time it works OK.

[Michael Kavanagh (michaelkavanagh)]

The problem seems to be related to the two vertices (bottom right in the example provided) at the same location.

It doesn't crash if the duplicate bottom right vertex is removed first before trying to remove the other vertices.

Removing either the top or bottom left vertices first causes validatePolygon( *polygon ) to return false at point_editor.cpp:1149.

It can be reproduced by:
1) creating a new footprint
2) drawing a polygon (>4 vertices)
3) moving a vertex on top a neighbouring vertex (a "Self-intersecting polygons are not allowed message" pops up but doesn't prevent you from doing it)
4) right-click and remove any other vertex.

[Wayne Stambaugh (stambaughw)]

I can reproduce this every time with the latest steps. Here is a full back trace from gdb:

Thread 1 received signal SIGSEGV, Segmentation fault.
0x0000000080795252 in KIGFX::VIEW_GROUP::ViewDraw (this=0x1ecb8668,
    aLayer=133, aView=0x143352f0)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/common/view/view_group.cpp:125
125 item->ViewGetLayers( item_layers, item_layers_count );
(gdb) bt
#0 0x0000000080795252 in KIGFX::VIEW_GROUP::ViewDraw (this=0x1ecb8668,
    aLayer=133, aView=0x143352f0)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/common/view/view_group.cpp:125
#1 0x000000008079315b in KIGFX::VIEW::draw (this=0x143352f0, aItem=
    0x1ecb8668, aLayer=133, aImmediate=false)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/common/view/view.cpp:1049
#2 0x0000000080933cf2 in KIGFX::VIEW::drawItem::operator() (this=0x849ccf0,
    aItem=0x1ecb8668)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/common/view/view.cpp:980
#3 0x000000008093bf6d in RTree<KIGFX::VIEW_ITEM*, int, 2, double, 8, 4>::Search<KIGFX::VIEW::drawItem> (this=0x143836d0, a_node=0x14383720,
    a_rect=0x849cc40, a_visitor=..., a_foundCount=@0x849cc3c: 0)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/include/geometry/rtree.h:499
#4 0x000000008093c03b in RTree<KIGFX::VIEW_ITEM*, int, 2, double, 8, 4>::Search<KIGFX::VIEW::drawItem> (this=0x143836d0, a_min=0x849cca8, a_max=0x849cca0,
    a_visitor=...)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/include/geometry/rtree.h:154
#5 0x000000008092f274 in KIGFX::VIEW_RTREE::Query<KIGFX::VIEW::drawItem> (
    this=0x143836d0, aBounds=..., aVisitor=...)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/include/view/view_rtree.h:98
#6 0x0000000080792fef in KIGFX::VIEW::redrawRect (this=0x143352f0, aRect=...)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/common/view/view.cpp:1019
#7 0x00000000807936a2 in KIGFX::VIEW::Redraw (this=0x143352f0)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/common/view/view.cpp:1163
#8 0x00000000807d8134 in EDA_DRAW_PANEL_GAL::onPaint (this=0x13152640)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/common/draw_panel_gal.cpp:208
#9 0x000000006a4c2f24 in ?? ()
   from C:\msys64\mingw64\bin\wxbase30u_gcc_custom.dll
#10 0x000000006a62cd42 in ?? ()
   from C:\msys64\mingw64\bin\wxbase30u_gcc_custom.dll
#11 0x000000006a62d14d in ?? ()
   from C:\msys64\mingw64\bin\wxbase30u_gcc_custom.dll
#12 0x000000006a62d1e2 in ?? ()
   from C:\msys64\mingw64\bin\wxbase30u_gcc_custom.dll
#13 0x000000006a62d28d in ?? ()
   from C:\msys64\mingw64\bin\wxbase30u_gcc_custom.dll
#14 0x000000006a62d321 in ?? ()
   from C:\msys64\mingw64\bin\wxbase30u_gcc_custom.dll
#15 0x000000006a62de09 in ?? ()
   from C:\msys64\mingw64\bin\wxbase30u_gcc_custom.dll
#16 0x000000006a4c513f in ?? ()
   from C:\msys64\mingw64\bin\wxbase30u_gcc_custom.dll
#17 0x00000000807a4d91 in TOOL_MANAGER::ProcessEvent (this=0x1ec94650,
    aEvent=...)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/common/tool/tool_manager.cpp:786
#18 0x000000008079d31d in CONTEXT_MENU::onMenuEvent (this=0x1edfa110,
    aEvent=...)
    at C:/msys64/home/wstambaugh/src/kicad-trunk/common/tool/context_menu.cpp:393
#19 0x000000006a4c2f24 in ...

[Seth Hillbrand (sethh)]

Wow, I am unable to recreate this on Linux with any of the procedures above. This smells like a memory issue but my first guess (in #13) apparently didn't help.

The redraw is clearly referencing an invalid pointer. The fact that it only gets this invalid pointer after failing to remove a point indicates that the points vector isn't being handled correctly in the undo code that gets called after failure.

Valgrind isn't giving me any pointers here.

@Wayne- It would be helpful if you could get the type of item the viewgroup is trying to draw in your crash. I suspect it is the overlay but I could be offbase

Application: pcbnew
Version: (5.1.0-108-g428e7a900b-dirty), release build
Libraries:
    wxWidgets 3.0.4
    libcurl/7.64.0 OpenSSL/1.1.1b zlib/1.2.11 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.5) libssh2/1.8.0 nghttp2/1.36.0 librtmp/2.3
Platform: Linux 4.19.0-4-amd64 x86_64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.4 (wchar_t,wx containers,compatible with 2.8) GTK+ 3.24
    Boost: 1.67.0
    OpenCASCADE Community Edition: 6.9.1
    Curl: 7.64.0
    Compiler: GCC 8.3.0 with C++ ABI 1013

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

[Seth Hillbrand (sethh)]

Oh, I just got one by crossing the segments before deleting a point.

[KiCad Janitor (kicad-janitor)]

Fixed in revision 940aef77afa88fe360baf5204dd0b688dd573409
https://git.launchpad.net/kicad/patch/?id=940aef77afa88fe360baf5204dd0b688dd573409

[Seth Hillbrand (sethh)]

The crash I got is resolved by using the patch from #13 so there must be multiple issues.

@Wayne I pushed #13 as it fixes a definite issue. Can you test your procedure with the latest master and see if you still get a crash?

[Seth Hillbrand (sethh)]

I spoke too soon. The issue was that our Revert() function invalidates the MODULE element lists, so pointers from VIEW get trashed when we cause an action to be undone. We handle this elsewhere by deselecting the item after a call to Revert() and so I've implemented that here as well.

There are a couple cleanup items that we should tackle related to the item.

1) Once we start generating UIDs for elements, we should start using them to restore the selected items. This is long term but within v6's scope.

2) We should probably cause invalid polygons to be simplified when created/loaded. This would create some additional points in the invalid polygons but would keep our handling routines correct.

[Michael Kavanagh (michaelkavanagh)]

Nice one Seth.

From a user perspective, it would be better to stop bitmap2component from creating the duplicate vertices. If a user wanted to edit such a polygon by deleting any of the other vertices, they'd probably be confused as to why it won't let them. Especially as it's not obvious that vertices are stacked.

[Seth Hillbrand (sethh)]

@Michael - Agreed. That said, bitmap2component shouldn't generate degenerate polygons now. I tried with a number of simple cases and couldn't get it to output a polygon like the attachment.

[Grzegorz Obuch (grzegorzobuch)]

@Seth
Somethings is changed in bitmap2component.
The last nightly version in which redundant points are generated:

Application: kicad
Version: (5.1.0-73-g8985badc6), 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:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=OFF
    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

[jean-pierre charras (jp-charras)]

Duplicate points are not a problem (besides I removed the last duplicate point).
bitmap2component generates valid polygons.
However, these polygons can have holes.
In this case, these holes are connected by 2 overlapping segments to the main outline.
In this a very usual way to generate a polygon "with holes", and this is a valid polygon.
Zones use widely these polygons without any issue.

But these polygons are not easy (read: cannot) to edit by hand:
they need (like zones) to be converted to a set of polygons: main outline and holes.

Beside our cross-intersecting polygon detection is overzealous:
it detects (incorrectly) such polygons as self-intersecting.

The crash in an other story.

[KiCad Janitor (kicad-janitor)]

Fixed in revision 7ee12d674ee80d09a4ad46e773873aeac4537f27
https://git.launchpad.net/kicad/patch/?id=7ee12d674ee80d09a4ad46e773873aeac4537f27