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 751847 - crash when sending notify to client
crash when sending notify to client
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2015-07-02 15:27 UTC by Marek Chalupa
Modified: 2016-05-12 18:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
First change/remove window and then set surface to NULL (2.17 KB, patch)
2015-07-02 15:27 UTC, Marek Chalupa
none Details | Review
reset surface after we don't need it (1.88 KB, patch)
2016-02-19 08:49 UTC, Marek Chalupa
none Details | Review
Don't resize wayland window when it's being unmanaged (1.76 KB, patch)
2016-03-17 14:01 UTC, Marek Chalupa
none Details | Review
Don't send notify when the window is being destroyed (2.28 KB, patch)
2016-04-22 14:11 UTC, Marek Chalupa
reviewed Details | Review
Don't send notify when the window is being destroyed (2.76 KB, patch)
2016-05-06 12:32 UTC, Marek Chalupa
committed Details | Review

Description Marek Chalupa 2015-07-02 15:27:31 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.
Comment 1 Marek Chalupa 2015-07-02 15:27:59 UTC
Downstream bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1214462
Comment 2 Owen Taylor 2015-11-09 15:31:21 UTC
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.
Comment 3 Marek Chalupa 2016-02-19 08:48:09 UTC
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?
Comment 4 Marek Chalupa 2016-02-19 08:49:00 UTC
Created attachment 321637 [details] [review]
reset surface after we don't need it
Comment 5 Jasper St. Pierre (not reading bugmail) 2016-02-19 08:54:15 UTC
There's a very good reason I did the unmanaging of the surface first, rather than last, but I forget what it was.
Comment 6 Marek Chalupa 2016-03-17 14:01:21 UTC
Created attachment 324184 [details] [review]
Don't resize wayland window when it's being unmanaged
Comment 7 Marek Chalupa 2016-04-22 14:11:19 UTC
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)
Comment 8 Marek Chalupa 2016-04-22 14:14:15 UTC
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
Comment 9 Rui Matos 2016-04-29 15:48:02 UTC
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?
Comment 10 Marek Chalupa 2016-05-06 12:28:19 UTC
(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.
Comment 11 Marek Chalupa 2016-05-06 12:32:33 UTC
Created attachment 327385 [details] [review]
Don't send notify when the window is being destroyed
Comment 12 Rui Matos 2016-05-11 17:10:29 UTC
Review of attachment 327385 [details] [review]:

looks fine