GNOME Bugzilla – Bug 776130
gtk-embed: Don't try to compare with unrealized window
Last modified: 2017-08-24 09:23:51 UTC
I hit this race; I got a window-created invocation from a Wayland client, before the GtkWindow being embedded had been fully realized. This caused a crash because gdk_x11_window_get_xid() tried to dereference the NULL GdkWindow that was passed to it.
Created attachment 342013 [details] [review] gtk-embed: Don't try to compare with unrealized window After having embedded a Gtk window, we start to listen for the corresponding MetaWindow to know when it has been mapped by the compositor. If the embedder does not realize the window immediately, and some other window is created and mapped before the embedder realizes the window, we would prior to this patch try to dereference the not created GdkWindow. Fix this by NULL checking the GdkWindow before dereferencing. We will never miss the needed MetaWindow creation since it can not have been mapped have it not yet been realized.
Review of attachment 342013 [details] [review]: Makes sense
Attachment 342013 [details] pushed as c131c44 - gtk-embed: Don't try to compare with unrealized window
Hm, are you sure this is the right fix? I'm fairly regularly seeing a crash here, with 3.24.1 in F26: Stack trace of thread 25834: #0 0x00007f549f255a46 g_type_check_instance_cast (libgobject-2.0.so.0) #1 0x00007f54a782014c shell_gtk_embed_window_created_cb (libgnome-shell.so) #2 0x00007f549f23030d g_closure_invoke (libgobject-2.0.so.0) #3 0x00007f549f2427ae signal_emit_unlocked_R (libgobject-2.0.so.0) #4 0x00007f549f24b1d5 g_signal_emit_valist (libgobject-2.0.so.0) #5 0x00007f549f24bb2f g_signal_emit (libgobject-2.0.so.0) #6 0x00007f54a398c732 _meta_window_shared_new (libmutter-0.so.0) #7 0x00007f54a39c0272 meta_window_wayland_new (libmutter-0.so.0) #8 0x00007f54a39c2a25 xdg_surface_constructor_get_toplevel (libmutter-0.so.0) #9 0x00007f549c023bde ffi_call_unix64 (libffi.so.6) #10 0x00007f549c02354f ffi_call (libffi.so.6) #11 0x00007f549e8e8a54 wl_closure_invoke (libwayland-server.so.0) #12 0x00007f549e8e52cf wl_client_connection_data (libwayland-server.so.0) #13 0x00007f549e8e6c52 wl_event_loop_dispatch (libwayland-server.so.0) #14 0x00007f54a39a80c7 wayland_event_source_dispatch (libmutter-0.so.0) #15 0x00007f549ef58207 g_main_dispatch (libglib-2.0.so.0) #16 0x00007f549ef585a8 g_main_context_iterate (libglib-2.0.so.0) #17 0x00007f549ef588c2 g_main_loop_run (libglib-2.0.so.0) #18 0x00007f54a397aa7c meta_run (libmutter-0.so.0) #19 0x0000555f9a7f84a7 main (gnome-shell) #20 0x00007f549d3945fe __libc_start_main (libc.so.6) #21 0x0000555f9a7f85ba _start (gnome-shell) Apr 18 16:46:04 strictly gnome-shell[25834]: gtk_widget_get_window: assertion 'GTK_IS_WIDGET (widget)' failed I suspect you want to test if priv->window is NULL first, before setting gdk_window. Attaching a speculative patch ...
Created attachment 350009 [details] [review] gtk-embed: Really don't try to compare with unrealized window c131c44 tried to fix the case where we could compare with an unrealized window, but missed that the window can sometimes be NULL.
NB: I have no idea if this is actually correct, or just papering over the issue.
Review of attachment 350009 [details] [review]: ::: src/shell-gtk-embed.c @@ +68,3 @@ + + if (priv->window == NULL) + return; This function would only ever be called if priv->window is not NULL. The listener is registered when its set to not-NULL, and unregistered when its set to NULL, so I don't think this patch is correct, or helps at all.
I haven't seen this crash myself. Have you any clue on when it happens?
Right, so doesn't actually help then. :( I don't have an easy reproducer, no; it just happens sometimes when windows open. Certainly XWayland windows, but not sure if it happens on native Wayland windows or not, because of a Chrome bug which leaks all the memory in the world, causing GL draw calls to sometimes fail (killing Shell) when new windows open too ... Seen it a fair few times now, but no rhyme or reason about it. Any ideas of where I should be looking?
(In reply to Daniel Stone from comment #9) > Right, so doesn't actually help then. :( I don't have an easy reproducer, > no; it just happens sometimes when windows open. > :( > Certainly XWayland windows, but not sure if it happens on native Wayland > windows or not, because of a Chrome bug which leaks all the memory in the > world, causing GL draw calls to sometimes fail (killing Shell) when new > windows open too ... > > Seen it a fair few times now, but no rhyme or reason about it. Any ideas of > where I should be looking? Well, it has something to do with tray icons, as that is AFAIK the only use of ShellGtkEmbed. So, it is probably the thing managing the ShellGtkEmbed that doesn't unset "window" property before it has been destroyed. One way that might give us a hint would be to set a weak pointer on the current window (the priv->window widget), and break/abort when it is triggered, so we'll know at what point it *should* have been destroyed.
> One way that might give us a hint would be to set a weak pointer on the > current window (the priv->window widget), and break/abort when it is > triggered, so we'll know at what point it *should* have been destroyed. No, that wouldn't help. We already listen to the "destroy" signal of priv->window and unset priv->window in this case, which would also unregister the signal listener that is the one which handler crashes in comment 4.
FWIW, I used to hit this quite a lot, but haven't seen it in a while now. So it's probably fixed, I guess.