Crash sometimes when window is hidden/minimized on Linux/X11/XMonad

[permelin]

Godot version

v4.0.alpha.custom_build [7a620e3]

System information

Debian Bookworm/Test, XMonad 0.17.0

Issue description

Both release builds and the editor are affected.

This is could very well be an XMonad specific race condition, but I've never had this issue with any other program. It does not happen with Godot 3.5.

X Error of failed request:  BadMatch (invalid parameter attributes)
  Major opcode of failed request:  42 (X_SetInputFocus)
  Serial number of failed request:  10683
  Current serial number in output stream:  10684

I will sometimes get this crash in either of these scenarios:

1. I switch to another workspace and the editor/game was the only window in the previous workspace.
2. The editor/game is maximized and I switch to a different maximized window, effectively hiding Godot.

The fix in #58891 ought to take care of this, but there seem to be a race condition where it's not enough. It added a check for IsViewable before calling XSetInputFocus but oddly enough the state is sometimes IsViewable for me even when switching away from Godot, and that's when I get the crash.

godot/platform/linuxbsd/display_server_x11.cpp

Lines 3686 to 3688 in 0f62e35

	if ((xwa.map_state == IsViewable) && !wd.no_focus && !wd.is_popup) {
	XSetInputFocus(x11_display, wd.x11_window, RevertToPointerRoot, CurrentTime);
	}

Steps to reproduce

1. Use XMonad window manager.
2. Open the editor (or built game) as the sole window on a workspace.
3. Switch away from, and then back to, this workspace until it crashes. Sometimes it crashes immediately, sometimes it takes 20 switches.

Minimal reproduction project

No response

[mxnemu]

I can confirm this happens in beta1 on Xmonad (Ubuntu 20.04).

Super quick to reproduce by holding Alt+Tab in the Fullscreen layout with a second window. Also happens in the project manager window that you get at the start.

[Evimilly]

Tried both methods, can't reproduce.
Archlinux XMonad 0.17.1-48, Godot beta 4

[Calinou]

Can you reproduce this when starting the editor with the --single-window command line argument? Note that starting the project manager with this argument won't carry it over to the editor, so you have to enable the Single Window Mode editor setting instead if you're going through the project manager.

[permelin]

Can you reproduce this when starting the editor with the --single-window command line argument?

Yes. Still crashes.

I'm on beta 4 and XMonad 0.17.1.

[mxnemu]

I tested again with the default xmonad config and it wouldn't crash if you have borders on fullscreen windows.

I need lessborders (no borders for fullscreen windows) to be active to reproduce this.
Here is the minimal ~/xmonad/xmonad.hs config:

import XMonad                                                                                                                            
import XMonad.Layout.NoBorders                                                                                                           
                                                                                                                                         
main = do                                                                                                                                
  xmonad (defaultConfig {layoutHook = lessBorders Screen Full})                                                                          

[permelin]

I tested again with the default xmonad config and it wouldn't crash if you have borders on fullscreen windows.

Same here.

I'm normally using smartBorders which removes the border if there is only one window visible. I tried removing that config and the crash no longer happened.

Rémi, please don't close this issue. This is not a fix since it also affects games built with Godot 4. Preferably, we don't want our games to crash because of some common user setting that is out of our control.

[lihop]

I can confirm the issue with Godot 4 beta 10 and an XMonad config with smartBorders enabled. I have added some additional X11 debug log output along with an explanation of how this is triggered by XMonad's smartBorders feature, and a possible fix.

Summary

• When smartBorders are enabled XMonad will re-add borders to a maximized window immediately before switching to another workspace or different maximized window in the same workspace.
• This re-adding of the border triggers a ConfigureNotify event, which is handled by Godot in the following code:
  

  godot/platform/linuxbsd/x11/display_server_x11.cpp

  Lines 3910 to 3927 in d0398f6

  	case ConfigureNotify: {
  	DEBUG_LOG_X11("[%u] ConfigureNotify window=%lu (%u), event=%lu, above=%lu, override_redirect=%u \n", frame, event.xconfigure.window, window_id, event.xconfigure.event, event.xconfigure.above, event.xconfigure.override_redirect);
  	const WindowData &wd = windows[window_id];
  	XWindowAttributes xwa;
  	XSync(x11_display, False);
  	XGetWindowAttributes(x11_display, wd.x11_window, &xwa);
  	// Set focus when menu window is re-used.
  	// RevertToPointerRoot is used to make sure we don't lose all focus in case
  	// a subwindow and its parent are both destroyed.
  	if ((xwa.map_state == IsViewable) && !wd.no_focus && !wd.is_popup) {
  	XSetInputFocus(x11_display, wd.x11_window, RevertToPointerRoot, CurrentTime);
  	}
  	_window_changed(&event);
  	} break;

  
  Godot will get the window attributes using XGetWindowAttributes and (thanks to [X11] Do not try to focus unmapped window. #58891) only call XSetInputFocus if the map_state attribute is IsViewable.
• However, due to a race condition, sometimes map_state will be IsViewable after calling XGetWindowAttributes but change to IsUnmapped by the time XSetInputFocus is requested.
• Grabbing the server using XGrabServer disables processing of requests from other connections (i.e. XMonad), ensuring that map_state does not change between XGetWindowAttributes and XSetInputFocus.

Debugging

In order to debug this issue I recompiled the engine with the following patch:

diff --git a/platform/linuxbsd/x11/display_server_x11.cpp b/platform/linuxbsd/x11/display_server_x11.cpp
index 57f125a754..64055d9f27 100644
--- a/platform/linuxbsd/x11/display_server_x11.cpp
+++ b/platform/linuxbsd/x11/display_server_x11.cpp
@@ -78,7 +78,7 @@
 #define VALUATOR_TILTX 3
 #define VALUATOR_TILTY 4
 
-//#define DISPLAY_SERVER_X11_DEBUG_LOGS_ENABLED
+#define DISPLAY_SERVER_X11_DEBUG_LOGS_ENABLED
 #ifdef DISPLAY_SERVER_X11_DEBUG_LOGS_ENABLED
 #define DEBUG_LOG_X11(...) printf(__VA_ARGS__)
 #else
@@ -3762,6 +3762,9 @@ void DisplayServerX11::process_events() {
                                XSync(x11_display, False);
                                XGetWindowAttributes(x11_display, wd.x11_window, &xwa);
 
+                               printf("Border width: %d, Map state: %d (%s)\n", xwa.border_width, xwa.map_state,
+                                       xwa.map_state == 0 ? "IsUnmapped" : xwa.map_state == 1 ? "IsUnviewable" : "IsViewable");
+
                                // Set focus when menu window is started.
                                // RevertToPointerRoot is used to make sure we don't lose all focus in case
                                // a subwindow and its parent are both destroyed.
@@ -3916,6 +3919,9 @@ void DisplayServerX11::process_events() {
                                XSync(x11_display, False);
                                XGetWindowAttributes(x11_display, wd.x11_window, &xwa);
 
+                               printf("Border width: %d, Map state: %d (%s)\n", xwa.border_width, xwa.map_state,
+                                       xwa.map_state == 0 ? "IsUnmapped" : xwa.map_state == 1 ? "IsUnviewable" : "IsViewable");
+
                                // Set focus when menu window is re-used.
                                // RevertToPointerRoot is used to make sure we don't lose all focus in case
                                // a subwindow and its parent are both destroyed.

Here are the logs with smartBorders disabled when switching between workspaces:
Keycode 108 is the mod key I use for XMonad.

[135] KeyPress window=73400324 (0), keycode=108, time=46749563
[146] FocusOut window=73400324 (0), mode='1'
[147] FocusOut window=73400324 (0), mode='3'
[147] LeaveNotify window=73400324 (0), mode='0'
All focus lost, triggering NOTIFICATION_APPLICATION_FOCUS_OUT
[251] MapNotify window=73400324 (0)
Border width: 1, Map state: 2 (IsViewable)
[251] VisibilityNotify window=73400324 (0), state=0
[251] Expose window=73400324 (0), count='0'
[251] EnterNotify window=73400324 (0), mode='0'
[251] FocusIn window=73400324 (0), mode='3'
[263] FocusOut window=73400324 (0), mode='2'
[263] FocusIn window=73400324 (0), mode='2'
[291] KeyRelease window=73400324 (0), keycode=108, time=46751108

The ConfigureNotify event is never emitted so the issue doesn't occur.

Here are the logs with smartBorders enabled, when things work:

[249] KeyPress window=73400324 (0), keycode=108, time=46594485
[276] FocusOut window=73400324 (0), mode='1'
[276] ConfigureNotify window=73400324 (0), event=73400324, above=73400322, override_redirect=0
Border width: 1, Map state: 0 (IsUnmapped)
[276] VisibilityNotify window=73400324 (0), state=1
[276] FocusOut window=73400324 (0), mode='3'
[276] LeaveNotify window=73400324 (0), mode='0'
All focus lost, triggering NOTIFICATION_APPLICATION_FOCUS_OUT
[560] ConfigureNotify window=73400324 (0), event=73400324, above=73400322, override_redirect=0
Border width: 0, Map state: 2 (IsViewable)
[560] MapNotify window=73400324 (0)
[560] VisibilityNotify window=73400324 (0), state=0
[560] Expose window=73400324 (0), count='0'
[560] EnterNotify window=73400324 (0), mode='0'
[560] FocusIn window=73400324 (0), mode='3'
[567] FocusOut window=73400324 (0), mode='2'
[567] FocusIn window=73400324 (0), mode='2'
[674] KeyRelease window=73400324 (0), keycode=108, time=46598802

As can be seen by the border_width attribute, XMonad is adding and removing the border when switching. This triggers the ConfigureNotify event. However, map_state is IsUnmapped so XSetInputFocus is never called and the issue doesn't occur.

When things go wrong:

[1433] KeyPress window=73400324 (0), keycode=108, time=46418057
[1442] FocusOut window=73400324 (0), mode='1'
[1442] ConfigureNotify window=73400324 (0), event=73400324, above=33554438, override_redirect=0
Border width: 1, Map state: 2 (IsViewable)
X Error of failed request:  BadMatch (invalid parameter attributes)
  Major opcode of failed request:  42 (X_SetInputFocus)
  Serial number of failed request:  7456
  Current serial number in output stream:  7457

In this case, map_state is IsViewable so XSetInputFocus is called. However, by the time the XSetInputFocus request is received, the window is now unmapped, so the request fails with the BadMatch error resulting in a crash.

Fix

The following patch disables processing of requests from other connections (i.e. XMonad), ensuring that the map_state attribute will not change until after XSetInputFocus is called:

diff --git a/platform/linuxbsd/x11/display_server_x11.cpp b/platform/linuxbsd/x11/display_server_x11.cpp
index 57f125a754..d989712917 100644
--- a/platform/linuxbsd/x11/display_server_x11.cpp
+++ b/platform/linuxbsd/x11/display_server_x11.cpp
@@ -3914,6 +3914,9 @@ void DisplayServerX11::process_events() {
 
                                XWindowAttributes xwa;
                                XSync(x11_display, False);
+
+                               // Grab the server to avoid map_state changes between calling XGetWindowAttributes and XSetInputFocus.
+                               XGrabServer(x11_display);
                                XGetWindowAttributes(x11_display, wd.x11_window, &xwa);
 
                                // Set focus when menu window is re-used.
@@ -3922,6 +3925,7 @@ void DisplayServerX11::process_events() {
                                if ((xwa.map_state == IsViewable) && !wd.no_focus && !wd.is_popup) {
                                        XSetInputFocus(x11_display, wd.x11_window, RevertToPointerRoot, CurrentTime);
                                }
+                               XUngrabServer(x11_display); // Very important to ungrab the server otherwise X11 becomes unresponsive.
 
                                _window_changed(&event);
                        } break;

map_state being IsViewable when switching away from Godot happens at roughly the same rate, however, the BadMatch error and subsequent crash no longer occurs.

Logs after applying this patch:

• When map_state is IsViewable (previously crashed):

  [15874] KeyPress window=23068676 (0), keycode=108, time=63483199 
  [15881] FocusOut window=23068676 (0), mode='1' 
  [15881] ConfigureNotify window=23068676 (0), event=23068676, above=81788934, override_redirect=0 
  Border width: 1, Map state: 2 (IsViewable)
  [15881] VisibilityNotify window=23068676 (0), state=1 
  [15882] VisibilityNotify window=23068676 (0), state=2 
  [15882] LeaveNotify window=23068676 (0), mode='0' 
  [15882] FocusOut window=23068676 (0), mode='3' 
  All focus lost, triggering NOTIFICATION_APPLICATION_FOCUS_OUT
  [16208] ConfigureNotify window=23068676 (0), event=23068676, above=81788934, override_redirect=0 
  Border width: 0, Map state: 2 (IsViewable)
  [16208] MapNotify window=23068676 (0) 
  Border width: 0, Map state: 2 (IsViewable)
  [16208] VisibilityNotify window=23068676 (0), state=2 
  [16208] FocusIn window=23068676 (0), mode='3' 
  [16208] VisibilityNotify window=23068676 (0), state=0 
  [16208] Expose window=23068676 (0), count='0' 
  [16208] EnterNotify window=23068676 (0), mode='0' 
  [16214] FocusOut window=23068676 (0), mode='2' 
  [16214] FocusIn window=23068676 (0), mode='2' 
  [16216] KeyRelease window=23068676 (0), keycode=108, time=63485785 
  

• When map_state is IsUnmapped (previously worked):

  [15750] KeyPress window=23068676 (0), keycode=108, time=63481986 
  [15761] FocusOut window=23068676 (0), mode='1' 
  [15761] ConfigureNotify window=23068676 (0), event=23068676, above=81788934, override_redirect=0 
  Border width: 1, Map state: 0 (IsUnmapped)
  [15761] VisibilityNotify window=23068676 (0), state=1 
  [15761] VisibilityNotify window=23068676 (0), state=2 
  [15761] LeaveNotify window=23068676 (0), mode='0' 
  [15761] FocusOut window=23068676 (0), mode='3' 
  All focus lost, triggering NOTIFICATION_APPLICATION_FOCUS_OUT
  [15855] ConfigureNotify window=23068676 (0), event=23068676, above=81788934, override_redirect=0 
  Border width: 0, Map state: 2 (IsViewable)
  [15855] MapNotify window=23068676 (0) 
  Border width: 0, Map state: 2 (IsViewable)
  [15855] VisibilityNotify window=23068676 (0), state=2 
  [15855] FocusIn window=23068676 (0), mode='3' 
  [15855] VisibilityNotify window=23068676 (0), state=0 
  [15855] Expose window=23068676 (0), count='0' 
  [15855] EnterNotify window=23068676 (0), mode='0' 
  [15861] FocusOut window=23068676 (0), mode='2' 
  [15861] FocusIn window=23068676 (0), mode='2' 
  [15865] KeyRelease window=23068676 (0), keycode=108, time=63483031
  

[mxnemu]

Grabbing the XServer does still crash for me for some reason.

One other way to work around this is to set a default error handler with XSetErrorHandler when X is initialized.
Setting a local handler before the XSetInputFocus call and resetting to the old handler at the end of the function (like it is done in some other places) doesn't work for some reason, the handler won't trigger.

Since this is an invalid call during an race-condition state and another valid call comes in just after/before it when the border modified again, it should be fair to just ignore the X error and move on.

Here is a patch default_error_handler.patch (.txt because github won't let me upload patch files for some reason atm)

Not crashing on X errors by default is probably better for users imo. So even if things get weird from an unhandled error they might still save their changes and restart.

But maybe there is a better way to print the error So that it looks like the default X error again.

[lihop]

Thanks for testing my PR @mxnemu. I can confirm your patch also prevents crashing for me.

I think it's also worth linking this issue to #68305 (and possibly #68572) as it also involves problems caused by calls to XSetInputFocus in response to events from the window manager (e.g. ConfigureNotify).

[mxnemu]

If anyone wants to improve that patch's error logging and make a PR, feel free to do so. I'm too busy atm and probably won't submit one.

[mxnemu]

I tested if #69995 fixes this bug, since it changes a lot of the X11 code and adds an x11fixes wrapper but I still get the bug.

I think lihop had the best diagnosis so far.

Anyway #75099 is a functioning workaround and I hope it can get merged soon.

[permelin]

This problem has been greatly mitigated by #75099. Now, instead of crashing when switching workspaces I just get an error in the editor console. That's mildly annoying but obviously much better than a crash. Thank you @mxnemu for this workaround.

I assume this is the best we can hope for so I'm closing the issue.