FreeCAD 0.19_pre Crashes (SIGSEGV) on MacOS; Triggered by kicadStepUp

[wmayer]

OK, next try with a build that uses the address-sanitizer option. It's a clang specific thing https://clang.llvm.org/docs/AddressSanitizer.html and when running it then FreeCAD already crashes when loading the demo file. The function calls at the top of the callstack is the same as in the bug report.

[wmayer]

When using the above build the whole issue can be reduced to a three-liner:

Code: Select all

doc = App.newDocument()
obj = doc.addObject("App::Origin")
doc.removeObject(obj.Name)

Callstack is:

#0 0x7fba36f21dde in App::DocumentObject::getNameInDocument() const /home/user/Projects/FreeCAD/src/App/DocumentObject.cpp:246:10
#1 0x7fba3a0cccb6 in Gui::DocumentItem::getViewProvider(App::DocumentObject*) /home/user/Projects/FreeCAD/src/Gui/Tree.cpp23
#2 0x7fba3a0ce1f4 in Gui::TreeWidget::_slotDeleteObject(Gui::ViewProviderDocumentObject const&, Gui::DocumentItem*) /home/user/Projects/FreeCAD/src/Gui/Tree.cpp37
#3 0x7fba3a0c7f00 in Gui::TreeWidget::slotDeleteObject(Gui::ViewProviderDocumentObject const&) /home/user/Projects/FreeCAD/src/Gui/Tree.cpp5
<snip> ignore the whole boost signal processing
#19 0x7fba396a65b2 in Gui::Document::slotDeletedObject(App::DocumentObject const&) /home/user/Projects/FreeCAD/src/Gui/Document.cpp:776:9
<snip> ignore the whole boost signal processing
#35 0x7fba36c039bb in App::Document::removeObject(char const*) /home/user/Projects/FreeCAD/src/App/Document.cpp:4166:5
#36 0x7fba370b8862 in App::DocumentPy::removeObject(_object*) /home/user/Projects/FreeCAD/src/App/DocumentPyImp.cpp:303:27
...

Interesting is the callstack of the deleted object that causes all the troubles:

#0 0x4ce93d in operator delete(void*) (/home/user/Projects/build_asan/bin/FreeCAD+0x4ce93d)
#1 0x7fba371c9b47 in App::Line::~Line() /home/user/Projects/FreeCAD/src/App/OriginFeature.h:59:17
#2 0x7fba36ef7a8f in std::default_delete<App::DocumentObject>::operator()(App::DocumentObject*) const /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/unique_ptr.h:78:2
#3 0x7fba36c4a261 in std::unique_ptr<App::DocumentObject, std::default_delete<App::DocumentObject> >::~unique_ptr() /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/unique_ptr.h:263:4
#4 0x7fba36c044f4 in App::Document::removeObject(char const*) /home/user/Projects/FreeCAD/src/App/Document.cpp:4228:1
#5 0x7fba371be20c in App::Origin::unsetupObject() /home/user/Projects/FreeCAD/src/App/Origin.cpp:181:37
#6 0x7fba36c0394e in App::Document::removeObject(char const*) /home/user/Projects/FreeCAD/src/App/Document.cpp:4163:22
#7 0x7fba370b8862 in App::DocumentPy::removeObject(_object*) /home/user/Projects/FreeCAD/src/App/DocumentPyImp.cpp:303:27
#8 0x7fba370a17b4 in App::DocumentPy::staticCallback_removeObject(_object*, _object*) /home/user/Projects/build_asan/src/App/DocumentPy.cpp57
#9 0x7fba37fdd53a in _PyCFunction_FastCallDict (/usr/lib/x86_64-linux-gnu/libpython3.6m.so.1.0+0x21353a)

So, the deleted object is an App::Line feature that's part of the Origin feature. I guess it's the nested calls to remove an object that the tree view cannot handle correctly.

EDIT: The Line feature that causes the crash is the X_Axis of the Origin feature. I have added some printf() statements and its destructor is called before accessing it again in the tree view.

[berka]

When using the above build the whole issue can be reduced to a three-liner:

And I suspect I've seen this in more places in the past few years: usually when moving stuff in tree view among 'group's and 'part's, a crash. I don't get to dig in often. I'm usually using FreeCAD under pressure to get a design done.

BTW, I thank you @wmayer (and @realthunder) for your work on FreeCAD. I greatly appreciate that you're making and improving an open-source project that I find capable enough to design a product.

I'm glad you have that address-sanitizer option. That seems to make your debug much easier. Is there a way others can get more debuggable builds. I think it would be nice if we could download versions of weekly binaries with more debugging enabled: like symbols for source_file:line and address-sanitizer (or similar) options turned on. It would eliminate one of the reasons I can't dig into one of these kinds of issues -- I know how hard it can be to get a build to work on this project, so I pass building myself. I primarily want to use the tool, not make the tool.

With that comment I found in ::getViewProvider(), it sounds like you could be finding just one of a whole class of errors. And unfortunately, it's hard for every contributor to know enough to avoid causing them in the future. Since the beginning of FC, there's new stuff like STL smart pointers. It may be worthwhile to consider using them. They may help prevent future bugs propagating for years.

[wmayer]

And I suspect I've seen this in more places in the past few years: usually when moving stuff in tree view among 'group's and 'part's, a crash.

With a normal release build it is very difficult to catch this kind of errors.

I'm glad you have that address-sanitizer option. That seems to make your debug much easier.

That's a great option. However, you cannot find everything with it.

Is there a way others can get more debuggable builds. I think it would be nice if we could download versions of weekly binaries with more debugging enabled: like symbols for source_file:line and address-sanitizer (or similar) options turned on

I am not sure if this is feasible. You must be aware of that this version cannot be used for productive work because everything is much slower. So I think there are not that many people who use it and thus the extra effort to provide them is questionable.

With that comment I found in ::getViewProvider(), it sounds like you could be finding just one of a whole class of errors

One must be careful with this comment. I looked through the git history of Tree.cpp a bit and in the past it was used in a different context.
See: https://github.com/FreeCAD/FreeCAD/blob ... .cpp#L1059

The question is if the comment still makes sense.

Since the beginning of FC, there's new stuff like STL smart pointers. It may be worthwhile to consider using them. They may help prevent future bugs propagating for years.

What would be needed is the combination of shared and weak pointers. This way we could avoid all dangling pointers when an object is removed from the document. However, it will be a huge amount of work to get this done.

Some time ago I have implemented my own weak pointer class for features and it works pretty well. However, I am not sure if this is also suitable to be used in the tree where you can easily have several hundreds or thousands of objects.

[easyw-fc]

ping

Hi Werner, is there a way in my WB code to remove or reduce the triggering of this issue?

[berka]

And I suspect I've seen this in more places in the past few years: usually when moving stuff in tree view among 'group's and 'part's, a crash.

With a normal release build it is very difficult to catch this kind of errors.

I'm not claiming consistently-repeatable crashes. Just as you'd expect: the occasional crash that happens when moving stuff around in tree view. It's happened enough that I know to save before manipulating Group/Part/Body hierarchy.

You must be aware of that this version cannot be used for productive work because everything is much slower. So I think there are not that many people who use it and thus the extra effort to provide them is questionable.

I understand it wouldn't be usable for productive work. I'm envisioning that case when I have a random crash after "move to other body" or similar. Then I could launch in the debuggable version and try it again from my saved file.

Without the address-sanitizer option, the crash is unlikely to happen on my reproduction attempt. And without file:line symbols, it's harder to find my way around the project. I'm not immersed in it as you are.

I understand and respect you have to weigh work required vs. benefit vs. user confusion of more versions floating around. I'm just pointing this out from my viewpoint -- someone who wants to use the tool, but also contribute a little to the inevitable bug hunting.

One must be careful with this comment... The question is if the comment still makes sense.

Yeah; it's kind of generic: don't pass an invalid pointer. It just points out that someone had been here and seen this issue in the past. It was a good comment to get me thinking about who would know. I recognized that it was 10 years old, and had gotten there after refactoring; a lot changed in 10 years.

What would be needed is the combination of shared and weak pointers. This way we could avoid all dangling pointers when an object is removed from the document. However, it will be a huge amount of work to get this done.

Some time ago I have implemented my own weak pointer class for features and it works pretty well. However, I am not sure if this is also suitable to be used in the tree where you can easily have several hundreds or thousands of objects.

That class looks a bit like the std::shared_ptr except no dereference operator. I haven't tried converting anything from dumb pointers to smart pointers, so I can't comment on the work. On the surface, it looks like most of the pointer syntax would be supported (by shared_ptr). I was thinking, then most of the work would be limited to whatever functions are creating and destroying the pointed objects. I don't have a sense of the performance penalty in your thousands-of-objects case.

[wmayer]

ping

Hi Werner, is there a way in my WB code to remove or reduce the triggering of this issue?

A lot of objects are being added and deleted without undo/redo. So, to avoid the dangling pointer to the object you can activate undo/redo mode of the document and at the end clear the undos.

Code: Select all

doc = App.newDocument()
doc.UndoMode = 1
doc.openTransaction()
... # your code
doc.commitTransaction()
doc.clearUndos()

This way the item in the tree view won't become dangling because the objects still exists. At this time it's on the undo stack and not part of the document any more.

So to extend my example code above this will avoid the crash:

Code: Select all

doc = App.newDocument()
doc.UndoMode = 1
obj = doc.addObject("App::Origin")
doc.openTransaction()
doc.removeObject(obj.Name)
doc.commitTransaction()
doc.clearUndos()

[wmayer]

That class looks a bit like the std::shared_ptr except no dereference operator.

Actually it is supposed to more look like the std::weak_ptr.

I haven't tried converting anything from dumb pointers to smart pointers, so I can't comment on the work.

That's easy. All the feature types are created with a factory method of their type ids and that delivers a raw pointer. This raw pointer can be directly passed to a shared_ptr.

The hard work now is to change all the code that currently uses a raw pointer. This must be changed to use a weak_ptr. The weak_ptr will be required because it doesn't affect the internal reference counter so that the document is guaranteed to have full control over its features as otherwise it can easily happen that we produce memory leaks.

I don't have a sense of the performance penalty in your thousands-of-objects case.

There is for sure a certain overhead with shared_ptr/weak_ptr but I actually meant my class WeakPtrT that internally uses several boost signals and that might be expensive.

[realthunder]

EDIT: The Line feature that causes the crash is the X_Axis of the Origin feature. I have added some printf() statements and its destructor is called before accessing it again in the tree view.

What do you think about this patch, which makes it safe to lookup a view provider given a raw object pointer using Application::Instance->getViewProvider(). I have changed DocumentItem::getViewProvider() in my branch to call this.

[easyw-fc]

A lot of objects are being added and deleted without undo/redo. So, to avoid the dangling pointer to the object you can activate undo/redo mode of the document and at the end clear the undos.

Code: Select all

doc = App.newDocument()
doc.UndoMode = 1
doc.openTransaction()
... # your code
doc.commitTransaction()
doc.clearUndos()

This way the item in the tree view won't become dangling because the objects still exists. At this time it's on the undo stack and not part of the document any more.

@Werner many thanks for your suggestion...
I have added the following code on loading pcb and tracks:

Code: Select all

doc.openTransaction('add_kicad')
... # my function to load kicad objects
doc.commitTransaction()

should it be fine?

ping

ping

I have updated ksu wb... would you please have a try to see if this has solved the issue both for board and tracks?
easyw/kicadStepUpMod
it should be available with the FC Addons tool