Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak #270

Open
mikkeljohnsen opened this issue May 1, 2019 · 14 comments
Open

Memory leak #270

mikkeljohnsen opened this issue May 1, 2019 · 14 comments

Comments

@mikkeljohnsen
Copy link

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: GtkSharp/GtkSharp#82

@harry-cpp
Copy link

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
Copy link
Contributor

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

@mikkeljohnsen
Copy link
Author

@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
Copy link

madsjohnsen commented May 3, 2019

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:

g_object_ref (o);

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
Copy link
Contributor

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
Copy link
Contributor

You did not include TestDialog.ui in your sample.

@madsjohnsen
Copy link

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

@madsjohnsen
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

Therzok commented May 4, 2019

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
Copy link
Contributor

Therzok commented May 4, 2019

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
Copy link
Author

I can see that you @Therzok have made many patches in 2.12. ex: 9f40d6c#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
Copy link
Contributor

Therzok commented May 6, 2019

Yep, it starts around there in the commit history. 👍

@mikkeljohnsen
Copy link
Author

mikkeljohnsen commented Jun 7, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants