Access violation when adding Custom Property

[AtD]

Hi guys!
I may have found another way: I cannot produce a crash when I use a Part::Vertex object instead of the Draft Point. Here, the landmark dimension works, adjusts according to changing coordinates and I am able to edit the sketch of the body.

Update: it doesn't crash anymore. But there is still the Access Violation, when something changes in the sketch.

[AtD]

I created a Bug report for the discussed behavior:
https://tracker.freecadweb.org/view.php?id=4741

[wmayer]

I could add a defensive patch but probably it would be better to understand how such duplicate comes and fix this.

The fact that the same object appears several times in the array is probably not easy to avoid. So, a secure way to delete them is putting them into a std::set and then the objects there can be safely destroyed.

But there are more problems in the class GeometryObject. E.g. when looking at clearFaceGeom() then it simply clears the array without destroying the Face objects and that results into a memory leak. Or setEdgeGeometry/setVertexGeometry that replaces the existing arrays without destroying the content before.

So, IMO the only proper solution would to consequently replace the raw pointers with shared pointers. Then one doesn't have to worry about which class is responsible to destroy an object as this will happen automatically.

[wmayer]

...

Here is a branch I have pushed to my repo: https://github.com/wwmayer/FreeCAD/pull/new/issue4741

This only replaces the use of raw pointers of Vertex and Face but maybe we should also do it for the BaseGeom class.

[openBrain]

Here is a branch I have pushed to my repo: https://github.com/wwmayer/FreeCAD/pull/new/issue4741

This only replaces the use of raw pointers of Vertex and Face but maybe we should also do it for the BaseGeom class.

Thanks for keeping me informed. I didn't go deeper in this issue mainly because my spare time is very limited ATM, but to be honest it would have been at the limit of (if not above) my skills/knowledge... -- Actually your branch code makes me think it's above --.
I'm sorry I can't help deciding if same fix should be applied to BaseGeom because I just don't understand what would be the impacts.
Anyway, good to see someone works on this and I'm glad if my preliminary analysis may have help.

[wmayer]

I'm sorry I can't help deciding if same fix should be applied to BaseGeom because I just don't understand what would be the impacts.

First of all, the PR fixes the crash because there is no double deletion any more.

While fixing the code I have seen some places where for the programmer it wasn't clear if to delete an object or not and he decided to do not (probably because of a possible later double deletion). Then there were a few other places where some objects are copied from one array to another which is also error-prone for double deletion.

Thus I fear there are more classes that suffer from possible double deletions. When extending this by BaseGeom there won't be a regression as long as it's done correctly. The only downside is that it requires a bit more memory.

I don't mind if you don't have time ATM. But because you made a suggestion about a possible workaround I thought I will ping you before you start to work on it.

[wandererfan]

This issue #4741 seems to be fixed by @wmayer's patch. Can anybody confirm?

[wandererfan]

This only replaces the use of raw pointers of Vertex and Face but maybe we should also do it for the BaseGeom class.

Using shared_ptr for BaseGeom in development here https://github.com/WandererFan/FreeCAD/tree/memLeak

[wmayer]

This issue #4741 seems to be fixed by @wmayer's patch. Can anybody confirm?

I was able to reproduce the described crash and with my patch it was fixed. Insofar I can confirm it.

[AtD]

Nice to see, the problem is being addressed. Thanks alot, wmayer.

I am a bit confused by the conversation here https://github.com/FreeCAD/FreeCAD/pull/5231
Does it mean, its is not in the current build yet?