symbol editor crashes when editing symbol name

[Martin Sivak]

Loading an existing symbol and changing its name crashes symbol editor with class_libentry.cpp(272): assert "m_aliases.size()" failed in GetName(): no aliases defined for symbol

I am trying to rename IRLML6402 from the attached library to IRLML5203TRPBF by using E key (edit) on top of the symbol name. The app crashes upon confirming that change.

Version: (2018-01-02 revision 44d4559f8)-master, debug build
Libraries:
    wxWidgets 3.0.2
    libcurl/7.53.1 NSS/3.33 zlib/1.2.11 libidn2/2.0.4 libpsl/0.18.0 (+libidn2/2.0.3) libssh2/1.8.0 nghttp2/1.21.1
Platform: Linux 4.14.8-200.fc26.x86_64 x86_64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.2 (wchar_t,wx containers,compatible with 2.8) GTK+ 2.24
    Boost: 1.63.0
    Curl: 7.53.1
    Compiler: GCC 7.2.1 with C++ ABI 1011

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

[Oivind Toien (otoien)]

I tested with the provided library and the described procedure on the latest Windows nightly under Win7 64 bit, and cannot reproduce. Perhaps a Linux specific bug?

Application: kicad
Version: (2018-01-02 revision 44d4559f8)-makepkg, release build
Libraries:
    wxWidgets 3.0.3
    libcurl/7.54.1 OpenSSL/1.0.2l zlib/1.2.11 libssh2/1.8.0 nghttp2/1.23.1 librtmp/2.3
Platform: Windows 7 (build 7601, Service Pack 1), 64-bit edition, 64 bit, Little endian, wxMSW
Build Info:
    wxWidgets: 3.0.3 (wchar_t,wx containers,compatible with 2.8)
    Boost: 1.60.0
    Curl: 7.54.1
    Compiler: GCC 7.1.0 with C++ ABI 1011

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

[Seth Hillbrand (sethh)]

Probably as it is a memory error.

The attached patch corrects the crashy behavior by (I think) placing the correct amount of memory on the heap to avoid dereferencing a bad location when checking the LIB_ID. However, it doesn't fix the underlying memory leak in the component tree.

[Seth Hillbrand (sethh)]

Attached is a patch to fully address this error.

The issue arose because we were trying to store the tree state including the currently selected part while renaming. The node was freed after renaming, so dereferencing the LIB_ID caused the segfault.

The solution in this patch is to select the parent library if the selected part is being renamed. Then we re-select the newly renamed part. If the selected part is not being renamed, this patch should do nothing.

[Martin Sivak (mars-montik)]

The patch does not fix the issue for me. It still crashes.

Moreover I tried renaming a component that has aliases defined (I did that musing text editor) and I got slightly different traceback. IRLML6402K was the new name I tried to use.

ASSERT INFO:
/home/msivak/Work/kicad/eeschema/libeditframe.cpp(607): assert "alias != __null" failed in OnUpdateViewDoc(): Alias "IRLML6402" not found in symbol "IRLML6402K".

BACKTRACE:
[1] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&)
[2] wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*)
[3] wxEvtHandler::TryHereOnly(wxEvent&)
[4] wxEvtHandler::DoTryChain(wxEvent&)
[5] wxEvtHandler::ProcessEvent(wxEvent&)
[6] wxWindowBase::TryAfter(wxEvent&)
[7] wxAuiToolBar::DoIdleUpdate()
[8] wxAuiToolBar::UpdateWindowUI(long)
[9] wxWindowBase::SendIdleEvents(wxIdleEvent&)
[10] wxWindowBase::SendIdleEvents(wxIdleEvent&)
[11] wxFrame::SendIdleEvents(wxIdleEvent&)
[12] wxAppBase::ProcessIdle()
[13] wxApp::DoIdle()
[14] g_main_context_dispatch
[15] g_main_loop_run
[16] gtk_dialog_run
[17] wxMessageDialog::ShowModal()
[18] wxMessageBox(wxString const&, wxString const&, long, wxWindow*, int, int)
[19] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&)
[20] wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*)
[21] wxEvtHandler::TryHereOnly(wxEvent&)
[22] wxEvtHandler::DoTryChain(wxEvent&)
[23] wxEvtHandler::ProcessEvent(wxEvent&)
[24] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&)
[25] wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*)
[26] wxEvtHandler::TryHereOnly(wxEvent&)
[27] wxEvtHandler::ProcessEventLocally(wxEvent&)
[28] wxEvtHandler::ProcessEvent(wxEvent&)
[29] wxScrollHelperEvtHandler::ProcessEvent(wxEvent&)
[30] wxEvtHandler::SafelyProcessEvent(wxEvent&)
[31] wxWindow::GTKDoInsertTextFromIM(char const*)
[32] g_closure_invoke
[33] g_signal_emit_valist
[34] g_signal_emit_by_name
[35] g_closure_invoke
[36] g_signal_emit_valist
[37] g_signal_emit_by_name
[38] g_closure_invoke
[39] g_signal_emit_valist
[40] g_signal_emit
[41] gtk_window_propagate_key_event
[42] g_closure_invoke
[43] g_signal_emit_valist
[44] g_signal_emit
[45] gtk_propagate_event
[46] gtk_main_do_event
[47] g_main_context_dispatch
[48] g_main_loop_run
[49] gtk_main
[50] wxGUIEventLoop::DoRun()
[51] wxEventLoopBase::Run()
[52] wxAppConsoleBase::MainLoop()
[53] APP_KICAD::OnRun() /home/msivak/Work/kicad/kicad/kicad.cpp:233
[54] wxEntry(int&, wchar_t**)
[55] main /home/msivak/Work/kicad/kicad/kicad.cpp:265
[56] __libc_start_main
[57] _start

[Wayne Stambaugh (stambaughw)]

@Seth, is this patch on top of the previous patch or does it completely
replace the previous patch?

On 1/3/2018 3:02 PM, Seth Hillbrand wrote:
> Attached is a patch to fully address this error.
>
> The issue arose because we were trying to store the tree state including
> the currently selected part while renaming. The node was freed after
> renaming, so dereferencing the LIB_ID caused the segfault.
>
> The solution in this patch is to select the parent library if the
> selected part is being renamed. Then we re-select the newly renamed
> part. If the selected part is not being renamed, this patch should do
> nothing.
>
> ** Patch added: "0001-Library-Editor-Keep-cmptree-synchronized-while-updat.patch"
> https://bugs.launchpad.net/kicad/+bug/1740952/+attachment/5030641/+files/0001-Library-Editor-Keep-cmptree-synchronized-while-updat.patch
>

[Wayne Stambaugh (stambaughw)]

Which patch? There are now two patches linked to this bug report.

On 1/3/2018 3:56 PM, Martin Sivak wrote:
> The patch does not fix the issue for me. It still crashes.
>
> Moreover I tried renaming a component that has aliases defined (I did
> that musing text editor) and I got slightly different traceback.
> IRLML6402K was the new name I tried to use.
>
> ASSERT INFO:
> /home/msivak/Work/kicad/eeschema/libeditframe.cpp(607): assert "alias != __null" failed in OnUpdateViewDoc(): Alias "IRLML6402" not found in symbol "IRLML6402K".
>
> BACKTRACE:
> [1] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&)
> [2] wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*)
> [3] wxEvtHandler::TryHereOnly(wxEvent&)
> [4] wxEvtHandler::DoTryChain(wxEvent&)
> [5] wxEvtHandler::ProcessEvent(wxEvent&)
> [6] wxWindowBase::TryAfter(wxEvent&)
> [7] wxAuiToolBar::DoIdleUpdate()
> [8] wxAuiToolBar::UpdateWindowUI(long)
> [9] wxWindowBase::SendIdleEvents(wxIdleEvent&)
> [10] wxWindowBase::SendIdleEvents(wxIdleEvent&)
> [11] wxFrame::SendIdleEvents(wxIdleEvent&)
> [12] wxAppBase::ProcessIdle()
> [13] wxApp::DoIdle()
> [14] g_main_context_dispatch
> [15] g_main_loop_run
> [16] gtk_dialog_run
> [17] wxMessageDialog::ShowModal()
> [18] wxMessageBox(wxString const&, wxString const&, long, wxWindow*, int, int)
> [19] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&)
> [20] wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*)
> [21] wxEvtHandler::TryHereOnly(wxEvent&)
> [22] wxEvtHandler::DoTryChain(wxEvent&)
> [23] wxEvtHandler::ProcessEvent(wxEvent&)
> [24] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&)
> [25] wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*)
> [26] wxEvtHandler::TryHereOnly(wxEvent&)
> [27] wxEvtHandler::ProcessEventLocally(wxEvent&)
> [28] wxEvtHandler::ProcessEvent(wxEvent&)
> [29] wxScrollHelperEvtHandler::ProcessEvent(wxEvent&)
> [30] wxEvtHandler::SafelyProcessEvent(wxEvent&)
> [31] wxWindow::GTKDoInsertTextFromIM(char const*)
> [32] g_closure_invoke
> [33] g_signal_emit_valist
> [34] g_signal_emit_by_name
> [35] g_closure_invoke
> [36] g_signal_emit_valist
> [37] g_signal_emit_by_name
> [38] g_closure_invoke
> [39] g_signal_emit_valist
> [40] g_signal_emit
> [41] gtk_window_propagate_key_event
> [42] g_closure_invoke
> [43] g_signal_emit_valist
> [44] g_signal_emit
> [45] gtk_propagate_event
> [46] gtk_main_do_event
> [47] g_main_context_dispatch
> [48] g_main_loop_run
> [49] gtk_main
> [50] wxGUIEventLoop::DoRun()
> [51] wxEventLoopBase::Run()
> [52] wxAppConsoleBase::MainLoop()
> [53] APP_KICAD::OnRun() /home/msivak/Work/kicad/kicad/kicad.cpp:233
> [54] wxEntry(int&, wchar_t**)
> [55] main /home/msivak/Work/kicad/kicad/kicad.cpp:265
> [56] __libc_start_main
> [57] _start
>
>
>
> ** Attachment added: "Part library where IRLML6402 has an alias defined"
> https://bugs.launchpad.net/kicad/+bug/1740952/+attachment/5030667/+files/transistors.lib
>

[Seth Hillbrand (sethh)]

@stambaughw-

It completely replaces. Sorry that wasn't clear.

@mars-montik

That looks like a debug assertion. Was it actually a segfault?

[Martin Sivak (mars-montik)]

I applied the second patch only.

The assertion dialog allows me to continue now. I have to uncheck the "Show this dialog again" checkbox to continue in case the part has aliases. It was immediately reappearing otherwise.

It does not segfault anymore.

[Wayne Stambaugh (stambaughw)]

@Orson, please take a look at the patches attached to this bug report and apply them as soon as you get a chance if they make sense.

[Oivind Toien (otoien)]

As I understand the patch has not yet been applied. While testing and reporting bug #1741820, I realized it might be related to the current one, and at the end tried the rename with some combinations of both eeschema and Libedit open at the same time and was able to confirm appcrash also when renaming a symbol. (In my above report I had only LibEdit open). Thus it seems confirmed with the latest Windows nightly too (2018-01-07 revision 47989ccfc), and these bugs might be the same. See details in bug #1741820.

[Mathias Weyland (launchpad-weyland)]

I get something very similar and the IRC room suggested to add my own BT. This is with arch linux and kicad compiled from the kicad-git AUR. I did modify the build flags to disable the new ngspice stuff. kicad -V or kicad --version simply start the program but the version is reported as 445fc0000 in Help -> About Kicad.

Steps to reproduce (no saving required at any stage):

1. Open kicad prject
2. Run library viewer
3. Load symbol using the tree view on the left
4. Rename symbol by hitting 'e' while mouse is hovering over component name (FST3253 -> FST3253x)
5. Rename symbol back to original name (FST3253x -> FST3253)

Application crashes after completion of step 5. BT attached.

[Maciej Suminski (orsonmmz)]

I cannot recreate the problem, but Seth's explanation seems sensible. There is another patch [1] in bug report #1738999 that does a similar thing, so this is the most likely problem here.
I would apply one of them immediately, but I would rather fix the problem at its root instead of calling Unselect() in every place that can potentially remove an entry in the tree widget.

Would you try the attached patch and tell me if it fixes the problem for you? Perhaps that would solve the problem once and for all.

1. https://bugs.launchpad.net/kicad/+bug/1738999/+attachment/5028557/+files/0001-Fix-segfault-in-symbol-library-editor.patch

[Seth Hillbrand (sethh)]

The patch works for me (fixes the crash) and I agree with the point of fixing it at its source.

However, it is annoying that the previously selected symbol is lost in the tree and the tree resets to the top of the list. Would you be adverse to keeping that functionality in the rename routine? I think the patches don't interfere with each other (or just keep the re-select if exists line)

[Martin Sivak (mars-montik)]

I still get the assert, but it allows me to continue with patch from comment #15 on top of master.

I see some weird behavior though. The tree stops displaying the currently edited symbol and restores it on no change. It also messes up the aliases (when the part has alias then it asks me to remove conflicting aliases and removes the alias upon confirmation).

[Maciej Suminski (orsonmmz)]

Thank you for testing, I knew it cannot be that simple. I really wanted to fix it at the root, as there is already another piece of code that clears the selection and restores it after an action.

I will give myself one more day to think about an alternative solution. If I cannot come up with one then I will apply either yours or the other patch. I do not want to keep eeschema in a state when it crashes so easily for too long.

[Oivind Toien (otoien)]

Perhaps I should do another bug report on this, but in at least the latest Windows nightly (2018-01-07 revision 6cee19d37) I have observed what looks like an anomaly when renaming a symbol; to reproduce use an expanded version of the crash test above (so if you get the crash you might not be able to test):
1. Open kicad project
2. Run library viewer
3. Load symbol using the tree view on the left.
4. Rename symbol by hitting 'e' while mouse is hovering over component name (notice symbol changed name and is in boldface, I do not get a crash on this).
5. Save symbol by pressing save current symbol icon in the toolbar (notice the boldface goes away, the new name retained. Nothing else appears).
6. Close and relaunch Libedit. Now suddenly the old symbol with the original name appears again alongside the renamed one in the tree view on the left side.
Comment: In pre sym-lib-table renaming of a symbol creating a copy was normal behavior, except that both symbols would be available right after saving. (So at least it is disconcerting that the original symbol does not show up until relaunch of Libedit.) However the discussion we had in bug #1737964 indicated that the current intentional functionality is not to create a copy of the symbol. Did the intentions change?

[Maciej Suminski (orsonmmz)]

Oivind, thank you for the notice. Obviously it is an erroneous behavior, but it should be fixed now. I am aware of problems related to renaming symbols with multiple aliases (lp:#1737964).

[Maciej Suminski (orsonmmz)]

Another patch attached, I am willing to commit the changes if they fix the problem. I would be grateful if you could verify it, as I cannot get eeschema to crash on my PCs.

[Janis Skujenieks (janis-skujenieks)]

I tested the patch and it works without segfaults. Only the small thing that new part isn't selected after rename, instead parent library gets selected.

[ludovic leau-mercier (ludovic-mercier)]

Hi,

the patch doesn't work :

process : open eeschema > load project > edit symbol with lib editor >
move an element of symbol

ASSERT INFO:
/home/ludovic/workspaceCDT/kicad/eeschema/class_libentry.cpp(272):
assert "m_aliases.size()" failed in GetName(): no aliases defined for symbol

BACKTRACE:
[1] wxAppConsoleBase::CallEventHandler(wxEvtHandler*, wxEventFunctor&,
wxEvent&) const
[2] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&,
wxEvtHandler*, wxEvent&)
[3] wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*)
[4] wxEvtHandler::TryHereOnly(wxEvent&)
[5] wxEvtHandler::ProcessEventLocally(wxEvent&)
[6] wxEvtHandler::ProcessEvent(wxEvent&)
[7] wxScrollHelperEvtHandler::ProcessEvent(wxEvent&)
[8] wxEvtHandler::SafelyProcessEvent(wxEvent&)
[9] g_closure_invoke
[10] g_signal_emit_valist
[11] g_signal_emit
[12] gtk_propagate_event
[13] gtk_main_do_event
[14] g_main_context_dispatch
[15] g_main_loop_run
[16] gtk_main
[17] wxGUIEventLoop::DoRun()
[18] wxEventLoopBase::Run()
[19] wxAppConsoleBase::MainLoop()
[20] wxEntry(int&, wchar_t**)
[21] __libc_start_main
[22] _start

On 09/01/2018 13:42, Maciej Suminski wrote:
> Another patch attached, I am willing to commit the changes if they fix
> the problem. I would be grateful if you could verify it, as I cannot get
> eeschema to crash on my PCs.
>
> ** Patch added: "libedit_rename_fix.patch"
> https://bugs.launchpad.net/kicad/+bug/1740952/+attachment/5033440/+files/libedit_rename_fix.patch
>

--
Ludovic Léau-Mercier

[Maciej Suminski (orsonmmz)]

Hi Ludovic,

I think you are applying the patch on an older revision, this assert should be fixed in 82fc038c which was committed this morning. Please let me know if you use 82fc038c or newer version with this patch.

Anyway, I discovered one more problem with this patch, so here comes version 3, hopefully the last one. I am going to verify the patch on Windows.

[Janis Skujenieks (janis-skujenieks)]

Everything works for me.

[Seth Hillbrand (sethh)]

This does not apply cleanly for me. Trying on 00fb7008

[Maciej Suminski (orsonmmz)]

Seth, would you try https://code.launchpad.net/~orsonmmz/kicad/+git/kicad/+ref/libedit_fixes then?

[Martin Sivak (mars-montik)]

I just tested https://code.launchpad.net/~orsonmmz/kicad/+git/kicad/+ref/libedit_fixes

Renaming simple symbol seems to work fine now. The operation still shows an assert when the symbol in question has aliases defined.

[Seth Hillbrand (sethh)]

That compiled.

Renaming works. No crashes.

Renaming a symbol that has aliases does not work correctly (I think) but it doesn't crash. If there are aliases, we get (renaming 2N2646 to 2N2646a)

/home/seth/code/kicad/orsonmmz/eeschema/libeditframe.cpp(607): assert "alias != __null" failed in OnUpdateViewDoc(): Alias "2N2646" not found in symbol "2N2646a".

Then we are presented with the dialog asking whether we want to remove conflicting aliases. If we click "No", then the symbol is not renamed. If we click "Yes", the symbol is renamed but all the aliases are deleted.

This is definitely a different bug but if you're squashing at the root, it may make sense to handle this issue at the same time?

[Mathias Weyland (launchpad-weyland)]

I can confirm that the issue I reported in #14 cannot be reproduced with the new version from git, which is:

Application: kicad
Version: (2018-01-09 revision 00fb70088)-master, release build
Libraries:
    wxWidgets 3.0.3
    libcurl/7.57.0 OpenSSL/1.1.0g zlib/1.2.11 libidn2/2.0.4 libpsl/0.19.1 (+libidn2/2.0.4) libssh2/1.8.0 nghttp2/1.29.0
Platform: Linux 4.9.74-2-MANJARO x86_64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.3 (wchar_t,wx containers,compatible with 2.8) GTK+ 2.24
    Boost: 1.65.1
    Curl: 7.57.0
    Compiler: GCC 7.2.1 with C++ ABI 1011

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

Awesome work thank you guys!

Matt

[Maciej Suminski (orsonmmz)]

Thank you all for testing, I am pushing the patch as is.

Seth, the bug you experience is reported in #1737964 and is next on my to-do list. I think it has a bit different cause.

[KiCad Janitor (kicad-janitor)]

Fixed in revision 8c42abc10c9f052ce82d69610e94e2dec2b3bb59
https://git.launchpad.net/kicad/patch/?id=8c42abc10c9f052ce82d69610e94e2dec2b3bb59

[Oivind Toien (otoien)]

Thanks, everything in this bug report seems to be fixed with Windows nightly (2018-01-10 revision dc62c0717), also the problem that a "ghost" copy was created on renaming.

[ludovic leau-mercier (ludovic-mercier)]

OK it's work.

Thanks

On 09/01/2018 17:23, Maciej Suminski wrote:
> Hi Ludovic,
>
> I think you are applying the patch on an older revision, this assert
> should be fixed in 82fc038c which was committed this morning. Please let
> me know if you use 82fc038c or newer version with this patch.
>
> Anyway, I discovered one more problem with this patch, so here comes
> version 3, hopefully the last one. I am going to verify the patch on
> Windows.
>
> ** Patch added: "0001-Symbol-Library-Editor-fix-crash-after-renaming-a-sym.patch"
> https://bugs.launchpad.net/kicad/+bug/1740952/+attachment/5033597/+files/0001-Symbol-Library-Editor-fix-crash-after-renaming-a-sym.patch
>

--
Ludovic Léau-Mercier