StefanSalewski / gintro

High level GObject-Introspection based GTK3/GTK4 bindings for Nim language
MIT License
297 stars 20 forks source link

Call g_object_ref_sink() only on floating objects #80

Open StefanSalewski opened 4 years ago

StefanSalewski commented 4 years ago

There is a bug in template gobjectTemp(): untyped for gintro <= v0.7.7

methodBuffer.writeLine(" discard g_object_ref_sink(result.impl)")

ref_sink should be called only for floating objects like most widgets, but not for GtkWindow, GtkApplicationWindow and other none floating windows.

sdroege commented 3 years ago

It seems like you currently don't handle floating references at all: there is no call to g_object_ref_sink() anywhere. This most likely means that you're leaking memory or keep dangling pointers around.

It is necessary for bindings to handle floating references correctly (and not expose them to the target language!). See https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#floating-ref for details how this is supposed to work, but for one thing you'll have to call g_object_ref_sink() on every transfer none return value of types with floating references. You can call g_object_ref_sink() also on other types and it behaves exactly the same as g_object_ref() in that case, if that makes anything easier for you.

See also https://github.com/StefanSalewski/gintro/issues/108#issuecomment-761773020 which suggests you handle transfer none wrong in general.

StefanSalewski commented 3 years ago

It seems like you currently don't handle floating references at all:

salewski@nuc ~ $ grep g_object_ref_sink ~/.nimble/pkgs/gintro-#head/gintro/* |wc -l 5345

It is nice that you try to help. But most of gintro seems to work fine. I would have to test for memory leaks more indeed.

sdroege commented 3 years ago

Hmm searching in this repository for ref_sink gave no hits at all so I might be missing something.

StefanSalewski commented 3 years ago

The bindings modules are created by the gen.nim generator script during install of the gintro module, that is

nimble install gintro

or

nimble install gintro@head

to get latest changes (recommended currently). That will give you

salewski@nuc ~/GtkProgrammingBook $ ls -lt ~/.nimble/pkgs/gintro-#head/gintro
total 11908
-rw-r--r-- 1 salewski salewski 2308858 Jan 16 16:48 gtk4.nim
-rw-r--r-- 1 salewski salewski  296367 Jan 14 15:59 gtksource5.nim
-rw-r--r-- 1 salewski salewski  268709 Jan 14 15:59 gtksource.nim
-rw-r--r-- 1 salewski salewski  226242 Jan 14 15:59 handy.nim
-rw-r--r-- 1 salewski salewski  177614 Jan 14 15:59 harfbuzz.nim
-rw-r--r-- 1 salewski salewski   86213 Jan 14 15:59 javascriptcore.nim
-rw-r--r-- 1 salewski salewski   47202 Jan 14 15:59 nice.nim
-rw-r--r-- 1 salewski salewski   12904 Jan 14 15:59 notify.nim
-rw-r--r-- 1 salewski salewski   11606 Jan 14 15:59 pangocairo.nim
-rw-r--r-- 1 salewski salewski    8156 Jan 14 15:59 pangoft2.nim
-rw-r--r-- 1 salewski salewski  176448 Jan 14 15:59 pango.nim
-rw-r--r-- 1 salewski salewski   31282 Jan 14 15:59 rsvg.nim
-rw-r--r-- 1 salewski salewski  297579 Jan 14 15:59 soup.nim
-rw-r--r-- 1 salewski salewski   78947 Jan 14 15:59 vte.nim
-rw-r--r-- 1 salewski salewski  427177 Jan 14 15:59 webkit2.nim
-rw-r--r-- 1 salewski salewski  820834 Jan 14 15:59 webkit2webextension.nim
-rw-r--r-- 1 salewski salewski    1445 Jan 14 15:59 xlib.nim
-rw-r--r-- 1 salewski salewski   81923 Jan 14 15:59 gisup3.nim
-rw-r--r-- 1 salewski salewski   52645 Jan 14 15:59 gisup4.nim
-rw-r--r-- 1 salewski salewski  429183 Jan 14 15:59 glib.nim
-rw-r--r-- 1 salewski salewski    1886 Jan 14 15:59 gmodule.nim
-rw-r--r-- 1 salewski salewski   90421 Jan 14 15:59 gobject.nim
-rw-r--r-- 1 salewski salewski   81840 Jan 14 15:59 graphene.nim
-rw-r--r-- 1 salewski salewski  116921 Jan 14 15:59 gsk.nim
-rw-r--r-- 1 salewski salewski   16750 Jan 14 15:59 gstapp.nim
-rw-r--r-- 1 salewski salewski  118863 Jan 14 15:59 gstbase.nim
-rw-r--r-- 1 salewski salewski  666232 Jan 14 15:59 gst.nim
-rw-r--r-- 1 salewski salewski 2708779 Jan 14 15:59 gtk.nim
-rw-r--r-- 1 salewski salewski  107752 Jan 14 15:59 cairoimpl.nim
-rw-r--r-- 1 salewski salewski   11187 Jan 14 15:59 cairo.nim
-rw-r--r-- 1 salewski salewski      74 Jan 14 15:59 dummygtk.nim
-rw-r--r-- 1 salewski salewski     446 Jan 14 15:59 fontconfig.nim
-rw-r--r-- 1 salewski salewski     569 Jan 14 15:59 freetype2.nim
-rw-r--r-- 1 salewski salewski  347723 Jan 14 15:59 gdk4.nim
-rw-r--r-- 1 salewski salewski  377339 Jan 14 15:59 gdk.nim
-rw-r--r-- 1 salewski salewski  107262 Jan 14 15:59 gdkpixbuf.nim
-rw-r--r-- 1 salewski salewski   30847 Jan 14 15:59 gdkx114.nim
-rw-r--r-- 1 salewski salewski   31953 Jan 14 15:59 gdkx11.nim
-rw-r--r-- 1 salewski salewski    2564 Jan 14 15:59 gimplglib.nim
-rw-r--r-- 1 salewski salewski   12457 Jan 14 15:59 gimplgobj.nim
-rw-r--r-- 1 salewski salewski    7631 Jan 14 15:59 gimplgtk.nim
-rw-r--r-- 1 salewski salewski    3953 Jan 14 15:59 gimplnice.nim
-rw-r--r-- 1 salewski salewski 1270554 Jan 14 15:59 gio.nim
-rw-r--r-- 1 salewski salewski  131491 Jan 14 15:59 atk.nim
-rw-r--r-- 1 salewski salewski    2718 Jan 13 16:57 gimplgst.nim

Indeed Mr. Rumpf does not like these "on the fly" generation that much, I think we already discussed it too. Later, when we may have a few thousand or at least a few hundred Nim GTK users we may try to ship pre-generated module files. But currently I can not provide these, as that would mean creating weekly fresh files for Linux, Mac and Windows, maybe for 32 and 64 bit.

StefanSalewski commented 3 years ago

Sorry, it is

nimble install gintro@#head