GNOME Bugzilla – Bug 783587
Crash when NULL is passed to GtkActionHelper to unset action-name
Last modified: 2017-06-13 20:00:13 UTC
gtk_actionable_set_action_name [1] is supposed to accept a NULL action-name to dissociate the widget from any previous GAction. However, implementations using GtkActionHelper fail to do that and crash:
+ Trace 237561
There are three GtkActionHelper based widgets at the moment - GtkMenuItem, GtkButton and GtkSwitch. Of those, the latter two seem to have regressed with https://git.gnome.org/browse/gtk+/commit/?id=694be447e3f08e8ff4341099048e9b17a24c8f0e and https://git.gnome.org/browse/gtk+/commit/?id=fbb38e95ac40a50178f7dbbba5dfa8c0c0cdfb85 Note how gtk_button_update_action_observer and gtk_switch_update_action_observer used to be careful about a NULL action-name. [1] https://developer.gnome.org/gtk3/stable/GtkActionable.html#gtk-actionable-set-action-name
Note that I didn't originally intend to use NULL to unset the GAction. I wanted to switch the same GtkButton between two GActions - one without any parameters, and one that takes a double. Unfortunately, these sequences of calls trigger the WARNING in gtk_action_helper_action_added: gtk_actionable_set_action_name (..., "action-no-parameter"); gtk_actionable_set_action_name (..., "action-parameter"); gtk_actionable_set_action_target (..., "d", 1.0); ... or: gtk_actionable_set_action_name (..., "action-parameter"); gtk_actionable_set_action_target (..., "d", 1.0); gtk_actionable_set_action_name (..., "action-no-parameter"); It tries to match the parameter type from the earlier action with the target from the newer action. Trying to work around that by forcefully unsetting the GAction led to this bug. One bug at a time...
Created attachment 353451 [details] [review] GtkActionHelper: Allow a NULL action-name to unset the previous GAction
Review of attachment 353451 [details] [review]: Seems like the function must accept NULL, indeed. ::: gtk/gtkactionhelper.c @@ +473,1 @@ I think this is not quite right. When setting action_name to NULL, we also need to set enabled to FALSE, I believe, and call gtk_action_helper_report_change
(In reply to Matthias Clasen from comment #3) > Review of attachment 353451 [details] [review] [review]: > > Seems like the function must accept NULL, indeed. > > ::: gtk/gtkactionhelper.c > @@ +473,1 @@ > > > I think this is not quite right. When setting action_name to NULL, we also > need to set enabled to FALSE, I believe, and call > gtk_action_helper_report_change Ok. I wasn't sure if enabled should be TRUE or FALSE because ultimately it drives GtkWidget:sensitive which is TRUE by default. But I think FALSE is right because it matches with the gtk_action_observer_action_removed behaviour.
Created attachment 353660 [details] [review] GtkActionHelper: Allow a NULL action-name to unset the previous GAction
Created attachment 353662 [details] [review] GtkActionHelper: Remove unnecessary NULL check I don't understand this comment in gtk_action_helper_set_action_name: * When called during construction, widget is NULL. We don't need to * report in that case. And in trying to understand that, I found this needless NULL check.
Review of attachment 353660 [details] [review]: it looks right to me
Review of attachment 353662 [details] [review]: hmm, right
Thanks for the reviews! Pushed to master and gtk-3-22.