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 792624 - Critical due to gtk_widget_reset_controllers() passing NULL to gtk_event_controller_reset()
Critical due to gtk_widget_reset_controllers() passing NULL to gtk_event_cont...
Product: gtk+
Classification: Platform
Component: Widget: Other
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
Depends on:
Reported: 2018-01-17 23:29 UTC by Daniel Boles
Modified: 2018-01-22 19:36 UTC
See Also:
GNOME target: ---
GNOME version: ---

Widget: Don’t call reset() on NULL EventController (2.10 KB, patch)
2018-01-18 01:34 UTC, Daniel Boles
committed Details | Review

Description Daniel Boles 2018-01-17 23:29:18 UTC
I'm getting a critical when Scales on which I have GestureMultiPresses are destroyed, using gtkmm. The Gesture gets destroyed right before the Scale does. Then the Scale is unparented, which leads to propagate_state_flags(), which - only if the widget is insensitive, for some reason? - leads to a call of gtk_widget_reset_controllers().

It's hard to write an MCVE for this, and probably depends on various factors in my program and chosen binding - but that doesn't matter, because the cause and bug therein are clear:

 * gtk_event_controller_dispose() calls _gtk_widget_remove_controller(self)
 * which sets the Controller* in the ControllerData to NULL
 * but doesn't remove the Controller from the widget's GList of ControllerData
 * (despite the fact that _add_controller() does - never a good sign)
 * so when the widget is unparented, and state propagated, the call to gtk_widget_reset_controllers() loops over the same list, which now includes a useless ControllerData entry for the NULLed-out Gesture
 * and passes it to gtk_event_controller_reset(), which chokes on the NULL

I guess whoever wrote the GtkWidget side of this thought it was faster to just g_list_free_full() after calling _remove_controller() to do the rest of the cleanup. The problem then is that no one thought about other callers of _gtk_widget_remove_controller(), such as GtkEventController itself, and how they inadvertently leave a useless leftover node in the list to cause trouble later.

_remove_controller() needs to be symmetrical with _add_controller(), and remove the ControllerData from the widget's GList, so that the removed controller is really removed, and doesn't leave behind a ghost to cause crashes later.
Comment 1 Daniel Boles 2018-01-18 00:48:36 UTC
Actually, since there's weak pointer shenanigans going on that may make NULLs magically appear, perhaps the correct and simplest fix is just to have the culprit, reset_controllers() check for NULL EventControllerData->controller and skip those, like pretty much every other loop over that GList does.
Comment 2 Daniel Boles 2018-01-18 00:51:04 UTC
The nodes whose controller pointer gets NULLed out by gtk_event_controller_dispose() calling _gtk_widget_remove_controller() probably get pruned in most cases, since _run_controllers() frees and deletes links whose controller is NULL. Obviously, though, that doesn't get run during finalize(), so we need to add that extra bit of protection to reset_controllers() for that.
Comment 3 Daniel Boles 2018-01-18 01:34:11 UTC
Created attachment 366981 [details] [review]
Widget: Don’t call reset() on NULL EventController

GtkGesture is a GtkEventController. gtk_event_controller_dispose() calls
_gtk_widget_remove_controller(). That NULLs the pointer-to-Controller in
our EventControllerData but does not delete said ECData from our GList.

Subsequently, if that same Widget gets unparent()ed, that method calls
unset_state_flags(), which leads to doing reset_controllers() if we are
insensitive. Now, unlike most most other loops over the GList of ECData,
reset_controllers() does not skip nodes whose pointer-to-Controller is
NULL. So, we call gtk_event_controller_reset(NULL) and get a CRITICAL.

This surfaced in a gtkmm program. The Gesture is destroyed before the
Widget. The Widget then gets dispose()d, which calls unparent()… boom.
I didn’t find an MCVE yet but would hope this logic is correct anyway:

The simplest fix is to make the loop in gtk_widget_reset_controllers()
skip GList nodes with a NULL Controller pointer, like most other such
loops, so we avoid passing the NULL to gtk_event_controller_reset().

In other, live cases, _gtk_widget_run_controllers() loops over the GList
and removes/frees nodes having NULL Controllers, so that should suffice.
But this clearly was not getting a chance to happen in the failing case.
Comment 4 Carlos Garnacho 2018-01-22 19:06:52 UTC
Comment on attachment 366981 [details] [review]
Widget: Don’t call reset() on NULL EventController

Agree with the last opinions and patch :). We already dug ourselves deep enough with event controller lifetimes.
Comment 5 Daniel Boles 2018-01-22 19:30:22 UTC
Attachment 366981 [details] pushed as b8e2430 - Widget: Don’t call reset() on NULL EventController
Comment 6 Daniel Boles 2018-01-22 19:36:40 UTC
For posterity, this is the missing backtrace, from the point at which gtkmm started destroying the widget, in this case a Scale:

> (foo:20374): Gtk-CRITICAL **: gtk_event_controller_reset: assertion 'GTK_IS_EVENT_CONTROLLER (controller)' failed

> Thread 1 "foo" received signal SIGTRAP, Trace/breakpoint trap.
> _g_log_abort (breakpoint=1) at /home/daniel/jhbuild/checkout/gnome/glib/glib/gmessages.c:568

> (ins)(gdb) bt
> #0  _g_log_abort (breakpoint=1) at /home/daniel/jhbuild/checkout/gnome/glib/glib/gmessages.c:568
> #1  0x00007ffff3d6c5b3 in g_logv (log_domain=0x7ffff612330e "Gtk", log_level=G_LOG_LEVEL_CRITICAL, format=0x7ffff3dc7acf "%s: assertion '%s' failed", args=0x7fffffffd498) at /home/daniel/jhbuild/checkout/gnome/glib/glib/gmessages.c:1376
> #2  0x00007ffff3d6c6a4 in g_log (log_domain=0x7ffff612330e "Gtk", log_level=G_LOG_LEVEL_CRITICAL, format=0x7ffff3dc7acf "%s: assertion '%s' failed") at /home/daniel/jhbuild/checkout/gnome/glib/glib/gmessages.c:1417
> #3  0x00007ffff3d6e1ee in g_return_if_fail_warning (log_domain=0x7ffff612330e "Gtk", pretty_function=0x7ffff61235e0 <__func__.37592> "gtk_event_controller_reset", expression=0x7ffff6123408 "GTK_IS_EVENT_CONTROLLER (controller)")
>     at /home/daniel/jhbuild/checkout/gnome/glib/glib/gmessages.c:2716
> #4  0x00007ffff5e0d013 in gtk_event_controller_reset (controller=0x0) at /home/daniel/jhbuild/checkout/gnome/gtk+-3/gtk/gtkeventcontroller.c:302
> #5  0x00007ffff6094bf2 in gtk_widget_reset_controllers (widget=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/gtk+-3/gtk/gtkwidget.c:17486
> #6  0x00007ffff608b222 in gtk_widget_propagate_state (widget=0x555556569880, data=0x7fffffffd6dc) at /home/daniel/jhbuild/checkout/gnome/gtk+-3/gtk/gtkwidget.c:12902
> #7  0x00007ffff6083d64 in gtk_widget_update_state_flags (widget=0x555556569880, flags_to_set=GTK_STATE_FLAG_NORMAL, flags_to_unset=GTK_STATE_FLAG_BACKDROP) at /home/daniel/jhbuild/checkout/gnome/gtk+-3/gtk/gtkwidget.c:8802
> #8  0x00007ffff6083f8e in gtk_widget_unset_state_flags (widget=0x555556569880, flags=GTK_STATE_FLAG_BACKDROP) at /home/daniel/jhbuild/checkout/gnome/gtk+-3/gtk/gtkwidget.c:8871
> #9  0x00007ffff607baa1 in gtk_widget_unparent (widget=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/gtk+-3/gtk/gtkwidget.c:4655
> #10 0x00007ffff5d421f1 in gtk_box_remove (container=0x5555565d0b70, widget=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/gtk+-3/gtk/gtkbox.c:2629
> #11 0x00007ffff78de3e6 in Gtk::Container_Class::remove_callback_normal (self=0x5555565d0b70, p0=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/gtkmm-3/gtk/gtkmm/
> #12 0x00007ffff78de440 in Gtk::Container_Class::remove_callback (self=0x5555565d0b70, p0=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/gtkmm-3/gtk/gtkmm/
> #13 0x00007ffff4051b0f in g_cclosure_marshal_VOID__OBJECTv (closure=0x555555efacb0, return_value=0x0, instance=0x5555565d0b70, args=0x7fffffffdca8,
>     marshal_data=0x7ffff78de402 <Gtk::Container_Class::remove_callback(_GtkContainer*, _GtkWidget*)>, n_params=1, param_types=0x555555eface0) at /home/daniel/jhbuild/checkout/gnome/glib/gobject/gmarshal.c:2102
> #14 0x00007ffff404d16e in g_type_class_meta_marshalv (closure=0x555555efacb0, return_value=0x0, instance=0x5555565d0b70, args=0x7fffffffdca8, marshal_data=0x340, n_params=1, param_types=0x555555eface0)
>     at /home/daniel/jhbuild/checkout/gnome/glib/gobject/gclosure.c:1024
> #15 0x00007ffff404cd30 in _g_closure_invoke_va (closure=0x555555efacb0, return_value=0x0, instance=0x5555565d0b70, args=0x7fffffffdca8, n_params=1, param_types=0x555555eface0)
>     at /home/daniel/jhbuild/checkout/gnome/glib/gobject/gclosure.c:867
> #16 0x00007ffff4068b4f in g_signal_emit_valist (instance=0x5555565d0b70, signal_id=121, detail=0, var_args=0x7fffffffdca8) at /home/daniel/jhbuild/checkout/gnome/glib/gobject/gsignal.c:3300
> #17 0x00007ffff4069d95 in g_signal_emit (instance=0x5555565d0b70, signal_id=121, detail=0) at /home/daniel/jhbuild/checkout/gnome/glib/gobject/gsignal.c:3447
> #18 0x00007ffff5da2581 in gtk_container_remove (container=0x5555565d0b70, widget=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/gtk+-3/gtk/gtkcontainer.c:1905
> #19 0x00007ffff608993f in gtk_widget_dispose (object=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/gtk+-3/gtk/gtkwidget.c:12081
> #20 0x00007ffff79d4bcd in Gtk::Widget_Class::dispose_vfunc_callback (self=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/gtkmm-3/gtk/gtkmm/
> #21 0x00007ffff40540cb in g_object_run_dispose (object=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/glib/gobject/gobject.c:1100
> #22 0x00007ffff7a24707 in Gtk::Object::_release_c_instance (this=0x555555daf030 <djb::foo::gui::(anonymous namespace)::s_bar+54256>) at /home/daniel/jhbuild/checkout/gnome/gtkmm-3/gtk/gtkmm/
> #23 0x00007ffff7a24c01 in Gtk::Object::destroy_ (this=0x555555daf030 <djb::foo::gui::(anonymous namespace)::s_bar+54256>) at /home/daniel/jhbuild/checkout/gnome/gtkmm-3/gtk/gtkmm/
> #24 0x00007ffff79734a7 in Gtk::Scale::~Scale (this=0x555555daf030 <djb::foo::gui::(anonymous namespace)::s_bar+54256>, __in_chrg=<optimized out>, __vtt_parm=<optimized out>)