Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[X11] Grab server while focusing window. #70625

Closed
wants to merge 1 commit into from

Conversation

lihop
Copy link

@lihop lihop commented Dec 27, 2022

Fixes #65425

More detail in #65425 (comment).

When handling the ConfigureNotify event, we get window attributes using XGetWindowAttributes and only call XSetInputFocus if the map_state attribute is IsViewable.

Occasionally, the map_state attribute was IsViewable but would become IsUnmapped by the time the XSetInputFocus request was received due to a request from another X server connection (e.g. the window manager). This would result in a BadMatch error and cause Godot to crash.

By calling XGrabServer before XGetWindowAttributes and XSetInputFocus we ensure that the X server will only receive requests from this connection and therefore the map_state attribute will not change until XUngrabServer is called.

When handling the ConfigureNotify event, we get window attributes using
XGetWindowAttributes and only call XSetInputFocus if the map_state
attribute is IsViewable.

Occasionally, the map_state attribute was IsViewable but would become
IsUnmapped by the time the XSetInputFocus request was receieved due
to a request from another X server connection (e.g. the window manager).
This would result in a BadMatch error and cause Godot to crash.

By calling XGrabServer before XGetWindowAttributes and XSetInputFocus we
ensure that the X server will only receive requests from this connection
and therefore the map_state attribute will not change until XUngrabServer
is called.

Fixes godotengine#65425
@lihop lihop requested a review from a team as a code owner December 27, 2022 09:10
@lihop lihop marked this pull request as draft December 27, 2022 10:00
@lihop
Copy link
Author

lihop commented Dec 27, 2022

I have converted this PR to draft as I am a little concerned about the wider impacts it may have.

The Xlib Programming Manual says in the XGrabServer section:

You should not grab the X server any more than is absolutely necessary.

And in the XUngrabServer section:

You should avoid grabbing the X server as much as possible.

I have just noticed that ConfigureNotify is called a lot of times (when dragging around a window for example):

configure-notify

So this patch would violate those suggestions as we only really care about calling XGrabServer in the specific case when the window is about to become unviewable.

Something I'm wondering is if we really need to call XSetInputFocus every time we handle ConfigureNotify with a viewable window, or if we could be a bit more selective about when we actually call it. If so, we might be able to avoid calling it in the case of the associated issue.

@mxnemu
Copy link

mxnemu commented Jan 14, 2023

Hey, thanks for locating the bug and testing it more. For some reason this PR still crashes for me. Strangely XGrabServer doesn't seem to help at all, when it should. I still get the same error.

Commenting out XSetInputFocus makes the crash go away, so it's definitely at that location.

@lihop
Copy link
Author

lihop commented Jan 14, 2023

Closing as this PR does not appear to fix the issue and @mxnemu has provided a suitable alternative patch in #65425 (comment).

@lihop lihop closed this Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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