GNOME Bugzilla – Bug 729820
GMenuModel submenu-action set 'false' before item activation
Last modified: 2014-06-16 20:10:14 UTC
Created attachment 276176 [details] Test that crashes because activation happens after hide GtkMenuItem "activate" is emitted after its parent shell's "hide", which is an issue if the application makes changes to that shell in response to a "hide". For example, an application might build its menus lazily, adding items when about to show, and removing them on hide. In this case, the menu item would be finalized before activation occurs. The activation should occur while the parent menu is still in a shown state, before the menu is hidden.
Created attachment 276193 [details] Test that crashes because activation happens after hide
Created attachment 276194 [details] [review] Activate menu item before closing its parent shell
A bit of background to this issue: we see this problem with GtkMenuTracker and people who dynamically generate menu context. By the time the action gets invoked, the menu has already been destroyed. gedit was having trouble with this and their stack-switcher menu, for example. They were able to solve it by handling the hide via idle (which meant that the menu item was still around at the time of the activation). It gets trickier with D-Bus gets involved with submenu actions, however. In this case it's not possible to use the idle trick because the second D-Bus message may still be on the wire. The thought comes to mind to use a timeout, but that's obviously a pretty nasty hack. There are three ways that this problem can be fixed: - fix GtkMenu directly - fix the menu tracker to attach the activate signal in such a way that the action gets activated before the menu close signal fires - fix the submenu-action handling in the tracker so that the to-close transition is always handled from an idle I think it's probably best to fix GtkMenu directly, unless we find that it introduces substantial compatibility issues with existing code.
So... doing the activate before the hide can be a bad thing in the case that the activate handler does something like gtk_dialog_run () on a modal GtkDialog. In that case, the menu remains open until losing focus. But even worse is the case where the activate handler does anything blocking or resource intensive, because the menu retains the grab on the keyboard and pointer.
Review of attachment 276194 [details] [review]: We can't do this because an activate handler might block and the menu retains a grab on the keyboard and pointer until hidden.
Created attachment 276258 [details] [review] Defer submenu-action state change to an idle
I agree: changing the order of activate and hide is out of the question, because the hide takes down all the grabs, and you'd break all the worlds activate handlers that don't expect the grab to be around.
yet another alternative might be to add another signal to use for the tracker, instead of hide - and guarantee that it always gets emitted after the hide and the activate, and after the grab is gone.
Created attachment 276545 [details] [review] Change submenu-action state after activate
Comment on attachment 276258 [details] [review] Defer submenu-action state change to an idle This patch is working much better than the one using the selection-done signal.
Created attachment 276558 [details] [review] Change submenu-action state in an idle Updating the commit message to provide detail.
I don't like the idle approach. I'd be happier if we could avoid it
What is specifically bad about it? I'm kinda fond of it because it involves less digging inside the guts of GtkMenu... it's also only used for menuitems with a submenu-action... which is not a lot of them.
But we are inside gtk - what I hate is workarounds in one side of the code base for issues in the other side. We had and still have far too much of that in the a11y code, and it generally makes debugging hard, breaks in corner cases, or has unintended side effect.
Created attachment 276629 [details] [review] Change submenu-action state after activate We must add a new signal since selection-done doesn't cover all cases of hide sufficiently...
Review of attachment 276629 [details] [review]: ::: gtk/gtkmenushell.c @@ +610,3 @@ + menu_shell = GTK_MENU_SHELL (widget); + menu_shell->priv->should_close = TRUE; +} How is should_close different from !visible ? @@ +613,3 @@ + +void +_gtk_menu_shell_check_can_close (GtkMenuShell *menu_shell) I'd rather call this _gtk_menu_shell_maybe_emit_close or something like that, to indicate that this may emit a signal - check in the name makes me expect a boolean return
Review of attachment 276629 [details] [review]: ::: gtk/gtkmenushell.c @@ +610,3 @@ + menu_shell = GTK_MENU_SHELL (widget); + menu_shell->priv->should_close = TRUE; +} You're right that these should probably be called should_emit_close and can_emit_close instead. We're using these to prevent any possibility of emitting the close signal multiple times for a single hide. Once we emit the close, we reset should_close back to the default state of FALSE. Renaming all of these things will hopefully make it clearer...
Created attachment 276678 [details] [review] Change submenu-action state after activate Improve the naming for clarity
Review of attachment 276678 [details] [review]: Didn't check the validity of the approach in general, but one very large problem noticed: ::: gtk/gtkmenushell.h @@ +64,2 @@ void (*deactivate) (GtkMenuShell *menu_shell); + void (*close) (GtkMenuShell *menu_shell); you can't add new functions to the vtable like this -- it changes the ABI. you need to add this to the end, and remove one of the "reserved" slots.
Created attachment 276891 [details] [review] Change submenu-action state after activate Preserve ABI by using one of the reserved vfunc slots.
going back to the original bug description, I don't understand why the menuitem or its shell would get finalized prematurely - gtk_menu_shell_activate_item is careful to take references on both for the duration.
The problem is when people dynamically construct menus in response to menus being popped up and destroy them again when the menu is torn down.
Created attachment 278550 [details] [review] menu binding: emit submenu close after activate We want to make sure that the submenu action is changed back to FALSE _after_ the menu item has been activated. This prevents the menu teardown handler from deleting the menu item before it can be activated. Unfortunately, GtkMenuShell emits "hide" before the item activation. This is probably done to prevent the application from doing things like showing dialogs when the menu is still holding the grab. In the case where we are doing an activate, set a boolean flag on each of the open menus (following the parent stack) indicating that we'll be emitting another signal soon (selection done). If that flag is set, we defer the setting of the submenu action until we receive the second signal.
Review of attachment 278550 [details] [review]: If it solves your problem, I'm fine with it. ::: gtk/gtkmenushellprivate.h @@ +66,3 @@ + * signal is coming soon (when checked + * from inside of a "hide" handler). + */ The name is a bit misleading to me - it has to be read as an instruction to the menutracker, right ? ('defer hiding this menu to the selection-done signal handler'). Might be clearer to make it a bit more descriptive, like the comment. Maybe selection_done_coming_soon ? Anyway, nitpicks
Attachment 278550 [details] pushed as b532e1f - menu binding: emit submenu close after activate
Thanks, this does fix the problem for me!