[virt-tools-list] [PATCH virt-viewer] window: Keep reference on timed revealer

Tue Jun 28 13:05:52 UTC 2016

On Tue, Jun 28, 2016 at 02:03:02PM +0200, Pavel Grunt wrote:
> Avoid crash when closing virt-viewer due to multiple unrefs in
> virt_viewer_timed_revealer_dispose and virt_viewer_window_dispose:
>  #0  0x00007ffff3e92c9d in g_type_check_instance_is_fundamentally_a () at /lib64/libgobject-2.0.so.0
>  #1  0x00007ffff3e722a5 in g_object_unref () at /lib64/libgobject-2.0.so.0
>  #2  0x000000000041ebe3 in virt_viewer_timed_revealer_dispose (object=0x1127320) at virt-viewer-timed-revealer.c:128
>  #3  0x00007ffff3e723b6 in g_object_unref () at /lib64/libgobject-2.0.so.0
>  #4  0x000000000041c040 in virt_viewer_window_dispose (object=0x981f70) at virt-viewer-window.c:191
>  #5  0x00007ffff3e723b6 in g_object_unref () at /lib64/libgobject-2.0.so.0
>  #6  0x0000000000413a58 in virt_viewer_app_display_removed (nth=<optimized out>, self=0x680330) at virt-viewer-app.c:989
>  #7  0x0000000000413a58 in virt_viewer_app_display_removed (session=<optimized out>, display=<optimized out>, self=0x680330) at virt-viewer-app.c:1000
>  #8  0x00007ffff3e705e0 in g_cclosure_marshal_VOID__OBJECTv () at /lib64/libgobject-2.0.so.0
>  #9  0x00007ffff3e6d784 in _g_closure_invoke_va () at /lib64/libgobject-2.0.so.0
>  #10 0x00007ffff3e88cd9 in g_signal_emit_valist () at /lib64/libgobject-2.0.so.0
>  #11 0x00007ffff3e897eb in g_signal_emit_by_name () at /lib64/libgobject-2.0.so.0
>  #12 0x0000000000418973 in virt_viewer_session_remove_display (session=0x9c6de0, display=0x961a90) at virt-viewer-session.c:463
>  #13 0x0000000000420934 in destroy_display (data=<optimized out>) at virt-viewer-session-spice.c:851
>  #14 0x00007ffff3b6d0eb in g_ptr_array_foreach () at /lib64/libglib-2.0.so.0
>  #15 0x00007ffff3b6d180 in ptr_array_free () at /lib64/libglib-2.0.so.0
>  #16 0x000000000042072a in virt_viewer_session_spice_clear_displays (self=0x9c6de0) at virt-viewer-session-spice.c:94
>  #17 0x000000000042240d in virt_viewer_session_spice_close (session=<optimized out>) at virt-viewer-session-spice.c:459
>  #18 0x0000000000414be5 in virt_viewer_app_quit (self=self at entry=0x680330) at virt-viewer-app.c:285
>  #19 0x0000000000415500 in virt_viewer_app_maybe_quit (self=0x680330, window=window at entry=0x981a90) at virt-viewer-app.c:481
>  #20 0x000000000041c4ad in virt_viewer_window_delete (src=<optimized out>, dummy=<optimized out>, self=0x981a90) at virt-viewer-window.c:771
>  #21 0x00007ffff61807f1 in _gtk_marshal_BOOLEAN__BOXEDv () at /lib64/libgtk-3.so.0
>  #22 0x00007ffff3e6d784 in _g_closure_invoke_va () at /lib64/libgobject-2.0.so.0
>  #23 0x00007ffff3e887b3 in g_signal_emit_valist () at /lib64/libgobject-2.0.so.0
>  #24 0x00007ffff3e8933f in g_signal_emit () at /lib64/libgobject-2.0.so.0
>  #25 0x00007ffff62dde6c in gtk_widget_event_internal () at /lib64/libgtk-3.so.0
>  #26 0x00007ffff617f5ef in gtk_main_do_event () at /lib64/libgtk-3.so.0
>  #27 0x00007ffff5c7dd25 in _gdk_event_emit () at /lib64/libgdk-3.so.0
>  #28 0x00007ffff5cae672 in gdk_event_source_dispatch () at /lib64/libgdk-3.so.0
>  #29 0x00007ffff3b9895a in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
>  #30 0x00007ffff3b98d10 in g_main_context_iterate.isra () at /lib64/libglib-2.0.so.0
>  #31 0x00007ffff3b98dbc in g_main_context_iteration () at /lib64/libglib-2.0.so.0
>  #32 0x00007ffff41643cd in g_application_run () at /lib64/libgio-2.0.so.0
>  #33 0x000000000040fc1a in main (argc=3, argv=0x7fffffffdec8) at virt-viewer-main.c:41
> 
> Fixes: cc455b7f916110d7cfae6b7af753349e070c9494
> ---
>  src/virt-viewer-window.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
> index 1ebb423..f78df03 100644
> --- a/src/virt-viewer-window.c
> +++ b/src/virt-viewer-window.c
> @@ -1113,7 +1113,7 @@ virt_viewer_window_toolbar_setup(VirtViewerWindow *self)
>      gtk_toolbar_insert(GTK_TOOLBAR(priv->toolbar), GTK_TOOL_ITEM(button), 0);
>      g_signal_connect(button, "clicked", G_CALLBACK(virt_viewer_window_toolbar_leave_fullscreen), self);
>  
> -    priv->revealer = virt_viewer_timed_revealer_new(priv->toolbar);
> +    priv->revealer = g_object_ref_sink(virt_viewer_timed_revealer_new(priv->toolbar));

VirtViewerTimedRevealer is not a gtk+ widget, so _ref_sink() is odd.
After this VirtViewerWindow will have 2 refs on VirtViewerTimedRevealer,
so it seems the crash above is avoided because
VirtViewerTimedRevealer::dispose is never going to be called?

The root cause of the crash above seems to be in
VirtViewerTimedRevealer which does:
    priv->revealer = gtk_revealer_new();
    gtk_container_add(GTK_CONTAINER(priv->revealer), toolbar);
in _new() and then in _dispose():
    g_clear_object(&priv->revealer)

gtk_container_add() will have taken ownership of the revealer floating
ref, so the g_clear_object() should not be done. Alternatively, you can
_ref_sink() the GtkRevealer returned by gtk_revealer_new() to own a ref
on it.

A similar issue seems to be occurring with self->priv->evBox since
virt-viewer-window.c calls
virt_viewer_timed_revealer_get_overlay_widget() and then adds the
resulting widget to a container with gtk_container_add(). In this case,
I would _sink() the ref we got in virt_viewer_timed_revealer_new in
order not to make any assumptions on what the caller of
_get_overlay_widget is going to do with the returned value.

Looking at this code, I'm wondering why VirtViewerTimedRevealer is not
inheriting from GtkEventBox rather than having one instance of it as a
member? It seems that virt_viewer_timed_revealer_get_overlay_widget()
would become unneeded, and having it being a GtkContainer subclass would
make it more natural that it takes ownership of the GtkToolbar floating
ref.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20160628/60168268/attachment.sig>