mono / gtk-sharp

Gtk# is a Mono/.NET binding to the cross platform Gtk+ GUI toolkit and the foundation of most GUI apps built with Mono
http://www.mono-project.com/GtkSharp
Other
424 stars 140 forks source link

Memory leak #270

Open mikkeljohnsen opened 5 years ago

mikkeljohnsen commented 5 years ago

We have a fairly large product written in GtkSharp using Mono (Linux and macOS) and .NET Framework (Windows).

We have been using Gtk3 for a very long time, but our application is now becoming very unstable because of memory leaks. GtkSharp is simple not able to release native memory.

We have testet with native C gtk3 program, simple creating 1000 dialog boxes and calling Hide and Destroy on them. Valgrind is reporting no memory leaks. The same program creating 1000 dialogs using C#, valgrind is reporting massive native memory leaks, as do Memory Profiler on Windows.

We have been trying to solve this for some time, but have trouble finding the cause.

I hope that someone in here is able to help us, finding this bug in the GtkSharp (wrapper).

See also: https://github.com/GtkSharp/GtkSharp/issues/82

harry-cpp commented 5 years ago

To correct the original poster, this issue is present in both my fork and this repo, here is a simple repro program: https://gist.github.com/cra0zy/ac6ca4c2cebb4e5f2cf316a6d92e32db

sundermann commented 5 years ago

Has this been fixed by calling Destroy() before Dispose() in your case?

mikkeljohnsen commented 5 years ago

@stsundermann No it is not fixed. There is still a lot of native memory that is not freed. If you pack something into the dialog there is a leak. Some memory can be freed calling destroy, but only if you call destory on all widget in the dialog. That is not possible using Glade/Builder, then you have reference all widget and call destroy.

Will will make a better example and upload soon.

madsjohnsen commented 5 years ago

While calling Destroy() is certainly a step in the right direction, I sadly do not think it is a miracle cure.

https://gist.github.com/madsjohnsen/362c8e7ad9a084d9fdfd08957cc84ea6

When creating 1000 Dialogs which does not use Builder, and not accessing the ContentArea I can create Widgets and as long as I Dispose() them and Destroy() the dialog, I do not see a leak. However, if I pack anything into the ContentArea the program leaks. Even if I Dispose() the ContentArea the leak still occurs. Even going as far as calling Destroy() on the ContentArea, does not help, and that seems to be entirely overkill as Destroy() on the dialog should recursively Destroy() all children (?).

Considering that I tried to look at what happens when "accessing" the ContentArea. So the Gtk/Dialog.cs -> GLib/Object.cs path. What stands out a bit to me is g_object_ref on this line: https://github.com/mono/gtk-sharp/blob/c1b9bd4214b78dc426bc6e6a07c23a8ab4ab50ea/glib/Object.cs#L124

If I understand it correctly, even if a widget is destroyed its memory is only ever freed when the ref count is 0. So where/when is the ref that is done in the above line "undone"?

As an experiment I tried to created a static List in GObject to keep track of all widgets reffed by that line, and then accordingly perform an unref in Dispose(). And it does indeed seem to make the leak disappear, however I suspect it is not a viable place to do it.

The most likely answer is that I have misunderstood something :)

sundermann commented 5 years ago

That line ensures that if the object is not owned by the managed side that we take a ref here. This ensures that the object is kept alive as long as it's not collected on the managed side. This doesn't look wrong to me and not keeping a ref in these cases might lead to segfaults on the managed side.

It would be more of more interest to find out why owned_ref is false and if it's correct. There's a owned parameter in the XML file but there are certainly cases in which the parser misinterpreted the attribute.

sundermann commented 5 years ago

You did not include TestDialog.ui in your sample.

madsjohnsen commented 5 years ago

Updated the gist. Can upload the solution instead if you prefer.

madsjohnsen commented 5 years ago

To make it clear, I am also using the GtkSharp repo that the original post links to. Even though I linked to the (identical line) in this repo.

sundermann commented 5 years ago

I think you are right. There is a memory leak in cases where a ToggleRef has not yet been created. In these cases as you pointed out the current code takes a ref. Then it constructs the object. In the default case this will eventually lead to the constructor of Object.cs being called which then sets the Raw property. It then creates the ToggleRef and takes a ref. So there's one ref leaking.

Therzok commented 5 years ago

I think this was fixed in the 2.12 branch. Maybe the code should be made to match the behaviour in 2.12, but that also means adding in other fixes which actually fix manually added wrong refcount changed.

The ref should be there for initially unowned gobjects, which need sinking.

https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html

Therzok commented 5 years ago

I haven't had any time to actually migrate all the patches and/or work done in 2.12 to the 3.0 branch. And I can't make any guarantees on being able to fix it.

If anyone can do a simpler patch to fix this - or migrate the 2.12 changes done through the past year, I'll review and merge it.

mikkeljohnsen commented 5 years ago

I can see that you @Therzok have made many patches in 2.12. ex: https://github.com/mono/gtk-sharp/commit/9f40d6c2516c54e730b52b7326287ddbb603ab5c#diff-ceb73ace5d4318a3141b31d265100c75

We have to start from that point i the gtk-sharp 2.12 branche and start cherry picking paches for the 3.0 branch.

Therzok commented 5 years ago

Yep, it starts around there in the commit history. :+1:

mikkeljohnsen commented 5 years ago

We now have two branches we are testing (in production):

This branch is a attempt to move patches from the 2.12 branch to 3.22 branch. We had this in testing (in production) for 2 weeks and we saw multiple exceptions with cast to ToggleRef and other unexpected behaviors: https://github.com/GSharpKit/GtkSharp/tree/migrate_2.12_memory_patches

This is a simple patch/branch to only fix the memory leaks and we have that in testing from today (in production). I will comment later when we have more data: https://github.com/GSharpKit/GtkSharp/tree/2.12_memory_patches_minimal

I hope that someone will look at these branches and can help out.