Missing support for floating references

[sdroege]

See this for background: https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#floating-ref

Everything inheriting from GInitiallyUnowned (+ corner cases) allow "floating references". This affects GtkWidgets and GstElements for example. tl;dr is the following:

1. Newly created object has refcount of 1 and floating flag set
2. gtk_container_add() (and related: gtk_box_pack_*()), gst_bin_add() call g_object_ref_sink() on the "child" parameter: This does one of two things: a) If floating: unset floating, b) If not floating: refcount++

This is all for some "C programming convenience" (aka footgun often enough). As you can see, this can easily cause leaks or crashes. E.g. gst_bin_add(), gst_bin_remove(), gst_bin_add() will cause the first add() to not increase reference count (it's owned by the bin now), remove() will cause refcount-- (which might destroy the object), the second add() will refcount++ in any case.

What all other bindings are doing is that everything that arrives on the bindings side gets its floating flag removed (as it's just "C convenience"), and then normal reference counting happens. Which means that things like the functions in 2) would be "transfer none" (bug in gst_bin_add(), see https://bugzilla.gnome.org/show_bug.cgi?id=782499), and all kinds of constructors of such objects are also "transfer none" (aka "transfer floating"). You call g_object_ref_sink() whenever getting such object as "transfer none", and from there on all just works as usual.

What I would propose now is one of two options, but both result in not having any floating references ever at the gtk-rs level:

1. Always call g_object_ref_sink() instead of g_object_ref()
2. Like 1) but only for things inheriting from GInitiallyUnowned

Functionally both are the same, but g_object_ref_sink() is minimally more expensive than g_object_ref(). I think we should not care about that for now and just fix the memory leaks (see e.g. https://gist.github.com/sdroege/13459c559040c11b23733ac55d7aa9a7).

In addition double-checking has to happen that all wrappers around GInitiallyUnowned _new() functions use from_glib_none() correctly.

[sdroege]

If you apply the pull request, the testcase from the gist prints 0 (and accesses freed memory while doing so). Without the pull request it prints 1 and the button is leaked.

[EPashkin]

Thanks for explanation and fix.
I added your gist to https://github.com/EPashkin/gtk-rs-runtime-tests/tree/leak for future move to gtk-rs organization

[sdroege]

I added your gist to https://github.com/EPashkin/gtk-rs-runtime-tests/tree/leak for future move to gtk-rs organization

Keep in mind though that the printing of the final reference count (0) is an invalid memory access (to already freed memory).

Thanks for explanation and fix.

Now we just need to decide whether you want to generate different code for GObject and GInitiallyUnowned (i.e. g_object_ref() vs. g_object_ref_sink()). The performance penalty of the latter should be minimal, but it would be cleaner. However that would require some changes to the parser, code generator and the macro. E.g. it could use pub struct Element(InitiallyUnowned<ffi::GstElement>): Object; for the macro to decide which function to use.

Let me know if you want me to update everything according to that, or if the fix from the pull request is enough for now.
Maybe I should've also mentioned that because of this you currently leak almost everything in GTK :)

[EPashkin]

IMHO your fix is enough. I don't think g_object_ref_sink cause noticeable delay.
Final decision on @GuillaumeGomez

[EPashkin]

Also I don't like introduce InitiallyUnowned to rust code.

[sdroege]

Compare https://git.gnome.org/browse/glib/tree/gobject/gobject.c#n2972 (calling https://git.gnome.org/browse/glib/tree/gobject/gobject.c#n2912 ) and https://git.gnome.org/browse/glib/tree/gobject/gobject.c#n3174 . Shouldn't matter for any GTK code really.

I also wouldn't introduce InitiallyUnowned into Rust code, just that the identifier is there for the macro so that the macro can decide on the correct ref-function... and then map both to Object for all other purposes.

[sdroege]

Basically having the glib_wrapper!() macro also match on

pub struct $name:ident(InitiallyUnowned<$ffi_name:path>): $($implements:path),+;

etc., and map that to glib_object_wrapper!() with just some argument or so to let it us a different ref-function.

[sdroege]

Oh nevermind, this all works a bit different via the ObjectRef. So we would have to map those to some InitiallyUnownedRef instead, exposing that in the Rust API. But if that is acceptable, I can do the changes.

Your call in the end :)

[EPashkin]

IMHO even if we decide that we need separate ObjectRef this better not do in current PR pack

[sdroege]

Fine with me, just let me know if you want that at a later time and I can work on it :)

[sdroege]

Let's close this for now then?

[GuillaumeGomez]

We generally wait for the PR to get merged.

[sdroege]

That happened already, or am I missing something? :)

[GuillaumeGomez]

Euh... I think I missed something haha. Time to sleep I think. Thanks again for doing it! :)