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
429 stars 140 forks source link

[Gdk] Fix pixbuf leak #188

Closed Therzok closed 7 years ago

Therzok commented 7 years ago

This forced a pixbuf ref on querying. This was not good, as the managed wrapper already takes a ref. So we'd continously ref the pixbuf on the same object, without setting the Owned property. While we could grab ownership of the pixbuf, better rely on the ref/unref semantics to properly handle this.

Therzok commented 7 years ago

This changes:

         [DllImport("libgdk_pixbuf-2.0-0.dll", CallingConvention = CallingConvention.Cdecl)]
         static extern IntPtr gdk_pixbuf_loader_get_pixbuf(IntPtr raw);

         public Gdk.Pixbuf Pixbuf { 
             get {
                 IntPtr raw_ret = gdk_pixbuf_loader_get_pixbuf(Handle);
                 Gdk.Pixbuf ret = GLib.Object.GetObject(raw_ret) as Gdk.Pixbuf;
-                g_object_ref (raw_ret);
                 return ret;
             }
         }
decriptor commented 7 years ago

https://bugzilla.xamarin.com/show_bug.cgi?id=51535

knocte commented 7 years ago

@Therzok do you mind adding bug URL to commit message too please? Thanks

Therzok commented 7 years ago

Added it.

alanmcgovern commented 7 years ago

I'm a little worried by the Atk change because there's a method called ref_child and it no longer refs the child. We've definitely broken the semantics behind this method. It looks like native code is expecting a native ref to be added, but with this change we are not doing that. This method is now a no-op for the case where the managed wrapper already exists, and for the case where the managed wrapper does not exist we add a ref, but it's a ref owned by managed, not by native.

Therzok commented 7 years ago

The native code is a NOP if ref_child isn't set. So we should do the same, and not ref. The implementation should handle ref-ing.

Therzok commented 7 years ago

Split atk changes into its own PR https://github.com/mono/gtk-sharp/pull/192

alanmcgovern commented 7 years ago

Manually merged as i wanted to edit the commit message to append the bug#

Therzok commented 7 years ago

Don't know how it got removed.