Bug #45385 related to keyboard and probably wineserver - where to start the search

[John Found]

I just reported bug #45385 (https://bugs.winehq.org/show_bug.cgi?id=45385) and want to try to fix it.

So I want to ask about some preliminary directions - where to check the code, 
what is the general structure of the code related to the bug subject, possible suspicious places.

Here is the full bug report in order to save you a visit to the bug tracker:
> I noticed that the state of the keys sometimes sticks in pressed state.
>
> This happens when cycling windows with some shortcut key combination.
>
> For example if cycling with Alt+Tab, on pressing Alt, the program gets WM_KEYDOWN and the state of the VK_MENU becomes pressed. But after cycling windows, the program does not get WM_KEYUP because the window is not focused and VK_MENU (and the respective VK_LMENU or VK_RMENU) remain in pressed state.
>
> When cycling back to the program window, the window get focused only after releasing Alt key, so it does not get this event as well.
>
> If cycling windows with another shortcut key combination (for example Alt+Shift+Tab - for backward cycling) both VK_MENU and VK_SHIFT keys stick.
>
> In the same time, GetAsyncKeyState returns the proper state of the keys.
>
> Note1: The problem is obviously in the wineserver code, because it handles the key state tables for the different threads.
>
> Note2: The effect happens only sometimes. It seems the code for proper processing is already there, but some racing conditions have place.
>
> Note3: There is some probability that the effect is in result of my application code, but it never happens on real Windows, so I considered it a bug.
>
> Note4: I tried to workaround this problem by reading the whole table by GetAsyncKeyState and setting it then with SetKeyboardState on WM_ACTIVATE message of the main window. This workaround actually works, but is too ugly IMO.
> The same trick on WM_ACTIVATEAPP does not work.

[John Found]

While reading the code in wineX11.drv/keyboard.c I noticed the following fragment from the function X11DRV_KeymapNotify:

for (vkey = VK_LSHIFT; vkey <= VK_RMENU; vkey++)
    {
        int m = vkey - VK_LSHIFT;
> > > if (modifiers[m].vkey && !(keystate[vkey] & 0x80) != !modifiers[m].pressed)

{
            TRACE( "Adjusting state for vkey %#.2x. State before %#.2x\n",
                   modifiers[m].vkey, keystate[vkey]);

update_key_state( keystate, vkey, modifiers[m].pressed );
            changed = TRUE;
        }
    }

Can someone explain what the condition pointed by >>> is doing? (Unfortunately, my C skills are pretty low...)

Why not simply "(modifiers[m].vkey && (keystate[vkey] & 0x80) != modifiers[m].pressed)"?

And why is modifiers[m].vkey included in the condition?

Best Regards

On Wed, 27 Jun 2018 08:50:22 +0300
John Found  wrote:
> I just reported bug #45385 (https://bugs.winehq.org/show_bug.cgi?id=45385) and want to try to fix it.
>
> So I want to ask about some preliminary directions - where to check the code, 
> what is the general structure of the code related to the bug subject, possible suspicious places.
>
> Here is the full bug report in order to save you a visit to the bug tracker:
> > I noticed that the state of the keys sometimes sticks in pressed state.
> >
> > This happens when cycling windows with some shortcut key combination.
> >
> > For example if cycling with Alt+Tab, on pressing Alt, the program gets WM_KEYDOWN and the state of the VK_MENU becomes pressed. But after cycling windows, the program does not get WM_KEYUP because the window is not focused and VK_MENU (and the respective VK_LMENU or VK_RMENU) remain in pressed state.
> >
> > When cycling back to the program window, the window get focused only after releasing Alt key, so it does not get this event as well.
> >
> > If cycling windows with another shortcut key combination (for example Alt+Shift+Tab - for backward cycling) both VK_MENU and VK_SHIFT keys stick.
> >
> > In the same time, GetAsyncKeyState returns the proper state of the keys.
> >
> > Note1: The problem is obviously in the wineserver code, because it handles the key state tables for the different threads.
> >
> > Note2: The effect happens only sometimes. It seems the code for proper processing is already there, but some racing conditions have place.
> >
> > Note3: There is some probability that the effect is in result of my application code, but it never happens on real Windows, so I considered it a bug.
> >
> > Note4: I tried to workaround this problem by reading the whole table by GetAsyncKeyState and setting it then with SetKeyboardState on WM_ACTIVATE message of the main window. This workaround actually works, but is too ugly IMO.
> > The same trick on WM_ACTIVATEAPP does not work.

[Alex Henrie]

On Wed, Jun 27, 2018 at 11:24 AM John Found  wrote:
> While reading the code in wineX11.drv/keyboard.c I noticed the following fragment from the function X11DRV_KeymapNotify:
> for (vkey = VK_LSHIFT; vkey <= VK_RMENU; vkey++)
> {
>     int m = vkey - VK_LSHIFT;
> > > > if (modifiers[m].vkey && !(keystate[vkey] & 0x80) != !modifiers[m].pressed)
>     {
>         TRACE( "Adjusting state for vkey %#.2x. State before %#.2x\n",
>                modifiers[m].vkey, keystate[vkey]);
>
>         update_key_state( keystate, vkey, modifiers[m].pressed );
>         changed = TRUE;
>     }
> }
>
> Can someone explain what the condition pointed by >>> is doing? (Unfortunately, my C skills are pretty low...)
>
> Why not simply "(modifiers[m].vkey && (keystate[vkey] & 0x80) != modifiers[m].pressed)"?

I don't immediately know what this if statement is for, but I can tell
you that the ! operator converts a value of zero to 1 and a nonzero
value to 0. So, if keystate[vkey] & 0x80 is 0 then
modifiers[m].pressed must be nonzero and vice versa.

-Alex

[Kai Krakow]

Hello!

2018-06-27 21:14 GMT+02:00 Alex Henrie :
> On Wed, Jun 27, 2018 at 11:24 AM John Found  wrote:
> > While reading the code in wineX11.drv/keyboard.c I noticed the following fragment from the function X11DRV_KeymapNotify:
> > for (vkey = VK_LSHIFT; vkey <= VK_RMENU; vkey++)
> > {
> >     int m = vkey - VK_LSHIFT;
> > > > > if (modifiers[m].vkey && !(keystate[vkey] & 0x80) != !modifiers[m].pressed)
> >     {
> >         TRACE( "Adjusting state for vkey %#.2x. State before %#.2x\n",
> >                modifiers[m].vkey, keystate[vkey]);
> >
> >         update_key_state( keystate, vkey, modifiers[m].pressed );
> >         changed = TRUE;
> >     }
> > }
> >
> > Can someone explain what the condition pointed by >>> is doing? (Unfortunately, my C skills are pretty low...)
> >
> > Why not simply "(modifiers[m].vkey && (keystate[vkey] & 0x80) != modifiers[m].pressed)"?
>
> I don't immediately know what this if statement is for, but I can tell
> you that the ! operator converts a value of zero to 1 and a nonzero
> value to 0. So, if keystate[vkey] & 0x80 is 0 then
> modifiers[m].pressed must be nonzero and vice versa.

I always was under the impression that the bang operator does a binary
inversion of the value. Thus, it would convert 0x7F to 0x80. This also
means it would convert 0x00 (signed) to 0xFF (signed) which turns 0
into -1, not 1.

But however it works, this code is not working in any obvious way and
should maybe be rewritten.

Just my two cents.

Regards,
Kai

[Kai Krakow]

Hello again!

2018-06-28 8:56 GMT+02:00 Kai Krakow :
> Hello!
>
> 2018-06-27 21:14 GMT+02:00 Alex Henrie :
> > On Wed, Jun 27, 2018 at 11:24 AM John Found  wrote:
> > > While reading the code in wineX11.drv/keyboard.c I noticed the following fragment from the function X11DRV_KeymapNotify:
> > > for (vkey = VK_LSHIFT; vkey <= VK_RMENU; vkey++)
> > > {
> > >     int m = vkey - VK_LSHIFT;
> > > > > > if (modifiers[m].vkey && !(keystate[vkey] & 0x80) != !modifiers[m].pressed)
> > >     {
> > >         TRACE( "Adjusting state for vkey %#.2x. State before %#.2x\n",
> > >                modifiers[m].vkey, keystate[vkey]);
> > >
> > >         update_key_state( keystate, vkey, modifiers[m].pressed );
> > >         changed = TRUE;
> > >     }
> > > }
> > >
> > > Can someone explain what the condition pointed by >>> is doing? (Unfortunately, my C skills are pretty low...)
> > >
> > > Why not simply "(modifiers[m].vkey && (keystate[vkey] & 0x80) != modifiers[m].pressed)"?
> >
> > I don't immediately know what this if statement is for, but I can tell
> > you that the ! operator converts a value of zero to 1 and a nonzero
> > value to 0. So, if keystate[vkey] & 0x80 is 0 then
> > modifiers[m].pressed must be nonzero and vice versa.
>
> I always was under the impression that the bang operator does a binary
> inversion of the value. Thus, it would convert 0x7F to 0x80. This also
> means it would convert 0x00 (signed) to 0xFF (signed) which turns 0
> into -1, not 1.
>
> But however it works, this code is not working in any obvious way and
> should maybe be rewritten.
>
> Just my two cents.

I just looked it up, the bang operator seems to be a logical (not
binary) operator in C99:
https://stackoverflow.com/questions/3661751/what-is-0-in-c

According to this, I question how the code above is supposed to work
in the first place because "!modifiers[m].pressed" would always be 0
or 1, and it is compared to 0 or 0x80. This is exploiting implicit
behavior which is almost always a bad thing to do if you need to
understand code later.

Regards,
Kai

[Matteo Bruni]

2018-06-28 9:05 GMT+02:00 Kai Krakow :
> Hello again!
>
> 2018-06-28 8:56 GMT+02:00 Kai Krakow :
> > Hello!
> >
> > 2018-06-27 21:14 GMT+02:00 Alex Henrie :
> > > On Wed, Jun 27, 2018 at 11:24 AM John Found  wrote:
> > > > While reading the code in wineX11.drv/keyboard.c I noticed the following fragment from the function X11DRV_KeymapNotify:
> > > > for (vkey = VK_LSHIFT; vkey <= VK_RMENU; vkey++)
> > > > {
> > > >     int m = vkey - VK_LSHIFT;
> > > > > > >     if (modifiers[m].vkey && !(keystate[vkey] & 0x80) != !modifiers[m].pressed)
> > > >     {
> > > >         TRACE( "Adjusting state for vkey %#.2x. State before %#.2x\n",
> > > >                modifiers[m].vkey, keystate[vkey]);
> > > >
> > > >         update_key_state( keystate, vkey, modifiers[m].pressed );
> > > >         changed = TRUE;
> > > >     }
> > > > }
> > > >
> > > > Can someone explain what the condition pointed by >>> is doing? (Unfortunately, my C skills are pretty low...)
> > > >
> > > > Why not simply "(modifiers[m].vkey && (keystate[vkey] & 0x80) != modifiers[m].pressed)"?
> > >
> > > I don't immediately know what this if statement is for, but I can tell
> > > you that the ! operator converts a value of zero to 1 and a nonzero
> > > value to 0. So, if keystate[vkey] & 0x80 is 0 then
> > > modifiers[m].pressed must be nonzero and vice versa.
> >
> > I always was under the impression that the bang operator does a binary
> > inversion of the value. Thus, it would convert 0x7F to 0x80. This also
> > means it would convert 0x00 (signed) to 0xFF (signed) which turns 0
> > into -1, not 1.
> >
> > But however it works, this code is not working in any obvious way and
> > should maybe be rewritten.
> >
> > Just my two cents.
>
> I just looked it up, the bang operator seems to be a logical (not
> binary) operator in C99:
> https://stackoverflow.com/questions/3661751/what-is-0-in-c
>
> According to this, I question how the code above is supposed to work
> in the first place because "!modifiers[m].pressed" would always be 0
> or 1, and it is compared to 0 or 0x80.

No, it's compared to 0 or 1 (there is a ! on the left hand side of the == too)
> This is exploiting implicit
> behavior which is almost always a bad thing to do if you need to
> understand code later.

It's pretty clear and explicit I'd say.

[Kai Krakow]

2018-06-28 12:39 GMT+02:00 Matteo Bruni :
> 2018-06-28 9:05 GMT+02:00 Kai Krakow :
> > Hello again!
> >
> > 2018-06-28 8:56 GMT+02:00 Kai Krakow :
> > > Hello!
> > >
> > > 2018-06-27 21:14 GMT+02:00 Alex Henrie :
> > > > On Wed, Jun 27, 2018 at 11:24 AM John Found  wrote:
> > > > > While reading the code in wineX11.drv/keyboard.c I noticed the following fragment from the function X11DRV_KeymapNotify:
> > > > > for (vkey = VK_LSHIFT; vkey <= VK_RMENU; vkey++)
> > > > > {
> > > > >     int m = vkey - VK_LSHIFT;
> > > > > > >>     if (modifiers[m].vkey && !(keystate[vkey] & 0x80) != !modifiers[m].pressed)
> > > > >     {
> > > > >         TRACE( "Adjusting state for vkey %#.2x. State before %#.2x\n",
> > > > >                modifiers[m].vkey, keystate[vkey]);
> > > > >
> > > > >         update_key_state( keystate, vkey, modifiers[m].pressed );
> > > > >         changed = TRUE;
> > > > >     }
> > > > > }
> > > > >
> > > > > Can someone explain what the condition pointed by >>> is doing? (Unfortunately, my C skills are pretty low...)
> > > > >
> > > > > Why not simply "(modifiers[m].vkey && (keystate[vkey] & 0x80) != modifiers[m].pressed)"?
> > > >
> > > > I don't immediately know what this if statement is for, but I can tell
> > > > you that the ! operator converts a value of zero to 1 and a nonzero
> > > > value to 0. So, if keystate[vkey] & 0x80 is 0 then
> > > > modifiers[m].pressed must be nonzero and vice versa.
> > >
> > > I always was under the impression that the bang operator does a binary
> > > inversion of the value. Thus, it would convert 0x7F to 0x80. This also
> > > means it would convert 0x00 (signed) to 0xFF (signed) which turns 0
> > > into -1, not 1.
> > >
> > > But however it works, this code is not working in any obvious way and
> > > should maybe be rewritten.
> > >
> > > Just my two cents.
> >
> > I just looked it up, the bang operator seems to be a logical (not
> > binary) operator in C99:
> > https://stackoverflow.com/questions/3661751/what-is-0-in-c
> >
> > According to this, I question how the code above is supposed to work
> > in the first place because "!modifiers[m].pressed" would always be 0
> > or 1, and it is compared to 0 or 0x80.
>
> No, it's compared to 0 or 1 (there is a ! on the left hand side of the == too)

Oh *facepalm*, you're right. I didn't notice the opening and closing
parentheses in the correct position.
> > This is exploiting implicit
> > behavior which is almost always a bad thing to do if you need to
> > understand code later.
>
> It's pretty clear and explicit I'd say.

Maybe but still not very visible...

But this should answer the OPs question why it is done in this way.

Thanks for clarification,
Kai

[John Found]

On Thu, 28 Jun 2018 14:04:31 +0200
Kai Krakow  wrote:
> But this should answer the OPs question why it is done in this way.

Well, for me the code is still entangled, and not very legible. 
Nevertheless I understood how it works and it seems to be correct.

Thank you and all thread contributors above. :)

Regards.

P.S. Actually I found another possible reason for the researched bug, but will ask about it in another post below.

[John Found]

OK, continuing the bug #45385 investigation, I found out that it is somehow related to the window manager and its focus policies.

I am working and testing in XFCE and in the window manager settings there are two checkboxes:

[ ] Activate focus stealing protection
    [ ] Honor ICCCM focus hint

So, the buggy behavior happens when and only when both these options are checked. 
When only one of the is checked or both are unchecked, the bug does not manifests itself.

The big question here is whether the bug is in the WM/X11 or in the WINE itself.

I can't judge by myself, simply because I don't know how this "stealing prevention" works and
what is "ICCCM focus hint".

While this finding allows to workaround the problem, I am still curious about the actual reason for the wrong behavior.

Any considerations?

Regards

On Wed, 27 Jun 2018 08:50:22 +0300
John Found  wrote:
> I just reported bug #45385 (https://bugs.winehq.org/show_bug.cgi?id=45385) and want to try to fix it.
>
> So I want to ask about some preliminary directions - where to check the code, 
> what is the general structure of the code related to the bug subject, possible suspicious places.
>
> Here is the full bug report in order to save you a visit to the bug tracker:
> > I noticed that the state of the keys sometimes sticks in pressed state.
> >
> > This happens when cycling windows with some shortcut key combination.
> >
> > For example if cycling with Alt+Tab, on pressing Alt, the program gets WM_KEYDOWN and the state of the VK_MENU becomes pressed. But after cycling windows, the program does not get WM_KEYUP because the window is not focused and VK_MENU (and the respective VK_LMENU or VK_RMENU) remain in pressed state.
> >
> > When cycling back to the program window, the window get focused only after releasing Alt key, so it does not get this event as well.
> >
> > If cycling windows with another shortcut key combination (for example Alt+Shift+Tab - for backward cycling) both VK_MENU and VK_SHIFT keys stick.
> >
> > In the same time, GetAsyncKeyState returns the proper state of the keys.
> >
> > Note1: The problem is obviously in the wineserver code, because it handles the key state tables for the different threads.
> >
> > Note2: The effect happens only sometimes. It seems the code for proper processing is already there, but some racing conditions have place.
> >
> > Note3: There is some probability that the effect is in result of my application code, but it never happens on real Windows, so I considered it a bug.
> >
> > Note4: I tried to workaround this problem by reading the whole table by GetAsyncKeyState and setting it then with SetKeyboardState on WM_ACTIVATE message of the main window. This workaround actually works, but is too ugly IMO.
> > The same trick on WM_ACTIVATEAPP does not work.
>
> -- 
> John Found