GNOME Bugzilla – Bug 744879
Another core dump when clicking the Back button
Last modified: 2015-02-27 11:33:54 UTC
Created attachment 297476 [details] Backtrace of the core dump Steps to reproduce: 1. Make sure that gnome-software is not running; killall gnome-software if needed. 2. Get the latest gtk+ and optionally gnome-software from git. 3. Launch gnome-software using the latest GTK, jhbuild is helpful. 4. Search anything, click any search result and see the details. 5. Click the Back (<) button and see the core dump. This bug exists in both gnome-software 3.14.x (from the binary repository) and 3.15.x (from the git source) but requires the latest GTK 3.15.8 from git source because the code that crashes has been added with https://git.gnome.org/browse/gtk+/commit/?id=9141eeb60 which is the fix for bug 708320. It is obvious why it crashes: because box is NULL but why a list box row exists without a box? Is it because it had had the focus previously and has been referenced by the focus stack (see: https://git.gnome.org/browse/gnome-software/commit/src/gs-shell.c?id=f8b4a55fb), then the list box has been destroyed but we still keep the reference and we try to restore the focus on this list row? Should there be a check if entry->focus still has a parent and has not been destroyed? And by the way, why the list box row has 3 references? Who else references it?
(In reply to Rafal Luzynski from comment #0) > And > by the way, why the list box row has 3 references? Who else references it? 1. BackEntry->focus which has been obvious to me; 2. gtk_widget_grab_focus(); 3. g_signal_emit_valist().
Created attachment 297944 [details] [review] Avoid core dump in gtk_list_box_row_grab_focus() Note: This is a patch against gtk+, not gnome-software. There are two things we can (and should) do: 1. Prevent gtk_list_box_row_grab_focus() from crashing when an incorrect value is passed as an argument. This patch converts the core dump into a critical warning. (to be continued)
Created attachment 297946 [details] [review] Avoid core dump or critical warning in gs_shell_back_button_cb() 2. Check if entry->focus is still a valid widget for gtk_widget_grab_focus(). Note that it may still be a valid GtkWidget object so it will pass a simple check in gtk_widget_grab_focus() but it will cause a core dump in a deeper code.
Review of attachment 297944 [details] [review]: ::: gtk/gtklistbox.c @@ +3485,3 @@ gtk_list_box_row_grab_focus (GtkWidget *widget) { + g_return_if_fail (GTK_IS_LIST_BOX_ROW (widget)); I disagree with this one - static functions like this one must be able to rely on not getting called on the wrong type. @@ +3490,3 @@ GtkListBox *box = gtk_list_box_row_get_box (row); + g_return_if_fail (box != NULL); I agree with this one.
Review of attachment 297946 [details] [review]: ::: src/gs-shell.c @@ +282,3 @@ + if (entry->focus != NULL && gtk_widget_get_mapped (entry->focus) + && gtk_widget_get_realized (entry->focus)) I think the change should be slightly different here: Instead of taking a ref on entry->focus and thus keeping the object artificially alive, instead, use a weak_pointer to clear entry->focus when the object goes away.
Created attachment 298053 [details] [review] Avoid core dump in gtk_list_box_row_grab_focus() Corrected according to Matthias Clasen's suggestions.
Created attachment 298057 [details] [review] Avoid core dump or critical warning in gs_shell_back_button_cb() Uses the weak_pointer pattern, as suggested by Matthias. Still may display the critical warning when the focus widget is referenced too many times but this is correct.
Review of attachment 298057 [details] [review]: Great fix, thanks Rafal! Looks good to me.