GNOME Bugzilla – Bug 792624
Critical due to gtk_widget_reset_controllers() passing NULL to gtk_event_controller_reset()
Last modified: 2018-01-22 19:36:40 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.
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.
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.
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 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.
Attachment 366981 [details] pushed as b8e2430 - Widget: Don’t call reset() on NULL EventController
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/container.cc:143 > #12 0x00007ffff78de440 in Gtk::Container_Class::remove_callback (self=0x5555565d0b70, p0=0x555556569880) at /home/daniel/jhbuild/checkout/gnome/gtkmm-3/gtk/gtkmm/container.cc:163 > #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/widget.cc:678 > #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/object.cc:131 > #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/object.cc:266 > #24 0x00007ffff79734a7 in Gtk::Scale::~Scale (this=0x555555daf030 <djb::foo::gui::(anonymous namespace)::s_bar+54256>, __in_chrg=<optimized out>, __vtt_parm=<optimized out>)