GLibSharp / GtkSharp

.Net bindings for gtk+3 and above
Other
27 stars 21 forks source link

Ensure to sink floating references passed unowned to GetObject() and to not increase their reference count #41

Closed sdroege closed 4 years ago

sdroege commented 4 years ago

commit c7d7c9615477242583761bc1dc80418a9a502993 (HEAD -> floating-references) Author: Sebastian Dröge sebastian@centricular.com Date: Tue Jan 28 12:50:17 2020 +0200

Ensure to sink floating references passed unowned to GetObject() and to not increase their reference count

The API contract between GObject-Introspection and bindings is that
functions returning transfer-none floating references pass a reference
to the bindings that should be taken ownership of by sinking it. Not
doing so is wrong and will lead to memory leaks or double frees.

Previously we would not distinguish this case and simply increment the
reference count. In addition we would then sink the floating reference
when the Object.Raw field is set later for InitiallyUnowned subclasses.

Remove that last part and instead check directly in Object.Raw.set if we
get a floating reference and if so simply sink it here and take
ownership of it. The general assumption of Object.Raw.set is that it
gets passed a reference that it should take ownership of.

So in summary:
1) GetObject() would only increase the reference count of unowned,
   non-floating references so that we own it. For unowned, floating
   references it assumes ownership of the reference.
2) Raw.set assumes ownership of the reference passed to it and if it
   happens to be a floating reference then it will first sink it.

Also warn if we get a floating, owned reference passed to GetObject() as
that case is not allowed by GObject-Introspection and would cause the
reference to be leaked.

This fixes a memory leak with functions returning unowned, floating
references and with functions returning owned, non-floating references
of InitiallyUnowned subclasses. And at the same time keeps constructors
for InitiallyUnowned subclasses working correctly without leaks.

See https://gitlab.freedesktop.org/gstreamer/gstreamer-sharp/issues/31

commit e752b5b4837572d98c0a485bfcf6651e1b4e7036 Author: Sebastian Dröge sebastian@centricular.com Date: Tue Jan 28 12:49:29 2020 +0200

Use correct GType for GLib.InitiallyUnowned

It's not the same type as GLib.Object but a subclass thereof.
sdroege commented 4 years ago

Needs some more changes, on the way.

sdroege commented 4 years ago

Note that https://github.com/GtkSharp/GtkSharp is breaking this even more with this commit: https://github.com/GtkSharp/GtkSharp/commit/2e1882d31ea655dd04a39eefd6889d8a9e7643cd

That's making the situation much worse than before.

thiblahute commented 4 years ago

Are we missing anything else here?

sdroege commented 4 years ago

I don't think so, this should be good to merge but there might of course be more related bugs elsewhere :) I didn't observe any further memory leaks in my tests though.

thiblahute commented 4 years ago

It makes sense to me, I guess we should get it in and hope see if there are regressions.

sdroege commented 4 years ago

Thanks for merging!

City-busz commented 1 week ago

This change causes random crashes in Gnome Subtitles. See: https://gitlab.gnome.org/GNOME/gnome-subtitles/-/issues/212