BadAccess (X_GrabKey) when registering combination is obtained by others

[ventsislav-georgiev]

Hotkey register causes the app to crash with the following output:

ventsi@localhost:/media/sf_prosper> go run app.go 
X Error of failed request:  BadAccess (attempt to access private resource denied)
  Major opcode of failed request:  33 (X_GrabKey)
  Serial number of failed request:  10
  Current serial number in output stream:  11
exit status 1

Commenting out the code to register hotkey fixes the issue:

This is openSUSE Leap 15.3 running in VirtualBox.

[changkun]

I think this is a known issue which not been easy to fix for quite a long time. The problem is the registering combination was obtained by other applications and cause GrabKey's BadAccess.

A previous investigation seems to suggest a better error handling, but there are very rare resources really document this, becomes the last thing I remember

[ventsislav-georgiev]

Looking at the code of entrall, the approach there seems to solve the issue.
Here is a patch which after applied fixes the startup of the app:

From aef3451685d49b918f57f99a9683a9f615807bfa Mon Sep 17 00:00:00 2001
From: ventsislav-georgiev <[email protected]>
Date: Wed, 5 Jan 2022 00:51:26 +0200
Subject: [PATCH] feat(linux): grabkey bad access err handler

---
 hotkey_linux.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/hotkey_linux.c b/hotkey_linux.c
index 0cb8b1c..b83b4cc 100644
--- a/hotkey_linux.c
+++ b/hotkey_linux.c
@@ -27,27 +27,21 @@ int displayTest() {
 	return 0;
 }
 
-// FIXME: handle bad access properly.
-// int handleErrors( Display* dpy, XErrorEvent* pErr )
-// {
-//     printf("X Error Handler called, values: %d/%lu/%d/%d/%d\n",
-//         pErr->type,
-//         pErr->serial,
-//         pErr->error_code,
-//         pErr->request_code,
-//         pErr->minor_code );
-//     if( pErr->request_code == 33 ){  // 33 (X_GrabKey)
-//         if( pErr->error_code == BadAccess ){
-//             printf("ERROR: key combination already grabbed by another client.\n");
-//             return 0;
-//         }
-//     }
-//     return 0;
-// }
+static int keygrab_err;
+
+static int xerr_keygrab(Display* d, XErrorEvent* xev)
+{
+	if (!keygrab_err)
+		keygrab_err = xev->error_code;
+	return 0;
+}
+
 
 // waitHotkey blocks until the hotkey is triggered.
 // this function crashes the program if the hotkey already grabbed by others.
 int waitHotkey(uintptr_t hkhandle, unsigned int mod, int key) {
+	int (*prev_errhandler)(Display*, XErrorEvent*);
+
 	Display* d = NULL;
 	for (int i = 0; i < 42; i++) {
 		d = XOpenDisplay(0);
@@ -57,8 +51,17 @@ int waitHotkey(uintptr_t hkhandle, unsigned int mod, int key) {
 	if (d == NULL) {
 		return -1;
 	}
+
+	// ref: https://github.com/zevweiss/enthrall/blob/d24c5a376fa91efc38901d94adfd4b4c1ee9c85b/x11.c#L162
+	keygrab_err = 0;
+	prev_errhandler = XSetErrorHandler(xerr_keygrab);
+
 	int keycode = XKeysymToKeycode(d, key);
 	XGrabKey(d, keycode, mod, DefaultRootWindow(d), False, GrabModeAsync, GrabModeAsync);
+
+	if (keygrab_err)
+		goto out;
+
 	XSelectInput(d, DefaultRootWindow(d), KeyPressMask);
 	XEvent ev;
 	while(1) {
@@ -74,4 +77,7 @@ int waitHotkey(uintptr_t hkhandle, unsigned int mod, int key) {
 			return 0;
 		}
 	}
-}
\ No newline at end of file
+
+out:
+	XSetErrorHandler(prev_errhandler);
+}
-- 
2.32.0 (Apple Git-132)

If we also add a way to return that the register failed and it will work perfectly.

[changkun]

This reminds me the key issue.

Note that the mentioned approach must use a static global error code. Meaning that we need a proper way to deal with concurrent registration to make sure the error code is properly assessed from Go side.

[ventsislav-georgiev]

That would be easy to ensure from the client of the package and can be just a documentation for Linux in hotkey for now.

[changkun]

That would be easy to ensure from the client of the package and can be just documentation for Linux in hotkey for now.

Documenting a proper use seems to be less desired as we should have a better way to make the behavior consistent across platforms. mainthread.Init is already a thing that breaks the design goal whereas we don't have any other choices. Similar to darwin/windows that removes the awareness of concurrent registration towards an easy-to-use principle for the user.

[ventsislav-georgiev]

On another thought, making concurrent registrations is not really a valuable feature. It doesn't hurt UX for a GUI app and also there won't be that many hotkey registrations in general to bare any performance difference. So we can make that concurrent -> force sequential registrations in the hotkey_linux.go.

[changkun]

making concurrent registrations is not really a valuable feature

Note that a goroutine may be scheduled into different goroutines. It is impossible to consistently call the registration from the same thread unless all registration calls are issued in the same goroutine and runtime.LockOSThread is activated.

An early attempt suggests that even serialized event handler may not capture corresponding grab key error, the reason is not clear.

[ventsislav-georgiev]

It is impossible to consistently call the registration from the same thread

That is not necessary. In my understanding of the code, it doesn't matter which thread calls into the cgo register method. Correct me if I am wrong, but isn't the static error code the the actual issue? We just need to make sure that no other call to the cgo register method is invoked in parallel before a previous one has completed.

[changkun]

A PR that makes the multiple example work and returns the error properly would be interesting and welcome.

[ventsislav-georgiev]

It can't be done the way I thought. The XSetErrorHandler sets global handler, then XGrabKey sends a message for the window to get the key combination events. However, there is no way to know when the X11 server will actually handle the message and send us the error. There also isn't any way to get which key grab failed from the provided error.

I think we just need to handle the GrabKey error and return it to the user in some global error handler (optional to use, not for specific hotkey).

One more thing about the Linux implementation. I think the code should be refactored to have a single wait event loop, not one per hotkey. This seems like a waste of OS threads. We can have a single loop and extract the key and modifiers from the event.

[changkun]

Thanks for trying out. Yes, and as noted, the key issue is that the event handler is a global handler and we cannot do it per goroutines unless we centralize everything in the same Display loop. As documented in the existing code (same as windows), a unified event loop is ideal but surely needs more work.