Page MenuHomeFreeBSD

x11/libxfce4menu: Fix leaked keygrabs when layout changes
ClosedPublic

Authored by cem on Apr 8 2020, 3:53 PM.

Details

Summary

As diagnosed by Jethro Nederhof, xfce-shortcuts-grabber.c attempts to update
grabbed key shortcuts when xkeyboard layout changes. Unfortunately, it had no
memory of which keycodes it has actually grabbed. Instead, it attempted to
ungrab the *new* keycode, which obviously doesn't actually ungrab those codes.

This went unnoticed for some time, probably because nothing collided with
important keys. Recently, a default PrintScreen shortcut was added to Xfce,
which for whatever reason seems to collide with Up in initial layout. When the
kbd layout changes, the shortcut ungrabs the *new* Printscreen keycode and then
re-grabs the same keycode, leaving the Up keycode grabbed.

Fix this by giving xfce-shortcuts-grabber some memory of which keycodes it has
grabbed. When it grabs a key, it remembers the keycode it grabbed in the
XfceKey object. When it ungrabs a key, it ungrabs the keycodes in the XfceKey
object, rather than those for the new keyboard layout.

PR: 244290
Reported by: Aryeh Friedman <aryeh.friedman AT gmail.com>, many others
XDifferential Revision: https://reviews.freebsd.org/D24338

Test Plan

It fixed 'Up' on my system.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem created this revision.Apr 8 2020, 3:53 PM
cem edited the summary of this revision. (Show Details)Apr 8 2020, 3:58 PM

Hi,

Thanks for this patch. Looks like a good idea.

While I'm not experiencing this issue, I'm going to test it, to check no regression happens.

Have you already proposed this patch upstream? Will you file a bug report upstream to push this or would rather me doing this on your behalf? (giving due credit, obviously)

cem added a comment.EditedApr 8 2020, 4:46 PM

Thanks for this patch. Looks like a good idea.

Thanks!

While I'm not experiencing this issue, I'm going to test it, to check no regression happens.

Great, thanks!

Have you already proposed this patch upstream? Will you file a bug report upstream to push this or would rather me doing this on your behalf? (giving due credit, obviously)

I have not sent this upstream yet. I would love it if you would do so — I wrote this in a fashion to minimize the diff for ports, but the right way to do it upstream is probably more of a refactoring. Plus I'm sure I violated upstream C style guidelines. Please be sure to credit Jethro for the analysis in the PR, it really pinpointed the problem.

madpilot accepted this revision.Apr 12 2020, 8:07 PM

I have been running this patch and see no ill effect. You can commit it.

I'll also take some time to rework it and send it upstream.

This revision is now accepted and ready to land.Apr 12 2020, 8:07 PM
This revision was automatically updated to reflect the committed changes.