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

wvkbd crashes when touching a key #1355

Open
realroot2185 opened this issue Mar 7, 2024 · 11 comments
Open

wvkbd crashes when touching a key #1355

realroot2185 opened this issue Mar 7, 2024 · 11 comments

Comments

@realroot2185
Copy link

I had this in catacomb.

[ 137267.693]  -> [email protected]_virtual_keyboard(wl_seat@10, new id zwp_virtual_keyboard_v1@14)
[ 137268.547]  -> [email protected](1, fd 5, 61419)
[ 140645.118]  -> [email protected](0, 0, 0, 0)
[ 140646.961]  -> [email protected](15023157, 21, 1)
[ 140647.852] [email protected](zwp_virtual_keyboard_v1@14, 0, "`modifiers` sent before keymap.")

Is it maybe possible for Smithay to remove trailing \0s in a keymap and strip them?
Or whatever fixes this.

@realroot2185
Copy link
Author

Is it a problem of wvkbd jjsullivan5196/wvkbd#35?

@chrisduerr
Copy link
Contributor

chrisduerr commented Mar 14, 2024

You should share the debug log output, the most important part is in the debug output of Smithay, which will tell you that the xkbcommon call failed.

@realroot2185
Copy link
Author

Do I need to do something or do I just check the system log?

@Quackdoc
Copy link

I have replicated this on Cosmic-comp and niri, this is my post from wvkbd below, Same thing applies I can grab squeekboard logs if necessary since it seems to work.


I added some fprintfs to make it easier to find since I wanted to take a look at this when I could too

Send Keymap <------- DEBUG latin

Send Modifers #LINENUM <------- DEBUG latin

seems to be this line https://github.com/jjsullivan5196/wvkbd/blob/8106d7606d5d0ea24d23b3f0fadeafe7e6e528f6/keyboard.c#L391

Keymap does seem to be sent first which is odd.

log.txt

EDIT: let me know if you want squeekboards logs, I need to reset my compositor so I havent provided them yet but can if needed.

EDIT2: looks like this was also reported to smithay so it seems like cosmic-comp might not be the only issue

@chrisduerr
Copy link
Contributor

@Quackdoc I don't think the issue is with the keymap being sent first. With debug logs included you should get an error that the keymap could not be loaded, since it's invalid. The reason why the "mods before keymap" error is emitted is because no valid keymap is ever received.

@Quackdoc
Copy link

Quackdoc commented Mar 31, 2024

Yeah I booted niri and cosmic-comp into nested mode and both of them spit this error

xkbcommon: ERROR: [XKB-769] (input string):1:61418: syntax error
xkbcommon: ERROR: Failed to parse input xkb string

Any more steps I can go to debug this aside from patching smithay to print out whatever this is?

@proycon
Copy link

proycon commented Mar 31, 2024

The offset will refer to this string: https://github.com/jjsullivan5196/wvkbd/blob/8106d7606d5d0ea24d23b3f0fadeafe7e6e528f6/keymap.mobintl.h#L9 . Your debug output showed:

-> [email protected](1, fd 5, 61419), the last int is the total length (strlen()) of that keymap string, so excluding any \0 bytes.

So I would guess 61418 is \n, perhaps xkbcommon stumbles over that trailing newline at the very end?? One could test by stripping it from wvkbd or trimming the string before sending to xkbcommon.

Wvkbd < 0.14 used to send too many \0 bytes at the end (jjsullivan5196/wvkbd#35), smithay was patched to cope with that in 7f8618c . Wvkbd was patched to fix the original problem and sends the size accurately now (without any \0 bytes).

@proycon
Copy link

proycon commented Mar 31, 2024

Looking at the xkbcommon-rs code, it looks like xkb::Keymap::new_from_fd() reduces the size by one before
passing to libxkbcommon's xkb_keymap_new_from_buffer()... so I think it just assumes the file descriptor holds the extra \0 byte and strips it, as xkb_keymap_new_from_buffer() explicitly states "takes a length argument so the input string does not have to be zero-terminated. the input string does not have to be zero-terminated". I think this may be a xkbcommon-rs bug in that case.

Wvkbd sends no \0 byte now so that would strip the newline (which I'd say would be fine too) but if my math is off or I'm missing some trimming somewhere it might strip the last semicolon, which indeed would be a clear syntax error.

Should wvkbd send a \0 byte in the keymap file descriptor or not? I'm inclined not to (current behaviour) because the size is provided explicitly (same reasoning as libxkbcommon). I'm not actually sure when reading the wayland specification. I'd rather see compositors be a bit flexible about it though.

Here's a patch for wvkbd you can try to see if it resolves the issue: jjsullivan5196/wvkbd@master...issue70 , but I think that may be patching the wrong thing (also considering other compositors like sway, wayfire, hyprland have no issues) and we'd better look at xkbcommon-rs (or I may be missing some handling in the middle in smithay itself).

@Quackdoc
Copy link

I can confirm the patch does indeed get wvkbd working on cosmic.

@drwankingstein
Copy link

@proycon is there something blocking that patch from being merged upstream? It would be nice to have support for it. or should this be forwarded to xkbcommon-rs?

@proycon
Copy link

proycon commented Nov 11, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants