From f839b3ecbae2c907328cc7f644d130b17b75e401 Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Fri, 26 Mar 2021 14:47:58 +0100 Subject: [PATCH] wm: make hover handling robust against input races The window manager infers the overall state from the intercepted input events for the decorator and all GUI clients. However, each of those parties have an independent input-event stream. Whereas the order of events within one GUI session is strict, the order of events between GUI sessions is arbitrary. The window manager wrongly relied on a global event ordering to track the pointed-at GUI session. The patch removes the assumption of a global event order by tracking the relevant pointer state for each GUI session independently and evaluating these states when propagating the pointer position to the decorator. Fixes #4059 --- repos/gems/src/server/wm/decorator_gui.h | 78 +++-------------- repos/gems/src/server/wm/gui.h | 102 +++++++++++------------ repos/gems/src/server/wm/last_motion.h | 22 ----- repos/gems/src/server/wm/main.cc | 20 ++++- repos/gems/src/server/wm/pointer.h | 88 +++++++++++++++++++ 5 files changed, 166 insertions(+), 144 deletions(-) delete mode 100644 repos/gems/src/server/wm/last_motion.h create mode 100644 repos/gems/src/server/wm/pointer.h diff --git a/repos/gems/src/server/wm/decorator_gui.h b/repos/gems/src/server/wm/decorator_gui.h index 4d8c5c494d..3aae43543b 100644 --- a/repos/gems/src/server/wm/decorator_gui.h +++ b/repos/gems/src/server/wm/decorator_gui.h @@ -17,7 +17,6 @@ /* Genode includes */ #include #include -#include #include #include #include @@ -25,7 +24,7 @@ /* local includes */ #include -#include +#include namespace Wm { class Main; using Genode::size_t; @@ -173,18 +172,12 @@ struct Wm::Decorator_gui_session : Genode::Rpc_object, Command_buffer &_command_buffer = *_command_ds.local_addr(); - Point _pointer_position { }; - - Reporter &_pointer_reporter; - - Last_motion &_last_motion; + Pointer::State _pointer_state; Input::Session_component &_window_layouter_input; Decorator_content_callback &_content_callback; - unsigned _key_cnt = 0; - /* XXX don't allocate content-registry entries from heap */ Decorator_content_registry _content_registry { _heap }; @@ -206,15 +199,13 @@ struct Wm::Decorator_gui_session : Genode::Rpc_object, Decorator_gui_session(Genode::Env &env, Genode::Ram_allocator &ram, Allocator &md_alloc, - Reporter &pointer_reporter, - Last_motion &last_motion, + Pointer::Tracker &pointer_tracker, Input::Session_component &window_layouter_input, Decorator_content_callback &content_callback) : _env(env), _ram(ram), - _pointer_reporter(pointer_reporter), - _last_motion(last_motion), + _pointer_state(pointer_tracker), _window_layouter_input(window_layouter_input), _content_callback(content_callback), _md_alloc(md_alloc) @@ -229,62 +220,10 @@ struct Wm::Decorator_gui_session : Genode::Rpc_object, void _handle_input() { - auto report_pointer_position = [&] () { - Reporter::Xml_generator xml(_pointer_reporter, [&] () - { - xml.attribute("xpos", _pointer_position.x()); - xml.attribute("ypos", _pointer_position.y()); - }); - }; - while (_gui_session.input()->pending()) _gui_session.input()->for_each_event([&] (Input::Event const &ev) { - - if (ev.press()) _key_cnt++; - - if (ev.release()) { - _key_cnt--; - - /* - * When returning from a drag operation to idle state, the - * pointer position may have moved to another window - * element. Propagate the least recent pointer position to - * the decorator to update its hover model. - */ - if (_key_cnt == 0) - report_pointer_position(); - } - - ev.handle_absolute_motion([&] (int x, int y) { - - _last_motion = LAST_MOTION_DECORATOR; - - _pointer_position = Point(x, y); - - /* update pointer model, but only when hovering */ - if (_key_cnt == 0) - report_pointer_position(); - }); - - if (ev.hover_leave()) { - - /* - * Invalidate pointer as reported to the decorator if the - * pointer moved from a window decoration to a position - * with no window known to the window manager. If the last - * motion referred to one of the regular client session, - * this is not needed because the respective session will - * update the pointer model with the entered position - * already. - */ - if (_last_motion == LAST_MOTION_DECORATOR) { - Reporter::Xml_generator xml(_pointer_reporter, [&] () - { }); - } - } - - _window_layouter_input.submit(ev); - }); + _pointer_state.apply_event(ev); + _window_layouter_input.submit(ev); }); } void _execute_command(Command const &cmd) @@ -393,6 +332,11 @@ struct Wm::Decorator_gui_session : Genode::Rpc_object, _gui_session.upgrade_ram(ram_quota); } + Pointer::Position last_observed_pointer_pos() const + { + return _pointer_state.last_observed_pos(); + } + /*************************** ** GUI session interface ** diff --git a/repos/gems/src/server/wm/gui.h b/repos/gems/src/server/wm/gui.h index 05d450bb10..36a8d51a1d 100644 --- a/repos/gems/src/server/wm/gui.h +++ b/repos/gems/src/server/wm/gui.h @@ -85,7 +85,6 @@ namespace Wm { namespace Gui { struct Wm::Gui::Click_handler : Interface { virtual void handle_click(Point pos) = 0; - virtual void handle_enter(Point pos) = 0; }; @@ -491,6 +490,7 @@ class Wm::Gui::Session_component : public Rpc_object, Area _requested_size { }; bool _resize_requested = false; bool _has_alpha = false; + Pointer::State _pointer_state; Point const _initial_pointer_pos { -1, -1 }; Point _pointer_pos = _initial_pointer_pos; Point _virtual_pointer_pos { }; @@ -594,30 +594,33 @@ class Wm::Gui::Session_component : public Rpc_object, _click_handler.handle_click(_pointer_pos); /* - * Reset pointer model for the decorator once the pointer - * enters the application area of a window. + * Hide application-local motion events from the pointer + * position shared with the decorator. The position is + * propagated only when entering/leaving an application's + * screen area or when finishing a drag operation. */ + bool propagate_to_pointer_state = false; + + /* pointer enters application area */ if (ev.absolute_motion() && _first_motion && _key_cnt == 0) { - _click_handler.handle_enter(_pointer_pos); + propagate_to_pointer_state = true; _first_motion = false; } - /* - * We may leave the dragged state on another window than - * the clicked one. During the dragging, the decorator - * remained unaware of pointer movements. Now, when leaving - * the drag stage, we need to make the decorator aware of - * the most recent pointer position to update the hover - * model. - */ + /* may be end of drag operation */ if (ev.release() && _key_cnt == 0) - _click_handler.handle_enter(_pointer_pos); + propagate_to_pointer_state = true; + /* pointer has left the application area */ if (ev.hover_leave()) { _pointer_pos = _initial_pointer_pos; _first_motion = true; + propagate_to_pointer_state = true; } + if (propagate_to_pointer_state) + _pointer_state.apply_event(ev); + /* submit event to the client */ _input_session.submit(_translate_event(ev, input_origin)); } @@ -775,6 +778,7 @@ class Wm::Gui::Session_component : public Rpc_object, Window_registry &window_registry, Allocator &session_alloc, Session_label const &session_label, + Pointer::Tracker &pointer_tracker, Click_handler &click_handler, Session_control_fn &session_control_fn) : @@ -787,6 +791,7 @@ class Wm::Gui::Session_component : public Rpc_object, _child_view_alloc(&session_alloc), _input_session_cap(env.ep().manage(_input_session)), _click_handler(click_handler), + _pointer_state(pointer_tracker), _view_handle_registry(session_alloc) { _gui_input.sigh(_input_handler); @@ -886,6 +891,11 @@ class Wm::Gui::Session_component : public Rpc_object, v->hidden(hidden); } + Pointer::Position last_observed_pointer_pos() const + { + return _pointer_state.last_observed_pos(); + } + /** * Return session capability to real GUI session */ @@ -1070,14 +1080,12 @@ class Wm::Gui::Root : public Genode::Rpc_object >, enum { STACK_SIZE = 1024*sizeof(long) }; - Reporter &_pointer_reporter; + Pointer::Tracker &_pointer_tracker; Reporter &_focus_request_reporter; unsigned _focus_request_cnt = 0; - Last_motion _last_motion = LAST_MOTION_DECORATOR; - Window_registry &_window_registry; Input::Session_component _window_layouter_input { _env, _env.ram() }; @@ -1088,32 +1096,9 @@ class Wm::Gui::Root : public Genode::Rpc_object >, struct Click_handler : Gui::Click_handler { Input::Session_component &window_layouter_input; - Reporter &pointer_reporter; - Last_motion &last_motion; - - void handle_enter(Gui::Point pos) override - { - last_motion = LAST_MOTION_NITPICKER; - - Reporter::Xml_generator xml(pointer_reporter, [&] () - { - xml.attribute("xpos", pos.x()); - xml.attribute("ypos", pos.y()); - }); - } void handle_click(Gui::Point pos) override { - /* - * Propagate clicked-at position to decorator such that it can - * update its hover model. - */ - Reporter::Xml_generator xml(pointer_reporter, [&] () - { - xml.attribute("xpos", pos.x()); - xml.attribute("ypos", pos.y()); - }); - /* * Supply artificial mouse click to the decorator's input session * (which is routed to the layouter). @@ -1123,17 +1108,10 @@ class Wm::Gui::Root : public Genode::Rpc_object >, window_layouter_input.submit(Input::Release{Input::BTN_LEFT}); } - Click_handler(Input::Session_component &window_layouter_input, - Reporter &pointer_reporter, - Last_motion &last_motion) - : - window_layouter_input(window_layouter_input), - pointer_reporter(pointer_reporter), - last_motion(last_motion) - { } + Click_handler(Input::Session_component &window_layouter_input) + : window_layouter_input(window_layouter_input) { } - } _click_handler { _window_layouter_input, _pointer_reporter, - _last_motion }; + } _click_handler { _window_layouter_input }; /** * List of regular sessions @@ -1157,12 +1135,12 @@ class Wm::Gui::Root : public Genode::Rpc_object >, Root(Genode::Env &env, Window_registry &window_registry, Allocator &md_alloc, Genode::Ram_allocator &ram, - Reporter &pointer_reporter, Reporter &focus_request_reporter, + Pointer::Tracker &pointer_tracker, Reporter &focus_request_reporter, Gui::Session &focus_gui_session) : _env(env), _md_alloc(md_alloc), _ram(ram), - _pointer_reporter(pointer_reporter), + _pointer_tracker(pointer_tracker), _focus_request_reporter(focus_request_reporter), _window_registry(window_registry), _focus_gui_session(focus_gui_session) @@ -1172,6 +1150,24 @@ class Wm::Gui::Root : public Genode::Rpc_object >, env.parent().announce(env.ep().manage(*this)); } + Pointer::Position last_observed_pointer_pos() const + { + Pointer::Position pos { }; + + for (Decorator_gui_session const *s = _decorator_sessions.first(); s; s = s->next()) { + if (!pos.valid) + pos = s->last_observed_pointer_pos(); }; + + if (pos.valid) + return pos; + + for (Session_component const *s = _sessions.first(); s; s = s->next()) { + if (!pos.valid) + pos = s->last_observed_pointer_pos(); }; + + return pos; + } + /******************** ** Root interface ** @@ -1215,6 +1211,7 @@ class Wm::Gui::Root : public Genode::Rpc_object >, auto session = new (_md_alloc) Session_component(_env, _ram, _window_registry, _md_alloc, session_label, + _pointer_tracker, _click_handler, *this); _sessions.insert(session); return _env.ep().manage(*session); @@ -1224,8 +1221,7 @@ class Wm::Gui::Root : public Genode::Rpc_object >, { auto session = new (_md_alloc) Decorator_gui_session(_env, _ram, _md_alloc, - _pointer_reporter, - _last_motion, + _pointer_tracker, _window_layouter_input, *this); _decorator_sessions.insert(session); diff --git a/repos/gems/src/server/wm/last_motion.h b/repos/gems/src/server/wm/last_motion.h deleted file mode 100644 index b42eb9874f..0000000000 --- a/repos/gems/src/server/wm/last_motion.h +++ /dev/null @@ -1,22 +0,0 @@ -/* - * \brief Type of client that receive the last motion event - * \author Norman Feske - * \date 2014-06-10 - */ - -/* - * Copyright (C) 2014-2017 Genode Labs GmbH - * - * This file is part of the Genode OS framework, which is distributed - * under the terms of the GNU Affero General Public License version 3. - */ - -#ifndef _LAST_MOTION_H_ -#define _LAST_MOTION_H_ - -namespace Wm { - - enum Last_motion { LAST_MOTION_NITPICKER, LAST_MOTION_DECORATOR }; -} - -#endif /* _LAST_MOTION_H_ */ diff --git a/repos/gems/src/server/wm/main.cc b/repos/gems/src/server/wm/main.cc index 285162a867..36a1806be7 100644 --- a/repos/gems/src/server/wm/main.cc +++ b/repos/gems/src/server/wm/main.cc @@ -37,7 +37,7 @@ namespace Wm { } -struct Wm::Main +struct Wm::Main : Pointer::Tracker { Genode::Env &env; @@ -63,7 +63,7 @@ struct Wm::Main Gui::Connection focus_gui_session { env }; Gui::Root gui_root { env, window_registry, heap, env.ram(), - pointer_reporter, focus_request_reporter, + *this, focus_request_reporter, focus_gui_session }; void handle_focus_update() @@ -112,6 +112,22 @@ struct Wm::Main Report_forwarder _report_forwarder { env, heap }; Rom_forwarder _rom_forwarder { env, heap }; + /** + * Pointer::Tracker interface + */ + void update_pointer_report() override + { + Pointer::Position pos = gui_root.last_observed_pointer_pos(); + + Reporter::Xml_generator xml(pointer_reporter, [&] () + { + if (pos.valid) { + xml.attribute("xpos", pos.value.x()); + xml.attribute("ypos", pos.value.y()); + } + }); + } + Main(Genode::Env &env) : env(env) { pointer_reporter.enabled(true); diff --git a/repos/gems/src/server/wm/pointer.h b/repos/gems/src/server/wm/pointer.h new file mode 100644 index 0000000000..fbff2b7b12 --- /dev/null +++ b/repos/gems/src/server/wm/pointer.h @@ -0,0 +1,88 @@ +/* + * \brief Type of client that receive the last motion event + * \author Norman Feske + * \date 2014-06-10 + */ + +/* + * Copyright (C) 2014-2017 Genode Labs GmbH + * + * This file is part of the Genode OS framework, which is distributed + * under the terms of the GNU Affero General Public License version 3. + */ + +#ifndef _POINTER_H_ +#define _POINTER_H_ + +#include + +namespace Wm { struct Pointer; } + + +struct Wm::Pointer +{ + struct Position + { + bool valid; + Point value; + }; + + + struct Tracker : Genode::Interface, Genode::Noncopyable + { + virtual void update_pointer_report() = 0; + }; + + + class State : Genode::Noncopyable + { + private: + + Position _last_observed { }; + + unsigned _key_cnt = 0; + + Tracker &_tracker; + + public: + + State(Tracker &tracker) : _tracker(tracker) { } + + void apply_event(Input::Event const &ev) + { + bool pointer_report_update_needed = false; + + if (ev.hover_leave()) + _last_observed = { .valid = false, .value = { } }; + + ev.handle_absolute_motion([&] (int x, int y) { + _last_observed = { .valid = true, .value = { x, y } }; }); + + if (ev.absolute_motion() || ev.hover_leave()) + pointer_report_update_needed = true; + + if (ev.press()) + _key_cnt++; + + if (ev.release()) { + _key_cnt--; + + /* + * When returning from a drag operation to idle state, the + * pointer position may have moved to another window + * element. Propagate the least recent pointer position to + * the decorator to update its hover model. + */ + if (_key_cnt == 0) + pointer_report_update_needed = true; + } + + if (pointer_report_update_needed) + _tracker.update_pointer_report(); + } + + Position last_observed_pos() const { return _last_observed; } + }; +}; + +#endif /* _POINTER_H_ */