After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 339791 - Gtk::Menu crash with child
Gtk::Menu crash with child
Status: RESOLVED FIXED
Product: gtkmm
Classification: Bindings
Component: general
2.8.x
Other Linux
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2006-04-26 09:43 UTC by Murray Cumming
Modified: 2007-04-16 13:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
menucrash2.cc (1.51 KB, text/plain)
2006-04-26 09:45 UTC, Murray Cumming
  Details
Slightly altered test case. (2.19 KB, text/plain)
2006-05-17 10:08 UTC, Paul Davis
  Details
Patch for this bug (6.16 KB, patch)
2006-05-17 10:11 UTC, Paul Davis
none Details | Review
New test case. (2.49 KB, text/plain)
2006-05-20 20:23 UTC, Paul Davis
  Details
Plain C test case (1.15 KB, text/x-csrc)
2007-04-06 19:39 UTC, Johannes Schmid
  Details
Patch for the issue (1.51 KB, patch)
2007-04-11 22:27 UTC, Johannes Schmid
needs-work Details | Review
Patch to add a new special case for Gtk::Menu to Widget_Class::dispose_vfunc_callback() (1.44 KB, patch)
2007-04-12 12:17 UTC, Johannes Schmid
committed Details | Review

Description Murray Cumming 2006-04-26 09:43:38 UTC
See the test case.
Comment 1 Murray Cumming 2006-04-26 09:45:45 UTC
Created attachment 64305 [details]
menucrash2.cc

This test case is a version of one that was originally in bug #104194.

It shows this output on std::out:

debug: before deleting parent.
destructor for menu: parent
debug: after deleting parent.
debug: before deleting child.
destructor for menu: child

(a.out:2140): GLib-GObject-WARNING **: instance of invalid non-instantiatable type `(null)'

(a.out:2140): GLib-GObject-CRITICAL **: g_signal_handlers_disconnect_matched: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

(a.out:2140): Gtk-CRITICAL **: gtk_menu_item_detacher: assertion `GTK_IS_MENU_ITEM (widget)' failed

(a.out:2140): GLib-GObject-WARNING **: invalid uninstantiatable type `(null)' in cast to `GObject'

(a.out:2140): GLib-GObject-CRITICAL **: g_object_steal_data: assertion `G_IS_OBJECT (object)' failed

(a.out:2140): GLib-GObject-WARNING **: invalid uninstantiatable type `(null)' in cast to `GObject'

(a.out:2140): GLib-GObject-CRITICAL **: g_object_set_data: assertion `G_IS_OBJECT (object)' failed
debug: after deleting child.
Comment 2 Murray Cumming 2006-04-26 09:50:17 UTC
Here is the gdb backtrace with --g-fatal-warnings. valgrind's memcheck doesn't show any double-deletion, but valgrind might not work properly with the latest GTK+. Andrew, maybe you could try valgrind on your system.

Program received signal SIGTRAP, Trace/breakpoint trap.
IA__g_logv (log_domain=<value optimized out>, log_level=G_LOG_LEVEL_WARNING, format=0xb770027c "instance of invalid non-instantiatable type `%s'",
    args1=0xbfb2808c "") at gmessages.c:503
503               g_private_set (g_log_depth, GUINT_TO_POINTER (depth));
(gdb) bt
  • #0 IA__g_logv
    at gmessages.c line 503
  • #1 IA__g_log
    at gmessages.c line 517
  • #2 IA__g_type_check_instance
    at gtype.c line 3216
  • #3 IA__g_signal_handlers_disconnect_matched
    at gsignal.c line 1926
  • #4 IA__gtk_menu_detach
    at gtkmenu.c line 1121
  • #5 gtk_menu_destroy
    at gtkmenu.c line 949
  • #6 Gtk::Container_Class::destroy_callback
    at container.cc line 242
  • #7 IA__g_cclosure_marshal_VOID__VOID
    at gmarshal.c line 77
  • #8 g_type_class_meta_marshal
    at gclosure.c line 567
  • #9 IA__g_closure_invoke
    at gclosure.c line 490
  • #10 signal_emit_unlocked_R
    at gsignal.c line 2554
  • #11 IA__g_signal_emit_valist
    at gsignal.c line 2197
  • #12 IA__g_signal_emit
    at gsignal.c line 2241
  • #13 gtk_object_dispose
    at gtkobject.c line 418
  • #14 gtk_widget_dispose
    at gtkwidget.c line 6725
  • #15 Gtk::Widget_Class::dispose_vfunc_callback
    at widget.cc line 509
  • #16 IA__g_object_unref
    at gobject.c line 1734
  • #17 Gtk::Object::_destroy_c_instance
    at object.cc line 135
  • #18 Gtk::Object::destroy_
    at object.cc line 265
  • #19 ~Menu
    at menu.cc line 175
  • #20 MyMenu::~MyMenu
  • #21 MyBar::~MyBar
  • #22 main

Comment 3 Andrew E. Makeev 2006-04-27 07:38:46 UTC
Here is first part of valgrind output. Fedore Core 4, GTKMM-2.4, GTK+-2.4
Guess, it is obvious that child was deleted with gtk_container_foreach().

valgrind --tool=memcheck --num-callers=100 menutest
==8677== Memcheck, a memory error detector for x86-linux.
==8677== Copyright (C) 2002-2005, and GNU GPL'd, by Julian Seward et al.
==8677== Using valgrind-2.4.0, a program supervision framework for x86-linux.
==8677== Copyright (C) 2000-2005, and GNU GPL'd, by Julian Seward et al.
==8677== For more details, rerun with: -v
==8677==
debug: before deleting parent.
destructor for menu: parent
debug: after deleting parent.
debug: before deleting child.
destructor for menu: child
==8677== Invalid read of size 4
==8677==    at 0x2BDC3B: g_type_check_instance (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2B9CF8: g_signal_handlers_disconnect_matched (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x526415: gtk_menu_detach (in /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x527A21: (within /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x1BA6A6EB: Gtk::Container_Class::destroy_callback(_GtkObject*) (container.cc:160)
==8677==    by 0x2B17E6: g_cclosure_marshal_VOID__VOID (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2A5D9A: (within /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2A6284: g_closure_invoke (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2B4C65: (within /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2B5EAF: g_signal_emit_valist (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2B6222: g_signal_emit (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x53D1F0: (within /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x600614: (within /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x1BAE42C4: Gtk::Widget_Class::dispose_vfunc_callback(_GObject*) (widget.cc:508)
==8677==    by 0x2A7C53: g_object_unref (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x1BA96730: Gtk::Object::_destroy_c_instance() (object.cc:134)
==8677==    by 0x1BA9680E: Gtk::Object::destroy_() (object.cc:264)
==8677==    by 0x1BA8D262: Gtk::Menu::~Menu() (menu.cc:201)
==8677==    by 0x804C62F: MyMenu::~MyMenu() (menucrash2.cc:15)
==8677==    by 0x804C915: MyBar::~MyBar() (menucrash2.cc:44)
==8677==    by 0x804C08F: main (menucrash2.cc:82)
==8677==  Address 0x1C033D70 is 0 bytes inside a block of size 96 free'd
==8677==    at 0x1B909743: free (vg_replace_malloc.c:152)
==8677==    by 0x314BC23: g_free (in /usr/lib/libglib-2.0.so.0.600.6)
==8677==    by 0x2C2ADC: g_type_free_instance (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2A7C49: g_object_unref (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2A7CBB: g_object_run_dispose (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x53D1AC: gtk_object_destroy (in /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x5FB99D: gtk_widget_destroy (in /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x52F312: (within /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x1BA6AAFC: Gtk::Container_Class::forall_vfunc_callback(_GtkContainer*, int, void (*)(_GtkWidget*, void*), void*) (container.cc:396)
==8677==    by 0x49E3ED: gtk_container_foreach (in /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x4A0150: (within /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x527A90: (within /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x1BA6A6EB: Gtk::Container_Class::destroy_callback(_GtkObject*) (container.cc:160)
==8677==    by 0x2B17E6: g_cclosure_marshal_VOID__VOID (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2A5D9A: (within /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2A6284: g_closure_invoke (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2B4C65: (within /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2B5EAF: g_signal_emit_valist (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x2B6222: g_signal_emit (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x53D1F0: (within /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x600614: (within /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x1BAE42C4: Gtk::Widget_Class::dispose_vfunc_callback(_GObject*) (widget.cc:508)
==8677==    by 0x2A7CB3: g_object_run_dispose (in /usr/lib/libgobject-2.0.so.0.600.6)
==8677==    by 0x53D1AC: gtk_object_destroy (in /usr/lib/libgtk-x11-2.0.so.0.600.10)
==8677==    by 0x1BA966ED: Gtk::Object::_destroy_c_instance() (object.cc:166)
==8677==    by 0x1BA9680E: Gtk::Object::destroy_() (object.cc:264)
==8677==    by 0x1BA8D262: Gtk::Menu::~Menu() (menu.cc:201)
==8677==    by 0x804C62F: MyMenu::~MyMenu() (menucrash2.cc:15)
==8677==    by 0x804C8A3: MyBar::~MyBar() (menucrash2.cc:40)
==8677==    by 0x804C08F: main (menucrash2.cc:82)

(menutest:8677): GLib-GObject-WARNING **: instance with invalid (NULL) class pointer

(menutest:8677): GLib-GObject-CRITICAL **: g_signal_handlers_disconnect_matched: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed
Comment 4 Dodji Seketeli 2006-05-08 13:59:21 UTC
(In reply to comment #3)
> Here is first part of valgrind output. Fedore Core 4, GTKMM-2.4, GTK+-2.4
> Guess, it is obvious that child was deleted with gtk_container_foreach().
> 
Hmmh, I am not quite sure.
Actually deleting the parent menu does not delete the child menu.
The problem arises when the test code does 'delete m_child' ;

After a couple of gdb-ing,  here is what I think happens during delete m_child:
delete m_child somehow calls gtk_menu_destroy() on the child menu.
At this point, everything is fine.
Then, the gtk_menu_destroy() calls gtk_menu_detach() to apparently detach
the child from it parent ... bad thing.

Remember, the parent has already been destroyed ...

It seems GtkMenu has not been designed in a way that allow a parent menu
to be destroyed *before* its children. I am not sure this is a bug from
Gtkmm then.

To reproduce what I saw in gdb, try to run the sample code in
gdb and put a break point in gtk_menu_destroy and gtk_menu_detach.

Cheers

Dodji.



Comment 5 Andrew E. Makeev 2006-05-10 07:00:07 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Here is first part of valgrind output. Fedore Core 4, GTKMM-2.4, GTK+-2.4
> > Guess, it is obvious that child was deleted with gtk_container_foreach().
> > 
> Hmmh, I am not quite sure.
> Actually deleting the parent menu does not delete the child menu.
> The problem arises when the test code does 'delete m_child' ;
> 
> After a couple of gdb-ing,  here is what I think happens during delete m_child:
> delete m_child somehow calls gtk_menu_destroy() on the child menu.
> At this point, everything is fine.
> Then, the gtk_menu_destroy() calls gtk_menu_detach() to apparently detach
> the child from it parent ... bad thing.
> 
> Remember, the parent has already been destroyed ...
> 
> It seems GtkMenu has not been designed in a way that allow a parent menu
> to be destroyed *before* its children. I am not sure this is a bug from
> Gtkmm then.
> 

ok, then, If we aren't going to handle this with gtkmm functionality what should be done?
1. Make a note in documentation about order of menu child destructors.
2. Assign issue to GTK to let them check and fix it.

I am not sure that 2nd way even has a place here, because GTK programmers never will go that way of menu object destruction as it is possible in GTKMM.

Regards,
-andrew
Comment 6 Murray Cumming 2006-05-10 07:15:37 UTC
If this is indeed the problem (I have not yet investigated) then we would want to either 
a) Fix GTK+
b) Do some kind of workaround in gtkmm.

Simply documenting a problem would be the least desirable solution.
Comment 7 Paul Davis 2006-05-17 10:08:19 UTC
Created attachment 65656 [details]
Slightly altered test case.

I altered the test case for this bug slightly.

#1 I added a managed widget to the mix to make sure my changes weren't preventing managed widgets from being deleted properly.

#2 I added a second un-manage()ed widget to the scheme to make sure my changes wouldn't choke on something that wasn't a menu.
Comment 8 Paul Davis 2006-05-17 10:11:09 UTC
Created attachment 65657 [details] [review]
Patch for this bug

The patch I came up with for this bug.

Changes the Gtk::Menu_Helpers::MenuElem destructor to a custom implementation.

Adds a simple check, if( this->has_submenu() ) this->remove_submenu() ;

And thats that.

I think it took me longer to get gtkmm built from source than to figure out the patch.
Comment 9 Murray Cumming 2006-05-17 10:52:07 UTC
Sounds sensible, though I'd rather know why the submenu doesn't get informed of the death of its parent, and most sub-widgets do. Maybe it's a timing/sequence thing. At the least, we need a comment explaining why we do the remove, even if it's just the URL of bug.

I think we have a similar solution/hack for Container (maybe not any more). Maybe submenus are nore listed like regular children.
Comment 10 Paul Davis 2006-05-17 11:20:25 UTC
A little more probing reveals:

Submenus seem to be unconditionally destroyed. Thus negating all existance. Or at least some pretty big assumptions.

check out: gtk+/gtk/gtkmenuitem.c
Line: 296ish in gtk_menu_destroy

  if (menu_item->submenu)
    gtk_widget_destroy (menu_item->submenu);

It would appear to be deleting the C object out from under the C++ object. Then when we delete our Menu, it tries to detach itself, but gets caught up by having a dead C object.

Granted, I could be out in left field here too.  I'm assuming that gtk_menu_item_destroy is being called when a Gtk::MenuItem is destroyed.

And gtk_menu_item_destroy appears to be an internal API and I can't find any docs on whether this is expected behavior or not, so I'm not entirely sure whether to file this as a bug or pat someone on the back for writing it correctly.
Comment 11 Murray Cumming 2006-05-17 11:25:53 UTC
> Submenus seem to be unconditionally destroyed

This is also true of GtkContainer children in general. However, we prevent the destruction from actually happening. I think it's in some *dispose* vfunc/callback somewhere. Maybe it's not working properly for the sub menu.
Comment 12 Murray Cumming 2006-05-17 11:29:37 UTC
I think the relevant part might be:
"
void Widget_Class::dispose_vfunc_callback(GObject* self)
{
...
      // Remove the widget from its parent container so that it
      // won't be destroyed later by gtk_container_destroy().
...
"

But maybe there's something else too. We changed how we did this at least once, so I get confused.
Comment 13 Paul Davis 2006-05-17 12:14:19 UTC
Aha!  I think....

Gtk::MenuItem is a container. Duh.  What MenuItem doesn't have is a custom remove() method that removes the reference to its submenu.

I've added some g_warnings to track this down.  In my custom destructor, we're still trying to destroy the submen of the unmanaged widget.

But the problem is,

void Gtk::Bin::remove( Widget& w ) ;

is not virtual, so isn't it bad to override that?

I saw virual void Gtk::Bin::on_remove( Widget& w ) ;

But I can't find an example else where on how to implement this with all this crazy m4 stuff.

Comment 14 Murray Cumming 2006-05-17 12:27:56 UTC
> Gtk::MenuItem is a container

Yeah, but Gtk::Menu isn't a Gtk::MenuItem. I think it's more relevant that Gtk::Menu is a Gtk::Container. I don't see Gtk::Bin anywhere in the hierarchy.

> What MenuItem doesn't have is a custom remove() method that removes the reference to its submenu.

I'd expect GtkMenu to take care of making gtk_container_remove(submenu) actually remove the sub menu, possibly via some vfunc. Maybe it uses the GtkContainer::"remove"signal, but that seems odd.
Comment 15 Murray Cumming 2006-05-17 12:28:44 UTC
By gtk_container_remove(submenu), I mean gtk_container_remove(menu, submenu).
Comment 16 Paul Davis 2006-05-17 13:13:55 UTC
Another aha!

gtkmm/gtk/gtkmm/menu_elems.cc:93-96
MenuElem::MenuElem(const Glib::ustring& label, Menu& submenu)
{
  set_child( manage(new MenuItem(label, true)) );
  child_->set_submenu(submenu);
  child_->show();
}

We're talking about containers, but we're not in a container.  Or rather, this is not a container interface we're dealing with.

In Gtk::MenuItem, the submenu is stored as a member variable.  Its not stored with a container interface where we can just remove() it, we have to call remove_submenu() to get rid of the reference before we destroy it.

The Widget_Class::dispose_vfunc_callback would remove the Gtk::Label if it happened to be unmanaged, but not touch the submenu.

Now the question is, why does our managed menu get destroyed properly?

And I cannot answer this.  All I can say is that the MenuItem destructor never seems to get run.  Which I find odd.  But from my g_warning outputs it doesn't appear to be running.





Comment 17 Murray Cumming 2006-05-17 13:26:29 UTC
You've certainly given me some stuff to investigate. If you are right about the problem, it looks like we can find a solution.
Comment 18 Paul Davis 2006-05-17 14:31:21 UTC
Oh, and the hiererarchy as best as I can tell for the unamanged widget:

Gtk::Window
  -> Gtk::MenuBar
    -> Gtk::Menu ( m_parent )
      -> Gtk::MenuElem
        -> Gtk::MenuItem ( member of MenuElem )

          //This is a member variable, not a container relationship.
          //I'm pretty sure there's quite a difference, but I might be wrong.
          -> Gtk::Menu ( m_child1 )

Well, crap. Looking at this makes me think I'm wrong again.  Damn it.
(This is a run of my test case.  Kind of. I dunno, but its got a g_warning in
Widget_Class::dispose_vfunc_callback().  I copied one of the DEBUG_REFCOUNT
warning message things to right below if( obj->referenced_ ) to see what
pointer was being removed from what container.  And the pointer for the First
Child is definitely being removed from something.

Athough, notice it then gets removed twice from nothing.  Well I suppose those
could be all the weird call back signals or what not.

First Child: 0x80b4050
Second Child: 0x80b4150
Parent: 0x80b4250

BEFORE DELETE PARENT
destructor for menu: parent
(test:9256): gtkmm-WARNING **: REMOVING AN UNMANAGED WIDGET gtkmm__GtkMenu


This line says we are in fact removing the Menu from some goddamn container.

(test:9256): gtkmm-WARNING **: Widget_Class::dispose_vfunc_callback(): removing
gobject_: 0x80b4050 from parent: 0x80b6800


(test:9256): gtkmm-WARNING **: REMOVING AN UNMANAGED WIDGET gtkmm__GtkMenu
(test:9256): gtkmm-WARNING **: Widget_Class::dispose_vfunc_callback(): removing
gobject_: 0x80b4050 from parent: (nil)
(test:9256): gtkmm-WARNING **: Destroying object: 0x80b8468
(test:9256): gtkmm-WARNING **: Refcount: 1
(test:9256): gtkmm-WARNING **: Would've destroyed submenu: 0x80b4050
(test:9256): gtkmm-WARNING **: REMOVING AN UNMANAGED WIDGET gtkmm__GtkMenu
(test:9256): gtkmm-WARNING **: Widget_Class::dispose_vfunc_callback(): removing
gobject_: 0x80b4050 from parent: (nil)
destructor for menu: Second child
(test:9256): gtkmm-WARNING **: Destroying object: 0x80b84c8
AFTER DELETE PARENT

...

So I did some more poking, and I'm finding that when m_child1 is removed from
the container, it never detaches itself.

I think I might have another line of inquiry. I wonder if when a Menu is
removed from a container, it should be detached.

So, judging by some quick reading of the code, this could be done by listening
to on_parent_change and if the new parent is NULL, detach.

Tried this quick by overriding on_parent_changed in Gtk::Menu.  Didn't work. 
Doesn't look like it got called. :(

There is something to this though I do believe.  gtk_menu_detach is failing
because its being called after the MenuItem is destroyed

Judging from this:
(test:27650): Gtk-CRITICAL **: gtk_menu_item_detacher: assertion
`GTK_IS_MENU_ITEM (widget)' failed

Which was part of the original output of the error program ( before my attempt
at a patch )

So, the issue here is that when Gtk::Menu is removed from its container, it
should figure out to detach itself somehow.  I could check this real quick to
see if it happens for any container or not.

If this happens for any container, my patch is dead.  And if it happens from any container, Gtk::Menu is going to have to detect when its been removed, which as far as I can tell is by on_parent_changed, cause thats all I see being called in the Bin::remove( Widget& w ) and Box::remove( Widget& w ) (just two random ones with different implementations for sampling).

But my mind is absolutely fried, so this is it for tonight, today, whatever it
is now.

Hope that helps.

Comment 19 Paul Davis 2006-05-20 20:23:02 UTC
Created attachment 65915 [details]
New test case.

Adding the method below to the class derived from Gtk::Menu fixes the problem.

virtual void
on_parent_changed( Gtk::Widget* old_parent )
{
    fprintf( stderr, "Parent changed.\n" ) ;
    detach() ;
}
Comment 20 Murray Cumming 2006-05-28 19:24:21 UTC
That's a neat solution, and it makes sense. But maybe it should be done in GTK+ instead of gtkmm.

Also, overrides of default signal handlers should usually call the base class implementation, or document why they choose not to.
Comment 21 Murray Cumming 2006-06-11 17:18:46 UTC
So, while trying to make a C test case, I am wondering 
1. Where do we attach() the menu item?
2. What does attach/detach mean for a menu item?

Comment 22 Murray Cumming 2006-12-28 22:18:59 UTC
I still hope that someone finds the time to figure this out before I get to it.
Comment 23 Johannes Schmid 2007-04-06 19:39:47 UTC
Created attachment 85921 [details]
Plain C test case

This is a C test case for the problem. Destroying the child after the parent will result in a Gtk-Critical unless we ref() the child. Looks like this would be the expected behaviour for Gtk+.

=> Paul patch looks like a reasonable solution because the case is equal to the GtkContainer special case IMHO.
Comment 24 Johannes Schmid 2007-04-10 09:44:55 UTC
OK, after two hours of debugging I think I know what happens:

The child_submenu is destroyed because gtk_menu_item_destroy destroys it unconditionally as Paul already pointed out.
The menuitem is destroyed because the GtkContainer (parent of the menushell/menu) is destroyed.
The GtkMenuShell uses the GtkContainer interface to manage its GtkMenuItems and thus all GtkMenuItems part of a Menu are destroyed once the GtkMenu is destroyed and the MenuItems destroy their submenus.

The questions:
So, the submenu is dead before we call "delete m_child"! So either we prevent Gtk::MenuItem from destroying its submenu or we have to document this somehow.  

Of course we can, as Paul already showed, detach the submenu before the Gtk::MenuItem is destroyed but what about the 99% cases were we would want that the submenu is destroyed with the parent?
Comment 25 Paul Davis 2007-04-10 15:00:08 UTC
Its been quite a while, but I thought calling detach() would take care of automatically deleting when the submenu was managed.

Let me iterate its been a long time since I looked at this though.
Comment 26 Johannes Schmid 2007-04-11 22:27:06 UTC
Created attachment 86205 [details] [review]
Patch for the issue

This patch is based on Paul's work and uses the on_parent_changed()-vfunc of Gtk::Menu to ensure the Menu is not destroyed together with it's MenuItem.

I tested this will all testcases that are available here and also checked if it works with Gtk::manage(). Everything worked as expected so I think this solution would be OK. As I explained earlier I don't think that changing the behaviour in Gtk+ would make sense.
Comment 27 Paul Davis 2007-04-12 00:22:59 UTC
There's something not right here...

I've spent a couple hours reading up on what was actually causing this bug. If I have everything right in my head, the bug can be boiled down to this:

GtkMenuItem unconditionally destroy's its submenu.

Have we brought this up on the Gtk list to see what their response was? My feeling is that the submenu in GtkMenuItem should be treated like a container/child relationship, but the way the Gtk code is written, it appears to be more of a sole owner relationship.

The more I think about it the more I dislike the Gtk::Menu::on_parent_changed() patch. It would seem to me that we're implicitly assuming that the parent will always be a Gtk::MenuItem. I can't point to a specific test case, but it seems like this assumption could bite us in the ass when its invalid.

First things first, I think we should bring this up on the Gtk list and see what they have to say.  Maybe its a bug and it'll get fixed there.

If its expected behavior, then I'm thinking that we should be looking at a patch for the Gtk::MenuItem destructor, which could be a call to submenu->detach() or something.

Anyway, those are my thoughts for right now.
Comment 28 Johannes Schmid 2007-04-12 07:59:01 UTC
Hi Paul!

Yes, I see your point! I have written a mail to gtk-devel-list (CC'd you), we will see what they say.

A menu can be attached to any widget but I assume no widget other than GtkMenuItem destroys the menu.
Comment 29 Johannes Schmid 2007-04-12 12:17:36 UTC
Created attachment 86232 [details] [review]
Patch to add a new special case for Gtk::Menu to Widget_Class::dispose_vfunc_callback()

Tim Janik pointed out on gtk-devel-list that the behaviour in Gtk+ is correct and consistent. At least it works like GtkContainer works but with the small difference that gtk_menu_attach_to_widget cannot set widget->parent to the widget it is attached to because it needs a GtkWindow as parent to be able to popup.

So we need a new special case in Widget_Class::dispose_vfunc_callback to be able to catch the Gtk::Menu case. I decided to use Widget_Class and not Menu_Class because
a) I did not get it working correctly with Menu_Class
b) more important: Using Menu_Class might break ABI
Comment 30 Paul Davis 2007-04-12 17:05:18 UTC
else if (GTK_IS_MENU(pWidget) && gtk_menu_get_attach_widget(GTK_MENU(pWidget)) != NULL)

Shouldn't we make this:

if( GTK_IS_MENU( pWidget ) && GTK_IS_MENU_ITEM( gtk_menu_get_attach_widget( pWidget ) ) )


Granted I have no idea how robust GTK_IS_MENU_ITEM is to passing in NULL or garbage pointers, but again, it seems like we should be testing for the specific case of being attached to a MenuItem.

Also, in the source comment, you might want to add a line to the effect of:

Gtk::Menu does not use a parent widget because it must be contained in its Gtk::Window so that it can be displayed as a Popup.

Other than that, I'm satisified.
Comment 31 Johannes Schmid 2007-04-13 07:25:44 UTC
The question is what other widgets do with their menus. Usually they should also destroy them for consistence with other GtkContainer-like widgets but I am not sure that they do. Will it harm if we detach the menu before it is destroyed?
Comment 32 Paul Davis 2007-04-13 21:43:57 UTC
I did a quick google through the gtk sources to see if there were any other places where submenu's were getting destroyed.

grep "gtk_widget_destroy.*(.*submenu.*)" /usr/local/src/gnome/gtk+/gtk/*

And it only pulled up the line from gtkmenuitem.c so I think this is at least some evidence that its the only place to worry about for now.

I'm not certain if it'd harm anything or not. I do know that we know that we want to detach non-managed widgets from menuitems, and without any idea on what happens anywhere else, I would feel most comfortable making this patch as specific to this case as possible.  Then if we find in the future that there are other corner casese, we can relax this patch to include those cases as well.
Comment 33 Murray Cumming 2007-04-15 11:55:32 UTC
This looks like the ideal solution. I am now much happier that we understand the problem and solution properly. Please apply the patch to svn trunk. Well done all.

Paul's specialization sounds wise. And I'd like his extra comments to be in the source.

As a matter of style, I'd prefer to remove the unnecessary "!= NULL".
Comment 34 Johannes Schmid 2007-04-16 13:17:59 UTC
Committed modified version of the patch - Closing the bug report.