GtkBuilder issue

Platform Language bindings
1

Yesterday we got a new issue for the gintro Nim GTK bindings:

Widgets created by GtkBuilder are sometimes unref'd/destroyed too soon · Issue #203 · StefanSalewski/gintro · GitHub

Of course that is a strange use of GtkBuilder, as he creates the GtkApplication window manually, and then inserts a child widget from builder. And the use of buildSomeUI() at all is a bit strange too. I do generally not use GtkBuilder whenever possible myself, so I can not really decide if he uses it in a wrong way, I would assume that he has seen code like this somewhere and translated that to Nim?

Well, as the error messages looked a bit strange to me, I translated his code to plain C for testing. I have put the UI description in its own file, as C does not allow raw strings, and I wanted to avoid escaping all the quotation marks. Here are the two C compatible files, b2.ui and e2.c, based on Building user interfaces: GTK 4 Reference Manual

<?xml version="1.0" encoding="UTF-8"?>

<interface>
<object class="GtkBox" id="examplewidget">
<property name="valign">center</property>
<property name="halign">center</property>
<property name="orientation">vertical</property>
<child>
  <object class="GtkButton" id="examplebutton">
    <property name="label">Hello, I am a button! I like being a button.</property>
  </object>
</child>
</object>
</interface>

// gcc `pkg-config --cflags gtk4` -o test e2.c `pkg-config --libs gtk4`
#include <gtk/gtk.h>
#include <glib/gstdio.h>

static GtkBox* buildSomeUI(void)
{
  GtkBuilder *builder = gtk_builder_new();
  gtk_builder_add_from_file (builder, "b2.ui", NULL);
  GObject *box = gtk_builder_get_object (builder, "examplewidget");
  g_object_unref (builder); // this causes the issue!
  return GTK_BOX(box);
} 

static void
activate (GtkApplication *app, gpointer user_data)    
{
  GtkWidget *window = gtk_application_window_new(app);
  gtk_widget_show (GTK_WIDGET (window));
  GtkBox *box = buildSomeUI();
  gtk_window_set_child(GTK_WINDOW(window), GTK_WIDGET(box));
}

int main (int   argc, char *argv[])
{
  GtkApplication *app = gtk_application_new ("org.gtk.example", G_APPLICATION_FLAGS_NONE);
  g_signal_connect (app, "activate", G_CALLBACK (activate), NULL);
  int status = g_application_run (G_APPLICATION (app), argc, argv);
  g_object_unref (app);
  return status;
}

When I compile and run the code I get

$ gcc `pkg-config --cflags gtk4` -o test e2.c `pkg-config --libs gtk4`

$ ./test 

(test:13757): GLib-GObject-WARNING **: 10:06:53.126: invalid unclassed pointer in cast to 'GtkBox'

(test:13757): GLib-GObject-WARNING **: 10:06:53.126: invalid unclassed pointer in cast to 'GtkWidget'

(test:13757): Gtk-CRITICAL **: 10:06:53.126: gtk_window_set_child: assertion 'child == NULL || GTK_IS_WIDGET (child)' failed

The problem is “g_object_unref (builder);” in buildSomeUI(), which is executed automatically for the Nim code by Nim’s memory management system. But well, the original example from the GTK homepage executes g_object_unref (builder) as well. So where exactly is the problem? Is his code just invalid? (My guess would be, that the problem is, that he just uses the GtkBox from builder, but creates the app window manually?)

2

Independent of everything else, gtk_builder_get_object()'s return value is (transfer none) and only valid as long as the builder is around. Here you get rid of the builder but use the return value afterwards (the box), which gives you a classic use-after-free bug.

I would assume that the Nim bindings would always immediately get a strong reference for (transfer none) return values to avoid this, i.e. calling g_object_ref() immediately here?

3

FWIW, doing the refcounting correctly makes the C testcase work at least.

4

That was my initial assumption. But looking at the example from Building user interfaces: GTK 4 Reference Manual I am not really sure why in that case it is OK to call g_object_unref (builder);

[EDIT] Actually the example from Building user interfaces: GTK 4 Reference Manual works fine in our Nim version. To keep the builder instance automatically alive, as long as objects returned from gtk_builder_get_object() are in use, is not that easy for the gintro Nim bindings. That use case would need some additional extra code then, compared from ordinary objects like a GtkButton returned from gtk_button_new().

5

That’s a quite severe bindings bug then. Whenever you have a return value (or out parameter) that is (transfer none) or (transfer container) you must ensure to create a copy/new reference on the binding side.

The only exception to that is if you actually know the lifetime of the returned value and can somehow model that in your language, but I don’t think Nim has anything for that. (To be clear: this requires manual bindings that consider the situation for every single function and in many cases this is not possible at all!)

That doesn’t have anything to do with this being GtkBuilder or not but is a general, conceptual bug you have right now.

AFAICS the reason why that GTK example works while unreffing the builder is that windows have an additional strong reference internal to GTK until they’re destroyed. That is not the case for normal widgets.

1 Like
6

For me this GtkBuilder case looks very special. As in this case I have not to keep the object that I get alive, like the button instance returned from gtk_button_new(), but I have to keep the builder instance alive, from which I get an object. Are there other functions, that work similar? I can not see a case like that. Similar are containers like windows, from which I may get child widgets. But in that case I have to keep the childs alive, not the parent container Window.

FWIW, doing the refcounting correctly makes the C testcase work at least.

Can you please post how exactly you fixed my initial C example. I may try to fix the Nim bindings in a similar way then.

[EDIT] And do you know where excatly we can read about the fact, that we have to keep the builder instance alive as long as we are using objects returning from the builder. I can currently not find that info.

7

Like I said: the return value is marked as (transfer none). That tells you that without knowing anything else you must get a new reference / copy of the return value on the bindings side ASAP. And this has nothing to do with this being GtkBuilder or any other API. Not doing this will cause a lot of use-after-free bugs in applications using your bindings.

For GtkBuilder you can know that the object is valid as long as the builder is alive by checking the code. If you can model this lifetime in Nim then you can handle builder in a special case.

That’s a constructor. While its return value is (transfer none), widgets are floating references so you would actually call g_object_ref_sink() on the return value the moment it arrives in the bindings. If you don’t do that for GInitiallyUnowned subclasses then you have another conceptual bug in your bindings right now that will cause memory leaks and/or use-after-free bugs.

On GInitiallyUnowned types you call g_object_ref_sink() in case of (transfer none) immediately at the C/bindings boundary, on others g_object_ref().

Exactly. You have to keep the child (-> return value) alive because the return value is annotated as (transfer none). That’s the same for gtk_builder_get_object().

If you don’t keep the return value alive then you must keep the builder alive, and that the lifetime of the return value is bound to the container is a special case of GtkBuilder (and various other APIs) that is not something you can statically know based on the introspection data.

--- test.c.old	2022-05-13 14:23:42.919799337 +0300
+++ test.c	2022-05-13 12:39:43.039509906 +0300
@@ -5,7 +5,7 @@
 {
   GtkBuilder *builder = gtk_builder_new();
   gtk_builder_add_from_file (builder, "test.ui", NULL);
-  GObject *box = gtk_builder_get_object (builder, "examplewidget");
+  GObject *box = g_object_ref(gtk_builder_get_object (builder, "examplewidget"));
   g_object_unref (builder); // this causes the issue!
   return GTK_BOX(box);
 } 
@@ -17,6 +17,7 @@
   gtk_widget_show (GTK_WIDGET (window));
   GtkBox *box = buildSomeUI();
   gtk_window_set_child(GTK_WINDOW(window), GTK_WIDGET(box));
+  g_object_unref(box);
 }
 
 int main (int   argc, char *argv[])

This is what your bindings should’ve been doing automatically already based on the transfer annotations on all the involved functions.

Don’t fix this in a special way for GtkBuilder but fix it generically by handling the transfer annotations correctly (and also floating references if you don’t yet).

1 Like
8

Sorry, I think I completely misunderstood your first reply:

only valid as long as the builder is around.

Actually, what is missing from the Nim bindings is just a call of g_object_ref() for all the entities that we get from the builder. No need to keep the builder instance alive. This seems to be a systematic error in the Nim bindings generator, but only for gobjects returned from the builder. In all other cases it is fine, and this builder case is easily to fix. At least in principle, we always have to care that the Nim memory management frees the objects when they go out of scope, but we do it right in all other cases, so we can do it for this builder case as well.

Indeed, I think I had a bad week. For the initial issue, I tried something stupid like

  let builder = newBuilderFromString(ui, ui.len)
  result = builder.getBox("examplewidget")
  GC_ref(result) # fully stupid!

instead of the intended

  let builder = newBuilderFromString(ui, ui.len)
  result = builder.getBox("examplewidget")
  discard g_object_ref(result.impl) # this fixes the issue and tells us how to fix the bindings!
1 Like
9

This should be about the same code for all functions returning a (transfer none) return value, FWIW. Just note that for GInitiallyUnowned subclasses you must use g_object_ref_sink() instead (or simply for all GObjects as it does the same as g_object_ref() otherwise).

10

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.