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 744879 - Another core dump when clicking the Back button
Another core dump when clicking the Back button
Status: RESOLVED FIXED
Product: gnome-software
Classification: Applications
Component: General
3.14.x
Other Linux
: Normal critical
: ---
Assigned To: GNOME Software maintainer(s)
GNOME Software maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-02-21 01:15 UTC by Rafal Luzynski
Modified: 2015-02-27 11:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Backtrace of the core dump (8.87 KB, text/plain)
2015-02-21 01:15 UTC, Rafal Luzynski
  Details
Avoid core dump in gtk_list_box_row_grab_focus() (1.11 KB, patch)
2015-02-26 01:58 UTC, Rafal Luzynski
none Details | Review
Avoid core dump or critical warning in gs_shell_back_button_cb() (1.13 KB, patch)
2015-02-26 02:03 UTC, Rafal Luzynski
none Details | Review
Avoid core dump in gtk_list_box_row_grab_focus() (1000 bytes, patch)
2015-02-27 01:08 UTC, Rafal Luzynski
none Details | Review
Avoid core dump or critical warning in gs_shell_back_button_cb() (1.77 KB, patch)
2015-02-27 02:29 UTC, Rafal Luzynski
accepted-commit_now Details | Review

Description Rafal Luzynski 2015-02-21 01:15:23 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?
Comment 1 Rafal Luzynski 2015-02-26 01:51:54 UTC
(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().
Comment 2 Rafal Luzynski 2015-02-26 01:58:34 UTC
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)
Comment 3 Rafal Luzynski 2015-02-26 02:03:23 UTC
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.
Comment 4 Matthias Clasen 2015-02-27 00:10:25 UTC
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.
Comment 5 Matthias Clasen 2015-02-27 00:11:57 UTC
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.
Comment 6 Rafal Luzynski 2015-02-27 01:08:45 UTC
Created attachment 298053 [details] [review]
Avoid core dump in gtk_list_box_row_grab_focus()

Corrected according to Matthias Clasen's suggestions.
Comment 7 Rafal Luzynski 2015-02-27 02:29:41 UTC
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.
Comment 8 Kalev Lember 2015-02-27 11:18:12 UTC
Review of attachment 298057 [details] [review]:

Great fix, thanks Rafal! Looks good to me.