[PATCH weston] input: don't send to clients key events eaten by bindings

Pekka Paalanen ppaalanen at gmail.com
Mon Nov 10 07:15:52 PST 2014


On Wed, 5 Nov 2014 20:51:53 +0200
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

> 2014-11-05 16:23 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> > On Tue,  7 Oct 2014 22:30:25 +0300
> > Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> >
> >> weston key bindings are supposed to eat the key events, and not pass it
> >> on to clients, and indeed the wl_keyboard.key event is not sent. But
> >> we must also not put the key in the keys array to pass to client with
> >> the wl_keyboard.enter event, or else we may send the 'eaten' one too.
> >> In the case of a key binding hiding a surface having the keyboard focus,
> >> the shell may decide to give the focus to another surface, but that will
> >> happen before the key is released, so the new focus surface will receive
> >> the code of the bound key in the wl_keyboard.enter array.
> >> ---
> >>  src/bindings.c   |  7 +++++--
> >>  src/compositor.h |  2 +-
> >>  src/input.c      | 35 +++++++++++++++++++----------------
> >>  3 files changed, 25 insertions(+), 19 deletions(-)
> >
> > Ok, reproducing this problem with visible effects requires Xwayland.
> > Here's how I did it:
> >
> > - open xterm (via Xwayland)
> > - open weston-terminal
> > - press Mod
> > - press 'k' (Mod+k is a weston hotkey for killing an app)
> > - weston-terminal gets killed, focus moves to xterm
> > - before xterm gets killed too, release Mod
> > - 'k' starts repeating in xterm until you release 'k'
> >
> > The premise here is that since 'k' was the final key that triggered a
> > keybinding, the press and release of 'k' must not be sent to any client.
> >
> > This obviously fails in the above experiment, not via a key-press event
> > but via the list of pressed keys in keyboard enter event. (You cannot
> > see it with WAYLAND_DEBUG, because it cannot decode wl_array type.)
> >
> > So, problem confirmed.
> >
> >> diff --git a/src/bindings.c b/src/bindings.c
> >> index 5e24725..452d224 100644
> >> --- a/src/bindings.c
> >> +++ b/src/bindings.c
> >> @@ -255,16 +255,17 @@ install_binding_grab(struct weston_seat *seat, uint32_t time, uint32_t key)
> >>       weston_keyboard_start_grab(seat->keyboard, &grab->grab);
> >>  }
> >>
> >> -WL_EXPORT void
> >> +WL_EXPORT int
> >>  weston_compositor_run_key_binding(struct weston_compositor *compositor,
> >>                                 struct weston_seat *seat,
> >>                                 uint32_t time, uint32_t key,
> >>                                 enum wl_keyboard_key_state state)
> >>  {
> >>       struct weston_binding *b, *tmp;
> >> +     int eaten = 0;
> >>
> >>       if (state == WL_KEYBOARD_KEY_STATE_RELEASED)
> >> -             return;
> >> +             return 0;
> >>
> >>       /* Invalidate all active modifier bindings. */
> >>       wl_list_for_each(b, &compositor->modifier_binding_list, link)
> >> @@ -281,8 +282,10 @@ weston_compositor_run_key_binding(struct weston_compositor *compositor,
> >>                       if (seat->keyboard->grab ==
> >>                           &seat->keyboard->default_grab)
> >>                               install_binding_grab(seat, time, key);
> >> +                     ++eaten;
> >>               }
> >>       }
> >> +     return eaten;
> >>  }
> >>
> >>  WL_EXPORT void
> >> diff --git a/src/compositor.h b/src/compositor.h
> >> index 44379fe..5b6f33b 100644
> >> --- a/src/compositor.h
> >> +++ b/src/compositor.h
> >> @@ -1140,7 +1140,7 @@ weston_binding_destroy(struct weston_binding *binding);
> >>  void
> >>  weston_binding_list_destroy_all(struct wl_list *list);
> >>
> >> -void
> >> +int
> >>  weston_compositor_run_key_binding(struct weston_compositor *compositor,
> >>                                 struct weston_seat *seat, uint32_t time,
> >>                                 uint32_t key,
> >> diff --git a/src/input.c b/src/input.c
> >> index 530904d..d969949 100644
> >> --- a/src/input.c
> >> +++ b/src/input.c
> >> @@ -1316,6 +1316,7 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
> >>       struct weston_keyboard *keyboard = seat->keyboard;
> >>       struct weston_keyboard_grab *grab = keyboard->grab;
> >>       uint32_t *k, *end;
> >> +     int eaten = 0;
> >>
> >>       if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> >>               weston_compositor_idle_inhibit(compositor);
> >> @@ -1325,28 +1326,30 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
> >>               weston_compositor_idle_release(compositor);
> >>       }
> >>
> >> -     end = keyboard->keys.data + keyboard->keys.size;
> >> -     for (k = keyboard->keys.data; k < end; k++) {
> >> -             if (*k == key) {
> >> -                     /* Ignore server-generated repeats. */
> >> -                     if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
> >> -                             return;
> >
> > This early return worries me. There must have been some reason this is
> > here, but the commit that added it says little:
> >
> > commit c6587ea1553f4b8f58a70047fba91f203e03ecfa
> > Author: Daniel Stone <daniel at fooishbar.org>
> > Date:   Fri Jun 22 13:21:31 2012 +0100
> >
> >     Ignore repeat keys in notify_key
> >
> >     Let compositors just blithely post through every event they get,
> >     including repeating keys.
> >
> >     Signed-off-by: Daniel Stone <daniel at fooishbar.org>
> >
> > diff --git a/src/compositor.c b/src/compositor.c
> > index d914b67..52ef89e 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -1858,8 +1858,12 @@ notify_key(struct wl_seat *seat, uint32_t time, uint32_t key,
> >         mods = update_modifier_state(ws, key, state);
> >         end = seat->keyboard->keys.data + seat->keyboard->keys.size;
> >         for (k = seat->keyboard->keys.data; k < end; k++) {
> > -               if (*k == key)
> > +               if (*k == key) {
> > +                       /* Ignore server-generated repeats. */
> > +                       if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
> > +                               return;
> >                         *k = *--end;
> > +               }
> >         }
> >         seat->keyboard->keys.size = (void *) end - seat->keyboard->keys.data;
> >         if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> >
> >
> > To me it looks like this code protects against repeated notify_key()
> > calls with the same key pressed. I'd assume it's for the x11 backend,
> > but I cannot make it repeat.
> >
> > When you move this code to below, key bindings are no longer protected
> > from the repeat, I think.
> 
> You're right, i'll move that for back before the binding call.
> 
> >
> > But do we even need to protect against that repeat?
> >
> >> -                     *k = *--end;
> >> -             }
> >> -     }
> >> -     keyboard->keys.size = (void *) end - keyboard->keys.data;
> >> -     if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> >> -             k = wl_array_add(&keyboard->keys, sizeof *k);
> >> -             *k = key;
> >> -     }
> >> -
> >>       if (grab == &keyboard->default_grab ||
> >>           grab == &keyboard->input_method_grab) {
> >> -             weston_compositor_run_key_binding(compositor, seat, time, key,
> >> +             eaten = weston_compositor_run_key_binding(compositor, seat, time, key,
> >>                                                 state);
> >>               grab = keyboard->grab;
> >>       }
> >>
> >> +     if (eaten == 0) {
> >> +             end = keyboard->keys.data + keyboard->keys.size;
> >> +             for (k = keyboard->keys.data; k < end; k++) {
> >> +                     if (*k == key) {
> >> +                             /* Ignore server-generated repeats. */
> >> +                             if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
> >> +                             return;
> >> +                             *k = *--end;
> >> +                     }
> >> +             }
> >> +             keyboard->keys.size = (void *) end - keyboard->keys.data;
> >> +             if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> >> +                     k = wl_array_add(&keyboard->keys, sizeof *k);
> >> +                     *k = key;
> >> +             }
> >> +     }
> >> +
> >>       grab->interface->key(grab, time, key, state);
> >>
> >>       if (keyboard->pending_keymap &&
> >
> > This patch does fix the problem I confirmed, there is that one detail
> > I'm unsure about.
> >
> > I also have another worry. A swallowed key will not be in the keys
> > array, and I think there are other places that also use the keys array,
> > other than just sending it with the keyboard enter event. Could
> > something break there?
> >
> > Looks like notify_keyboard_focus_out() could fail to call idle_release
> > enough times.
> >
> 
> Yes, but also notify_keyboard_focus_in() calls
> weston_compositor_idle_inhibit() with only the keys in the array, so
> it shouldn't be a problem.

No, _focus_in() is different. It gets the list of keys from the caller,
and simply overwrites the whole list we had internally. So it doesn't
use the internal list at all.

That's pretty logical: the pressed keys can very likely change while we
are out.

That's not even a problem in _focus_in(). It only needs to bump the
inhibit counter as many times as there are keys down. It assumes it
starts from no keys down wrt. to the counter.

_focus_out() OTOH is responsible for making all keys reduce the
counter. If there is a mismatch between a key-down bumping up, and
focus_out bumping down, the counter cannot ever reach zero anymore, and
so weston will never hit idle timeout anymore.

It is not _focus_in() that needs to match with _focus_out(),
but active-time key-downs need to match _focus_out(), and _focus_in()
needs to match active-time key-ups.

Did I get something wrong here?


Thanks,
pq


More information about the wayland-devel mailing list