GNOME Bugzilla – Bug 746940
preview-nav-buttons: Crashes if shutdown and fade_out_button coincide
Last modified: 2015-12-15 14:17:34 UTC
I saw this crash a few days ago, but I haven't been able to reproduce it again with full debug symbols, and I can't get coredumpctl to show me the original with full debug symbols either. So we will have to go with what we have: #0 0x00007f8aeb2a5230 g_type_check_instance_cast (libgobject-2.0.so.0) #1 0x000000000045af58 photos_preview_nav_buttons_fade_out_button (gnome-photos) #2 0x000000000045b0d1 photos_preview_nav_buttons_update_visibility (gnome-photos) #3 0x000000000045b153 photos_preview_nav_buttons_auto_hide (gnome-photos) #4 0x00007f8aead7b343 g_timeout_dispatch (libglib-2.0.so.0) #5 0x00007f8aead7a91b g_main_dispatch (libglib-2.0.so.0) #6 0x00007f8aead7ac88 g_main_context_iterate (libglib-2.0.so.0) #7 0x00007f8aead7ad2c g_main_context_iteration (libglib-2.0.so.0) #8 0x00007f8aebdb79a6 g_application_run (libgio-2.0.so.0) #9 0x00000000004773ef main (gnome-photos) #10 0x00007f8aea25afe0 __libc_start_main (libc.so.6) #11 0x0000000000428979 _start (gnome-photos) The problem is that the lifetime of the overlaid widgets created by the PreviewNavButtons class are not tied to the lifetime of the class itself. So even though we keep the class alive across the timeout and idle sources, the widgets are destroyed as soon as their parent GtkOverlay is destroyed. Therefore if fade_out_button is called just after the widgets have been destroyed during shutdown, we get a crash. Not exactly sure how this can happen, but apparently it did happen. We should either remove the idle and timeout sources when the class is destroyed, in which case we should not take a ref when adding the sources. Or we can keep our own ref on the widgets themselves, and unref them during destruction.
Created attachment 300737 [details] [review] Initial_patch I have tried to put up the patch as you guided but I am not sure with the solution as if it is upto requirement although on run it posed no problem. Reason for the place I added ref_sink : for widget priv->prev_widget we will g_object_ref_sink() before gtk_overlay_add_overlay (GTK_OVERLAY (priv->overlay), priv->prev_widget); so that its ref_sink() makes floating reference normal reference and later i.e. add_overlay increases its reference count by 1. And finally we unref the widget making its final reference 0. PLEASE CAN YOU TELL ME IF I AM CORRECT. Also there are 2 Gtkbutton and 2 Gtkimages widgets created by button = gtk_button_new (); and image = gtk_image_new_from_icon_name (PHOTOS_ICON_GO_NEXT_SYMBOLIC, GTK_ICON_SIZE_INVALID); but on g_object_ref_sink(button) jhbuild said error: ‘button’ undeclared (first use in this function) Similar for the image. But these were added to the container(priv->prev_widget and priv->next_widget)that are successfully referenced(that is ref_sink() added to them) in the patch.
Review of attachment 300737 [details] [review]: Thanks, Sparsh. The commit message needs some work: a) No need for the "Fixed crash -" part. b) Instead of literally describing the code changes, try to describe why that was necessary. The code changes are obvious from look at the patch, but the reasoning isn't always so. Also, see https://wiki.gnome.org/Git/CommitMessages ::: src/photos-preview-nav-buttons.c @@ +293,3 @@ g_clear_object (&priv->model); + g_object_unref (priv->prev_widget); + g_object_unref (priv->next_widget); g_clear_object, not g_object_unref @@ +331,3 @@ gtk_widget_set_valign (priv->prev_widget, GTK_ALIGN_CENTER); gtk_revealer_set_transition_type (GTK_REVEALER (priv->prev_widget), GTK_REVEALER_TRANSITION_TYPE_CROSSFADE); + g_object_ref_sink(priv->prev_widget); Missing space between function name and opening parenthesis. Also do the g_object_ref_sink right where the revealer is created, as we do elsewhere. @@ +341,3 @@ context = gtk_widget_get_style_context (button); gtk_style_context_add_class (context, "osd"); + g_object_ref_sink(priv->prev_widget); Ditto.
(In reply to sparshpaliwal123@gmail.com from comment #1) > Reason for the place I added ref_sink : > for widget priv->prev_widget we will g_object_ref_sink() before > gtk_overlay_add_overlay (GTK_OVERLAY (priv->overlay), priv->prev_widget); so > that its ref_sink() makes floating reference normal reference and later i.e. > add_overlay increases its reference count by 1. And finally we unref the > widget making its final reference 0. Functionally, it doesn't matter whether we claim the reference before adding it to the container or after. > PLEASE CAN YOU TELL ME IF I AM CORRECT. No need to use caps. :) > Also there are 2 Gtkbutton and 2 Gtkimages widgets created by > button = gtk_button_new (); > and > image = gtk_image_new_from_icon_name (PHOTOS_ICON_GO_NEXT_SYMBOLIC, > GTK_ICON_SIZE_INVALID); > > but on g_object_ref_sink(button) jhbuild said > error: ‘button’ undeclared (first use in this function) > Similar for the image. I can't say why you would get such an error without knowing what you did, but as you said below, we don't need to touch those. > But these were added to the container(priv->prev_widget and > priv->next_widget)that are successfully referenced(that is ref_sink() added > to them) in the patch.
Created attachment 300753 [details] [review] final_patch Thankyou, I have made all the changes as you said and for the commit I read some examples on Code Contribution Workflow I have kept short description and long description and have tried to keep it more meaningful as you said.
Review of attachment 300753 [details] [review]: One of the most important things for a commit message is to not exceed 72 characters per line. See https://wiki.gnome.org/Git/CommitMessages Also, "preview-nav-buttons", not "preview-nav". ::: src/photos-preview-nav-buttons.c @@ +327,2 @@ priv->prev_widget = gtk_revealer_new (); + g_object_ref_sink (priv->prev_widget); ... = g_object_ref_sink (gtk_revealer_new ()); @@ +357,2 @@ priv->next_widget = gtk_revealer_new (); + g_object_ref_sink (priv->prev_widget); Ditto.
Created attachment 300791 [details] [review] final_patch_fix Thankyou and sorry for repeatedly making commit comment mistake. I guess this will be my last time with these mistakes. :-)
Review of attachment 300791 [details] [review]: After re-reading the code, I think my initial analysis was wrong. The PreviewNavButtons class already owns a hard reference on the GtkOverlay, and the buttons are part of it. So, there shouldn't be a need to take a reference on the buttons. Maybe something will pop up if we run this through valgrind.
(In reply to Debarshi Ray from comment #7) First of all, PreviewNavButtons shouldn't hold strong references on PreviewView and the overlay because PreviewNavButtons is meant to be owned by PreviewView. With that straightened out, it becomes more obvious that we should tie the lifetime of the internal widgets to that of PreviewNavButtons (which is not a GtkWidget).
Created attachment 317423 [details] [review] preview-nav-buttons: Break a reference cycle
Created attachment 317424 [details] [review] preview-nav-buttons: Be more strict about what is acceptable
Created attachment 317425 [details] [review] preview-nav-buttons: Hold references on internal widgets
Thanks for your effort, Sparsh. I am sorry that it took so long to sort this out.