GNOME Bugzilla – Bug 792818
Segfault in x11 session management code with sticky window
Last modified: 2018-01-25 15:21:44 UTC
Description: This is from a downstream bug yet the reproducing steps are unknown. The issue is reported on Wayland, but the client is X11 and nothing in the backtrace seemsto point toward Wayland code in mutter. The backtrace reads as follow:
+ Trace 238361
Looking at the window in frame 1, we see: window->type = META_WINDOW_NORMAL window->monitor = 0x1180000, window->override_redirect = 0, window->unmanaging = 0, window->visible_to_compositor = 1, window->known_to_compositor = 1 window->workspace = 0x0, window->always_sticky = 0, window->initial_workspace = 0, window->on_all_workspaces = 1, window->on_all_workspaces_requested = 0, window->initial_workspace_set = 1 And the code which crashes reads: https://git.gnome.org/browse/mutter/tree/src/x11/session.c#n948 948 if (window->on_all_workspaces_requested) 949 { 950 fputs (" <sticky/>\n", outfile); 951 } else { 952 int n; 953 n = meta_workspace_index (window->workspace); 954 fprintf (outfile, 955 " <workspace index=\"%d\"/>\n", n); 956 } So, indeed, on_all_workspaces_requested is FALSE and workspace is NULL. We could also check for “on_all_workspaces” here (to set sticky in the session file), but reading the commit 9d62d13 (“Split out on_all_workspaces and on_all_workspaces_requested”), I reckon we can have “on_all_workspaces” being TRUE without “on_all_workspaces_requested”, and yet the workspace being NULL. So instead, we might use “initial_workspace” as workspace index if workspace is NULL. Patch to follow.
Created attachment 367291 [details] [review] [PATCH] session: use initial workspace if no workspace set Having “on_all_workspaces_requested” not set on a window does not necessarily imply a workspace is set. If no workspace is set, use the “initial_workspace” instead to avoid a NULL pointer dereference.
It looks like it should only be possible to happen for override-redirect windows. Do we actually want to save the state for those, and do we really want to "restore" such state for those?
Nope, that window is not an O-R window, it's a regular toplevel window (from Pidgin)
(In reply to Olivier Fourdan from comment #3) > Nope, that window is not an O-R window, it's a regular toplevel window (from > Pidgin) In that case, I see only too possibilites that we end up here: 1. gnome-shell accidentally sets NULL as the active workspace. 2. the session is saved before the pidgin window is destroyed, but after it is unmanaged or am I missing something?
Window is not being unmanaged (visible_to_compositor = 1, unmanaging = 0)
Review of attachment 367291 [details] [review]: (on_all_workspaces == TRUE && on_all_workspaces_requested == FALSE) means that the user didn't set the window to sticky, but the window is located on a non-primary monitor and the workspaces-only-on-primary setting is on. I don't think that saving the sticky state makes sense in that case, so the patch looks right to me. Still, only marking as "reviewed" in case Jonas disagrees (also it would be good for the commit message to mention what's going on - it currently sounds like a workaround for an error of unknown reason)
(In reply to Florian Müllner from comment #6) > Review of attachment 367291 [details] [review] [review]: > > (on_all_workspaces == TRUE && on_all_workspaces_requested == FALSE) means > that the user didn't set the window to sticky, but the window is located on > a non-primary monitor and the workspaces-only-on-primary setting is on. I acheckedthat as well, unfortunately workspaces-only-on-primary is off :( > I don't think that saving the sticky state makes sense in that case, so the > patch looks right to me. Still, only marking as "reviewed" in case Jonas > disagrees (also it would be good for the commit message to mention what's > going on - it currently sounds like a workaround for an error of unknown > reason) It is exactly like it sound, I haven't been able to either reproduce or trace back the code that leads to this (workspaces-only-on-primary was my first idea as well).
Even if it was located on the non-primary when workspace-only-on-primary was on, wouldn't restoring the window to some particular workspace result in an invalid state, where it is technically on all and one workspace at the same time, or does the restoring take care of that some how?
(In reply to Olivier Fourdan from comment #7) > (In reply to Florian Müllner from comment #6) > > Review of attachment 367291 [details] [review] [review] [review]: > > > > (on_all_workspaces == TRUE && on_all_workspaces_requested == FALSE) means > > that the user didn't set the window to sticky, but the window is located on > > a non-primary monitor and the workspaces-only-on-primary setting is on. > > I acheckedthat as well, unfortunately workspaces-only-on-primary is off :( Dang. Just to double-check, it's meta_prefs_get_workspaces_only_on_primary that returns FALSE, right? This is one of those settings that is picked up from different schemas depending on the session ... (In reply to Jonas Ådahl from comment #8) > Even if it was located on the non-primary when workspace-only-on-primary was > on, wouldn't restoring the window to some particular workspace result in an > invalid state, where it is technically on all and one workspace at the same > time, or does the restoring take care of that some how? It should work, as restoring the geometry results in a call to meta_window_move_resize_internal(), which calls meta_window_update_monitor(), which then calls meta_window_on_all_workspaces_changed() if the monitor changed. That function should either set on_all_workspaces xor the workspace. But then the whole session-saving code is mostly untested nowadays, so who knows for sure ...
I think _meta_window_shared_new() takes care of that case after calling meta_window_x11_manage() (which applies the session properties) by setting window->on_all_workspaces = should_be_on_all_workspaces();
(In reply to Florian Müllner from comment #9) > Dang. Just to double-check, it's meta_prefs_get_workspaces_only_on_primary > that returns FALSE, right? I asked for the actual settings: $ gsettings get org.gnome.mutter workspaces-only-on-primary > This is one of those settings that is picked up > from different schemas depending on the session ... Odd, I didn't see that in the code (src/core/prefs.c), how could I check that? > (In reply to Jonas Ådahl from comment #8) > > Even if it was located on the non-primary when workspace-only-on-primary was > > on, wouldn't restoring the window to some particular workspace result in an > > invalid state, where it is technically on all and one workspace at the same > > time, or does the restoring take care of that some how? > > It should work, as restoring the geometry results in a call to > meta_window_move_resize_internal(), which calls > meta_window_update_monitor(), which then calls > meta_window_on_all_workspaces_changed() if the monitor changed. That > function should either set on_all_workspaces xor the workspace. > > But then the whole session-saving code is mostly untested nowadays, so who > knows for sure ... Yeap, exactly my thought as well, considering this is off by default.
I guess gsettings get org.gnome.shell.overrides workspaces-only-on-primary ?
(In reply to Jonas Ådahl from comment #12) > I guess gsettings get org.gnome.shell.overrides workspaces-only-on-primary ? Yes, or gsettings get org.gnome.shell.extensions.classic-overrides workspaces-only-on-primary
Blimey! You're right, I got confused by the overrides, “org.gnome.shell.overrides workspaces-only-on-primary” is true. And the window is located on the secondary output (based on its location)
Ah bingo, reproduced! So the patch is correct (imho) but the commit message needs update.
Created attachment 367363 [details] [review] [PATCH v2] session: use initial workspace if no workspace set v2: Rephrase commit message Having “on_all_workspaces_requested” FALSE on a window does not imply a workspace is set. If the X11 window is placed on a secondary monitor while workspaces applies on primary monitor only (“workspaces-only-on-primary” set) then “on_all_workspaces_requested” is FALSE while “on_all_workspaces“ is TRUE and the associated workspace is NULL, leading to a crash when saving the gnome-shell/mutter session. So if no workspace is set, use the “initial_workspace” instead to avoid a NULL pointer dereference.
Review of attachment 367363 [details] [review]: Opps, nope, it crashes on restoring the session with this!
Review of attachment 367363 [details] [review]: Oh, my bad, it's not a crash but an issue with gnome-session (bug 790913) which prevents the session from retarting once it's been saved... So reverting the review status to "none", the patch works and fixes the crash and the session is restored correctly.
Review of attachment 367363 [details] [review]: lets land it then
Comment on attachment 367363 [details] [review] [PATCH v2] session: use initial workspace if no workspace set attachment 367363 [details] [review] pushed to git master as commit fbd5a74 - session: use initial workspace if no workspace set