Looking at the example in the GtkMenu docs makes it clear that popup is not expected to do any such thing. It would be good to a) fix the places in GTK+ you identify and b) clarify the docs.

It seems a bit unreasonable to expect people to keep a reference around to the menu in order to notice when it pops down and destroy it for themselves...  I'd assume that if the user is not holding their own reference, we should assume that they don't want it to remain alive.

Also: the example in the docs does exactly the leaky behaviour: creates a menu, pops it up, and then never destroys it.

If the original bug report is correct then even our own docs can't get it right and this is truly a bad API...

fwiw:

#include <gtk/gtk.h>

void
is_destroyed (gpointer user_data,
              GObject *where_the_object_was)
{
  g_print ("menu was destroyed\n");
}

int
main (void)
{
  GtkWidget *menu;

  gtk_init (0, 0);

  menu = gtk_menu_new ();
  gtk_menu_shell_append (GTK_MENU_SHELL (menu), gtk_menu_item_new_with_label ("foo"));
  gtk_widget_show_all (menu);

  g_object_weak_ref (G_OBJECT (menu), is_destroyed, NULL);

  gtk_menu_popup (GTK_MENU (menu), NULL, NULL, NULL, NULL, 0, 0);

  /* gtk_widget_destroy (menu); */

  gtk_main ();
}


This never prints "menu was destroyed".  If you uncomment the explicit destroy then (of course) it does.

If we're worried about breaking API then at very least we should probably add gtk_menu_set_destroy_on_popdown() or such.

Some more info: the toplevel window that contains the menu is created when the GtkMenu is first initialised (from gtk_menu_init) and popping down the menu only calls hide() on it.

As long as this toplevel exists it will be in Gtk's toplevel list and a ref will be held from there.  The toplevel, in turn, holds the menu.  There is no ref cycle between the menu and the toplevel because the menu only keeps a weak ref on the toplevel (never having obtained a strong ref from gtk_window_new() because of how that function operates internally).

We could be tempted to delay the creation of the toplevel to the point where the user first calls popup() and then to destroy the toplevel (but preserve the child by deparenting it first) at popdown time, but this is very weird.  Presently GtkMenu self-consumes its floating ref on construction by inserting itself into this toplevel, so gtk_menu_new() is truly transfer:none.  Making this change would change it to returning a floating ref which would be sunk when you call popup() and eventually unreffed when the menu pops down.  From a "design this API from scratch" standpoint this is actually exactly what I would expect, but it's possibly too weird of a change to make now.

Introducing a _set_destroy_on_popdown() would also be very strange because I'd be inclined to implement that by doing exactly as above: not having a toplevel window when the menu is not popped up.  This means that calling that function would unparent the menu from its existing toplevel (created at construct) and free the toplevel.  Unfortunately that would also destroy the menu because its floating ref was already sunk and removing it from the toplevel dropped the last ref.  We could add another ref or force the object floating again, but now we're getting into seriously ugly territory.

Because of all of the above, I think the only reasonable thing to do here is to add a _set_destroy_on_popdown() that operates literally as the name suggests: in gtk_menu_popdown() we destroy the menu, preventing it from being represented, no matter how many refs the user is holding on to.  I think that this is probably what most people want anyway -- if they were interested in reusing the menu, then why would they set this?

(In reply to comment #2)

> Also: the example in the docs does exactly the leaky behaviour: creates a menu,
> pops it up, and then never destroys it.
> 

No, it doesn't. It shows creating a menu and using gtk_menu_popup on it repeatedly. That would break if you change the semantics as you propose.

(In reply to comment #5)
> No, it doesn't. It shows creating a menu and using gtk_menu_popup on it
> repeatedly. That would break if you change the semantics as you propose.


ah.  you're right.  I saw the "menu" local variable and didn't pay attention to where it came from.

But I'm not proposing to change the semantics.  In fact, I'm proposing to keep them the same exactly because of situations like this, which probably widely exist in the wild...

Created attachment 264423 [details] [review]
GtkMenu: add option to destroy on popdown

Many people create GtkMenu in order to pop it up once and then never use
it again.  It is inconvenient for these people to wait around for the
menu to be popped down and have to call destroy() on it manually, so add
gtk_menu_set_destroy_on_popdown() for them.



This is one possible approach.

I think another approach which could be considered conceptually cleaner
(but would do a lot to complicate the code) would be to introduce
gtk_menu_new_floating() which created a GtkMenu that behaves as my
"ideal world" discussion above.  Due to the complexity, I do not favour
this approach and would rather just go with the patch here.

If you just g_object_unref (menu), doesn't it destroy itself when closed?

You don't own a ref on the menu that you just created...

Review of attachment 264423 [details] [review]:

::: gtk/gtkmenu.c
@@ +5688,3 @@
+ * See gtk_menu_set_destroy_on_popdown().
+ *
+ * Returns: whether the menu will be destroy when it is popped down

typo: destroyed

Review of attachment 264423 [details] [review]:

::: gtk/gtkmenu.c
@@ +697,3 @@
+   */
+  g_object_class_install_property (gobject_class,
+                                   PROP_RESERVE_TOGGLE_SIZE,

PROP_DESTROY_ON_POPDOWN

Review of attachment 264423 [details] [review]:

So I think that dropping the property and having popup_once() that sets the internal flag and calls popup() might be the way to go here.

(In reply to comment #0)
> When calling gtk_menu_popup() and not keeping a reference to the menu around,
> I'd expect that the menu is destroyed after being closed. It isn't.
> 
> A few places in gtk expect this too and seem to leak as a result
> (gtkappchooserwidget.c:224, gtkcolorsel.c:1487).

I'm not sure how the leak in gtkappchooserwidget.c is related to this, though. If the menu is popped up, since it was attached to a widget, it should get destroyed properly (when the widget will be destroyed).

If the menu isn't popped up OTOH you have a leak, but it's unrelated since it wasn't ever shown/popped up. In that case the menu should be destroyed to fix the leak. While I agree that something could be done here, a destroy_on_popdown or popup_once() wouldn't change anything in that case, would it?



(In reply to comment #4)
> Presently GtkMenu self-consumes its floating ref on construction by inserting
> itself into this toplevel, so gtk_menu_new() is truly transfer:none.  Making
> this change would change it to returning a floating ref which would be sunk
> when you call popup() and eventually unreffed when the menu pops down.  From a

Sorry, but I don't get that. gtk_menu_init() does call g_object_force_floating() to make sure it *does* return a floating reference, doesn't it?
So then it's not a transfer:none, and that might be a reason for some leaks: If you create a menu and only want to pop it up, you should  take a ref on it (and unref when done/on popdown), and it's for cases when no one ever took a ref on it that there's the needs_destruction_ref flag.

Which is why I think a new function popup_once() might be better than the property, since that function could then add a reference on the menu, and not call destroy() but simply unref() it when done. It should do the same in most cases (i.e. lead to the menu's destruction), but if someone/the caller had actually taken a ref on the menu, then it'll be up to them to unref it for the widget to get destroyed.

(In reply to comment #13)
> I'm not sure how the leak in gtkappchooserwidget.c is related to this, though.
> If the menu is popped up, since it was attached to a widget, it should get
> destroyed properly (when the widget will be destroyed).

Right, the menu gets destroyed when the widget is destroyed. However, a new menu is created every time the tree view is right-clicked, and the old menus all linger around without ever being used again.

> If the menu isn't popped up OTOH you have a leak, but it's unrelated since it
> wasn't ever shown/popped up. In that case the menu should be destroyed to fix
> the leak. While I agree that something could be done here, a destroy_on_popdown
> or popup_once() wouldn't change anything in that case, would it?

I don't see a leak if the menu is never popped up.

(In reply to comment #14)
> Right, the menu gets destroyed when the widget is destroyed. However, a new
> menu is created every time the tree view is right-clicked, and the old menus
> all linger around without ever being used again.

Oh, right, got it.

> > If the menu isn't popped up OTOH you have a leak, but it's unrelated since it
> > wasn't ever shown/popped up. In that case the menu should be destroyed to fix
> > the leak. While I agree that something could be done here, a destroy_on_popdown
> > or popup_once() wouldn't change anything in that case, would it?
> 
> I don't see a leak if the menu is never popped up.

Well, what happens to the newly-created GtkMenu (menu) then? It is a floating menu that no one referenced nor destroyed, isn't it?

(In reply to comment #15)
> > I don't see a leak if the menu is never popped up.
> 
> Well, what happens to the newly-created GtkMenu (menu) then? It is a floating
> menu that no one referenced nor destroyed, isn't it?

My bad, sorry. I forgot about the "if (n_children > 0)" in there and thought you meant when the click handling function is never called. You're right, that's a leak which popup_once() wouldn't help with. There would need to be an else branch that destroys the widget.

I've now fixed the GtkAppChooserWidget and GtkColorSelection to do the same thing we do elsewhere: destroy the menu. But, looking at the code some more,
I don't think this is enough to actually clean up the menu object.

As we've established by now, gtk_menu_new returns a floating object with ref count 1. But really, that refcount is a lie - gtk_menu_init 'hides' the reference held by the toplevel in the needs_destruction_ref flag - the reference gets added back when the menu is destroyed. Therefore, in the common sequence:

menu = gtk_menu_new ()
...
gtk_widget_destroy (menu)

we end up with a destroyed, floating object that has a ref count of 1.


If I just do

menu = gtk_menu_new ()
...
g_object_unref ()

I get the 'floating object was finalized' warning


This works correctly, as far as I can tell:

menu = gtk_menu_new ()
gtk_object_ref_sink (menu);
...
g_object_unref ();

(In reply to comment #17)
> menu = gtk_menu_new ()
> ...
> gtk_widget_destroy (menu)
> 
> we end up with a destroyed, floating object that has a ref count of 1.

I'm not sure what you mean by that: If the menu has been destroyed it's gone/finalized, so it's not floating anymore/there's no leak there. This actually works, doesn't it?

>This works correctly, as far as I can tell:
>
> menu = gtk_menu_new ()
> gtk_object_ref_sink (menu);
> ...
> g_object_unref ();

Yeah this is the right way to do it, I think. That's what I actually do when I create a menu I only want to popup once, ref_sink it and then connect g_object_unref to unmap-event (not sure why I use that and not hide).

I think a little helper function would be nice for such (common) cases, but I don't think it should be called popup_once(); rather it should be popup_and_destroy()

The rationale being that if it only did a ref_sink() & unref() on popdown, that might not be enough to free/destroy the menu. Specifically, it might not destroy it if e.g. it was attached to a widget, since that does a ref_sink as well.

So, I think it should be popup_and_destroy() to make it clear that, on popdown, the menu *will* be destroyed. And in the code, the function should first check if the menu is (still) floating, and if so ref_sink it, and then on popdown just call destroy no matter what.

With such a function, using it instead of popup() would fix the leak in GtkColorSelection without the need to connect to "hide" manually, so it just makes things easier for such common cases.

In GtkAppChooserWidget, I'm not sure why you kept the menu around when it's either empty/not used, and after it's been popped down. I don't think there's a need to keep a pointer to it, just ref_sink & unref it when it's not used, and when it is using this popup_and_destroy() would handle it properly (i.e. calling destroy on popdown).

(In reply to comment #18)
> (In reply to comment #17)
> > menu = gtk_menu_new ()
> > ...
> > gtk_widget_destroy (menu)
> > 
> > we end up with a destroyed, floating object that has a ref count of 1.
> 
> I'm not sure what you mean by that: If the menu has been destroyed it's
> gone/finalized, so it's not floating anymore/there's no leak there. This
> actually works, doesn't it?

You can try for yourself:


#include <gtk/gtk.h>

static void
destroyed (GtkWidget *menu)
{
  g_print ("destroyed, ref count %d, floating %d\n", G_OBJECT (menu)->ref_count, g_object_is_floating (menu));
}

int main (void)
{
  GtkWidget *menu;

  gtk_init (NULL, NULL);

  menu = gtk_menu_new ();
  g_signal_connect (menu, "destroy", G_CALLBACK (destroyed), NULL);

  g_print ("new, ref count %d, floating %d\n", G_OBJECT (menu)->ref_count, g_object_is_floating (menu));

  g_object_ref_sink (menu);

  g_print ("reffed, ref count %d, floating %d\n", G_OBJECT (menu)->ref_count, g_object_is_floating (menu));

  gtk_widget_destroy (menu);

  g_print ("afterlife, ref count %d, floating %d\n", G_OBJECT (menu)->ref_count, g_object_is_floating (menu));

  return 0;
}


This prints:

new, ref count 1, floating 1
reffed, ref count 1, floating 0
destroyed, ref count 2, floating 0
afterlife, ref count 1, floating 0

Well, in that actual case that looks normal to me: you ref_sink() the menu and never unref it, so you still own a reference on the menu.

There is, however, a bug if you don't take a reference on the menu, because then it isn't finalized either, you're right.

It looks to me that this is a bug because, in gtk_menu_destroy(), when it adds a reference (if needs_destruction_ref is TRUE) it does a ref() instead of a ref_sink().

Doing a ref_sink there will not affect things if someone already sank it, but in case no one did, which is the case causing issue I believe, it will then only sink the current ref and the menu will, then, be finalized (unless someone did ref it without sinking, in which case the menu will still have one strong ref, as it should).

Would that be right?

So what is the conclusion here?  After letting the dust settle and coming back to look at it I think I like _popup_once() best.

Created attachment 266485 [details] [review]
menu: Fix possible leak on destroy

Upon creation menus are forced floating, hiding the reference took on
gtk_menu_init() by the toplevel. When re-adding that reference on destroy
we need to ref_sink() it in case no one took a reference, else we end up
with a reference left, and a leak.

Created attachment 266486 [details] [review]
menu: Add helper gtk_menu_popup_and_destroy()

Often one will create a menu in order to pop it up once and be done with
it. In such cases, they'd need not to forget to destroy it on popdown
to avoid a leak.

This helper makes it convenient to do so, which can also serves as a
reminder that menus used with gtk_menu_popup() needs to either be
manually destroyed, or ref_sink/unref-ed to avoid leaks.

Review of attachment 266485 [details] [review]:

I don't like this patch very much and I think it may introduce an API incompatibility.  I'd almost prefer we just removed the forced-floating part.  The menu is something vaguely like a toplevel and we have a precedent for the idea that we don't get a floating ref returned, but rather a ref held by Gtk.  In a sense, a floating ref returned to the user is kinda the same as 'no ref' returned to the user (ie: only Gtk owns it).

Otherwise, I'd assume that (like all widgets) I get a floating ref and it is my responsibility to sink it by inserting it into something else.  Of course, I'm not going to be able to do that with a menu....

(In reply to comment #24)
> Review of attachment 266485 [details] [review]:
> 
> I don't like this patch very much and I think it may introduce an API
> incompatibility.  I'd almost prefer we just removed the forced-floating part. 

What API incompatibility would it introduce? Everything should work as before, except without the leak that would occur when one didn't make sure to handle the reference. Specifically, currently if you create a menu only to pop it up once & that's it, you need to ref_sink it and unref it when done (e.g. on hide), else you have a leak (even if you call gtk_widget_destroy on it).
This makes sure there's no leak, and also doesn't require to ref_sink/unref the menu.

> The menu is something vaguely like a toplevel and we have a precedent for the
> idea that we don't get a floating ref returned, but rather a ref held by Gtk. 

I agree though, that it would be better/more consistent to instead drop the force_floating and make menus behave like GtkWindow: we get a widget with one (strong) reference, owned by GTK.

Slightly off-topic maybe, but: a new GtkWindow is a widget that isn't floating, which I believe is what you're referring to. Is that fact even documented anywhere? (it is mentionned on the FAQ, but not anywhere on GtkWindow, is it?)

> In a sense, a floating ref returned to the user is kinda the same as 'no ref'
> returned to the user (ie: only Gtk owns it).
> 
> Otherwise, I'd assume that (like all widgets) I get a floating ref and it is my
> responsibility to sink it by inserting it into something else.  Of course, I'm
> not going to be able to do that with a menu....

Note that that's how it works currently: if you don't ref_sink/unref your menu (or do so indirectly, e.g. attaching it to a widget) like any other widget, but just pop it up & destroy it, you have a leak.

Created attachment 266690 [details] [review]
menu: Fix possible leak on destroy

Upon creation menus were forced floating, hiding the reference took on
gtk_menu_init() by the toplevel. This could lead to leaks when the menu wasn't
ref_sink/unref-d and e.g. simply popped up & destroyed.

Instead, much like GtkWindow-s, menus are now simply returned with a strong
reference owned by GTK.

Is this likely to get in?

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/457.