GNOME Bugzilla – Bug 751847
crash when sending notify to client
Last modified: 2016-05-12 18:48:07 UTC
Created attachment 306639 [details] [review] First change/remove window and then set surface to NULL When screen is closing, it tries to unmaximize windows, thus sending notify event. But before that we set surface to NULL (since we're tearing down the window), so we get SIGSEGV when we do that.
Downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1214462
Review of attachment 306639 [details] [review]: I keep on staring at this patch, wondering if it's safe or it causes some weird unexpected side effect. Moving the call to unmaximize_window_before_freeing() before setting window->unmanaging definitely looks like it *could* cause a problem somewhere. Honestly, I'd rather that the window->screen->closing branch of unmaximize_window_before_freeing() was made a no-op when running as a wayland compositor - it's all about the next window manager that replaces us, and that is impossible in the Wayland case. So why do the work and potentially confuse a client.
Was running the patch for quite time and nothing happend - but you're right, setting window->unmanaging should probably happen before. Anyway, this bug is not just about closing the screen. It can be hit even when closing a window (https://bugzilla.redhat.com/show_bug.cgi?id=1293479). So the distilled problem is: meta_window_unmanage() sets window->surface to NULL and then some function called from meta_window_unmanage() later dereferences the surface. The surface pointer is valid all the time, so couldn't we just keep it until the end?
Created attachment 321637 [details] [review] reset surface after we don't need it
There's a very good reason I did the unmanaging of the surface first, rather than last, but I forget what it was.
Created attachment 324184 [details] [review] Don't resize wayland window when it's being unmanaged
Created attachment 326552 [details] [review] Don't send notify when the window is being destroyed This patch accounts even for https://bugzilla.redhat.com/show_bug.cgi?id=1327835 that I wrongly marked as duplicate (well, it was not so wrong, since the cause is the same - but the error path is different)
Reproducer for the other error path: 1. open weston-terminal 2. type: sleep 3 && kill `pidof weston-terminal` 3. grab corner of weston-terminal and start resizing it 4. when the weston-terminal is killed while being resized, gnome-shell crashes
Review of attachment 326552 [details] [review]: ::: src/wayland/meta-window-wayland.c @@ +134,3 @@ + /* don't send notify when the window is being unmanaged */ + if (window->unmanaging) + return; Moving this check here seems correct but please remove the same check in appears_focused_changed() below as it's redundant now. @@ +180,3 @@ + /* don't do anything if we're dropping the window, see #751847 */ + if (window->unmanaging) + return; I don't think we need this one, have you seen it crash here?
(In reply to Rui Matos from comment #9) > Review of attachment 326552 [details] [review] [review]: > > ::: src/wayland/meta-window-wayland.c > @@ +134,3 @@ > + /* don't send notify when the window is being unmanaged */ > + if (window->unmanaging) > + return; > > Moving this check here seems correct but please remove the same check in > appears_focused_changed() below as it's redundant now. Right, thanks for pointing that out. > > @@ +180,3 @@ > + /* don't do anything if we're dropping the window, see #751847 */ > + if (window->unmanaging) > + return; > > I don't think we need this one, have you seen it crash here? Yes, meta_window_wayland_move_resize_internal() calls meta_wayland_surface_configure_notify() directly, not via surface_state_changed() and that leads to crash here: https://bugzilla.redhat.com/show_bug.cgi?id=1214462 Solution that covers both error paths would be to check for NULL surface in meta_wayland_surface_configure_notify(), but IMO that could possibly hide some (future) bugs when NULL is incorrectly passed as the surface on some other occasion.
Created attachment 327385 [details] [review] Don't send notify when the window is being destroyed
Review of attachment 327385 [details] [review]: looks fine