From d02a3d25d0d02bacf9ff46386857acd911166fe5 Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Wed, 7 Aug 2024 17:07:43 +0200 Subject: [PATCH] gui_session: replace Handle_registry by Id_space This patch reworks the view-ID handling within the nitpicker GUI server and the window manager. The namespace of view handles are now represented as an Id_space. In constrast to the former "handles", which could be invalid, IDs cannot be semantically overloaded with anything other than an actual view reference. There is no notion of an invalid handle. IDs are like C++ references (which cannot be a nullptr). This change requires the code to be more explicit. E.g., the stacking of a few at the front-most position can no longer be expressed by passing an invalid handle as neighbor. Issue #5242 --- repos/gems/src/app/themed_decorator/theme.h | 1 + repos/gems/src/server/gui_fader/main.cc | 16 +- repos/gems/src/server/wm/gui.h | 503 +++++++++++-------- repos/os/include/decorator/window.h | 11 +- repos/os/include/gui_session/connection.h | 1 - repos/os/include/gui_session/gui_session.h | 20 +- repos/os/src/server/nitpicker/gui_session.cc | 335 ++++++------ repos/os/src/server/nitpicker/gui_session.h | 57 ++- repos/os/src/server/nitpicker/view.h | 15 +- 9 files changed, 520 insertions(+), 439 deletions(-) diff --git a/repos/gems/src/app/themed_decorator/theme.h b/repos/gems/src/app/themed_decorator/theme.h index 432fceba26..32988982f6 100644 --- a/repos/gems/src/app/themed_decorator/theme.h +++ b/repos/gems/src/app/themed_decorator/theme.h @@ -15,6 +15,7 @@ #define _THEME_H_ /* Genode includes */ +#include #include #include #include diff --git a/repos/gems/src/server/gui_fader/main.cc b/repos/gems/src/server/gui_fader/main.cc index f839f7169b..61065725ca 100644 --- a/repos/gems/src/server/gui_fader/main.cc +++ b/repos/gems/src/server/gui_fader/main.cc @@ -278,22 +278,22 @@ class Gui_fader::Gui_session_component Framebuffer::Session_capability _fb_cap { _env.ep().manage(_fb_session) }; - Gui::Session::View_handle _view_handle { }; + Constructible _view_handle { }; bool _view_visible = false; Rect _view_geometry { }; void _update_view_visibility() { - if (!_view_handle.valid() || (_view_visible == _fb_session.visible())) + if (!_view_handle.constructed() || (_view_visible == _fb_session.visible())) return; using Command = Gui::Session::Command; if (_fb_session.visible()) - _gui.enqueue(_view_handle, _view_geometry); + _gui.enqueue(*_view_handle, _view_geometry); else - _gui.enqueue(_view_handle, Rect()); + _gui.enqueue(*_view_handle, Rect()); _gui.execute(); @@ -347,16 +347,16 @@ class Gui_fader::Gui_session_component Create_view_result create_view() override { - _view_handle = _gui.create_view(); + _view_handle.construct(_gui.create_view()); _update_view_visibility(); - return _view_handle; + return *_view_handle; } Create_child_view_result create_child_view(View_handle parent) override { - _view_handle = _gui.create_child_view(parent); + _view_handle.construct(_gui.create_child_view(parent)); _update_view_visibility(); - return _view_handle; + return *_view_handle; } void destroy_view(View_handle handle) override diff --git a/repos/gems/src/server/wm/gui.h b/repos/gems/src/server/wm/gui.h index d9205e161e..33d8c6ea82 100644 --- a/repos/gems/src/server/wm/gui.h +++ b/repos/gems/src/server/wm/gui.h @@ -98,9 +98,6 @@ struct Wm::Gui::Input_origin_changed_handler : Interface }; -struct Gui::View : Genode::Interface { GENODE_RPC_INTERFACE(); }; - - class Wm::Gui::View : private Genode::Weak_object, public Genode::Rpc_object< ::Gui::View> { @@ -115,15 +112,15 @@ class Wm::Gui::View : private Genode::Weak_object, using Command = Gui::Session::Command; using View_handle = Gui::Session::View_handle; - Session_label _session_label; - Real_gui &_real_gui; - View_handle _real_handle { }; - Title _title { }; - Rect _geometry { }; - Point _buffer_offset { }; - Weak_ptr _neighbor_ptr { }; - bool _neighbor_behind { }; - bool _has_alpha; + Session_label _session_label; + Real_gui &_real_gui; + Constructible _real_handle { }; + Title _title { }; + Rect _geometry { }; + Point _buffer_offset { }; + Weak_ptr _neighbor_ptr { }; + bool _neighbor_behind { }; + bool _has_alpha; View(Real_gui &real_gui, Session_label const &session_label, @@ -143,37 +140,37 @@ class Wm::Gui::View : private Genode::Weak_object, */ void _unsynchronized_apply_view_config(Locked_ptr &neighbor) { - if (!_real_handle.valid()) + if (!_real_handle.constructed()) return; _propagate_view_geometry(); - _real_gui.enqueue(_real_handle, _buffer_offset); - _real_gui.enqueue (_real_handle, _title.string()); + _real_gui.enqueue(*_real_handle, _buffer_offset); + _real_gui.enqueue (*_real_handle, _title.string()); - View_handle real_neighbor_handle; + Constructible real_neighbor_handle { }; if (neighbor.valid()) _real_gui.session.alloc_view_handle(neighbor->real_view_cap()).with_result( - [&] (View_handle handle) { real_neighbor_handle = handle; }, + [&] (View_handle handle) { real_neighbor_handle.construct(handle); }, [&] (auto) { warning("unable to obtain real_neighbor_handle"); } ); - if (real_neighbor_handle.valid()) { + if (real_neighbor_handle.constructed()) { if (_neighbor_behind) - _real_gui.enqueue(_real_handle, real_neighbor_handle); + _real_gui.enqueue(*_real_handle, *real_neighbor_handle); else - _real_gui.enqueue(_real_handle, real_neighbor_handle); + _real_gui.enqueue(*_real_handle, *real_neighbor_handle); } else { if (_neighbor_behind) - _real_gui.enqueue(_real_handle); + _real_gui.enqueue(*_real_handle); else - _real_gui.enqueue(_real_handle); + _real_gui.enqueue(*_real_handle); } _real_gui.execute(); - if (real_neighbor_handle.valid()) - _real_gui.session.release_view_handle(real_neighbor_handle); + if (real_neighbor_handle.constructed()) + _real_gui.session.release_view_handle(*real_neighbor_handle); } void _apply_view_config() @@ -186,8 +183,8 @@ class Wm::Gui::View : private Genode::Weak_object, ~View() { - if (_real_handle.valid()) - _real_gui.session.destroy_view(_real_handle); + if (_real_handle.constructed()) + _real_gui.session.destroy_view(*_real_handle); } using Genode::Weak_object::weak_ptr; @@ -204,7 +201,7 @@ class Wm::Gui::View : private Genode::Weak_object, /* * Propagate new size to real GUI view but */ - if (_real_handle.valid()) { + if (_real_handle.constructed()) { _propagate_view_geometry(); _real_gui.execute(); } @@ -214,8 +211,8 @@ class Wm::Gui::View : private Genode::Weak_object, { _title = Title(title); - if (_real_handle.valid()) { - _real_gui.enqueue(_real_handle, title); + if (_real_handle.constructed()) { + _real_gui.enqueue(*_real_handle, title); _real_gui.execute(); } } @@ -224,19 +221,19 @@ class Wm::Gui::View : private Genode::Weak_object, virtual void stack(Weak_ptr, bool) { } - View_handle real_handle() const { return _real_handle; } - View_capability real_view_cap() { - return _real_gui.session.view_capability(_real_handle); + return _real_handle.constructed() + ? _real_gui.session.view_capability(*_real_handle) + : View_capability(); } void buffer_offset(Point buffer_offset) { _buffer_offset = buffer_offset; - if (_real_handle.valid()) { - _real_gui.enqueue(_real_handle, _buffer_offset); + if (_real_handle.constructed()) { + _real_gui.enqueue(*_real_handle, _buffer_offset); _real_gui.execute(); } } @@ -294,6 +291,31 @@ class Wm::Gui::Top_level_view : public View, private List::Eleme using List::Element::next; + void init_real_gui_view() + { + if (_real_handle.constructed()) + return; + + /* + * Create and configure physical GUI view. + */ + _real_gui.session.create_view().with_result( + [&] (View_handle handle) { + _real_handle.construct(handle); + _real_gui.enqueue(handle, _buffer_offset); + _real_gui.enqueue (handle, _title.string()); + _real_gui.execute(); + }, + [&] (Gui::Session::Create_view_error) { + warning("init_real_view failed"); + } + ); + + if (!_real_handle.constructed()) { + warning("failed to created content view for ", _title); + } + } + void _propagate_view_geometry() override { } void geometry(Rect geometry) override @@ -352,27 +374,13 @@ class Wm::Gui::Top_level_view : public View, private List::Eleme View_capability content_view() { - if (!_real_handle.valid()) { + init_real_gui_view(); - /* - * Create and configure physical GUI view. - */ - _real_gui.session.create_view().with_result( - [&] (View_handle handle) { - _real_handle = handle; - _real_gui.enqueue(_real_handle, _buffer_offset); - _real_gui.enqueue (_real_handle, _title.string()); - _real_gui.execute(); - }, - [&] (Gui::Session::Create_view_error) { } - ); + if (_real_handle.constructed()) + return _real_gui.session.view_capability(*_real_handle); - if (!_real_handle.valid()) { - warning("failed to created content view for ", _title); - return { }; - } - } - return _real_gui.session.view_capability(_real_handle); + error("content_view was unable to obtain real view"); + return { }; } void hidden(bool hidden) { _window_registry.hidden(_win_id, hidden); } @@ -415,7 +423,8 @@ class Wm::Gui::Child_view : public View, private List::Element void _propagate_view_geometry() override { - _real_gui.enqueue(_real_handle, _geometry); + if (_real_handle.constructed()) + _real_gui.enqueue(*_real_handle, _geometry); } void stack(Weak_ptr neighbor_ptr, bool behind) override @@ -443,33 +452,35 @@ class Wm::Gui::Child_view : public View, private List::Element void try_to_init_real_view() { - if (_real_handle.valid()) + if (_real_handle.constructed()) return; Locked_ptr parent(_parent); if (!parent.valid()) return; - View_handle parent_handle { }; + Constructible parent_handle { }; _real_gui.session.alloc_view_handle(parent->real_view_cap()).with_result( - [&] (View_handle handle) { parent_handle = handle; }, - [&] (Gui::Session::Alloc_view_handle_error) { } + [&] (View_handle handle) { parent_handle.construct(handle); }, + [&] (Gui::Session::Alloc_view_handle_error e) { + warning("try_to_init_real_view could not alloc parent handle e=", (int)e); + } ); - if (!parent_handle.valid()) { + if (!parent_handle.constructed()) { warning("try_to_init_real_view failed to obtain parent handle"); return; } - _real_gui.session.create_child_view(parent_handle).with_result( - [&] (View_handle handle) { _real_handle = handle; }, + _real_gui.session.create_child_view(*parent_handle).with_result( + [&] (View_handle handle) { _real_handle.construct(handle); }, [&] (Gui::Session::Create_child_view_error) { } ); - if (!_real_handle.valid()) { + if (!_real_handle.constructed()) { warning("try_to_init_real_view failed to create child view"); return; } - _real_gui.session.release_view_handle(parent_handle); + _real_gui.session.release_view_handle(*parent_handle); if (_neighbor_ptr == _parent) _unsynchronized_apply_view_config(parent); @@ -484,11 +495,11 @@ class Wm::Gui::Child_view : public View, private List::Element void hide() { - if (!_real_handle.valid()) + if (!_real_handle.constructed()) return; - _real_gui.session.destroy_view(_real_handle); - _real_handle = { }; + _real_gui.session.destroy_view(*_real_handle); + _real_handle.destruct(); } }; @@ -503,6 +514,37 @@ class Wm::Gui::Session_component : public Rpc_object, using View_handle = Gui::Session::View_handle; + using View_ids = Id_space; + + struct View_ref : Gui::View_ref + { + Weak_ptr _weak_ptr; + + View_ids::Element id; + + View_ref(Weak_ptr view, View_ids &ids) + : _weak_ptr(view), id(*this, ids) { } + + View_ref(Weak_ptr view, View_ids &ids, View_handle id) + : _weak_ptr(view), id(*this, ids, id) { } + + auto with_view(auto const &fn, auto const &missing_fn) -> decltype(missing_fn()) + { + /* + * Release the lock before calling 'fn' to allow the nesting of + * 'with_view' calls. The locking aspect of the weak ptr is not + * needed here because the component is single-threaded. + */ + View *ptr = nullptr; + { + Locked_ptr view(_weak_ptr); + if (view.valid()) + ptr = view.operator->(); + } + return ptr ? fn(*ptr) : missing_fn(); + } + }; + Genode::Env &_env; Session_label _session_label; @@ -511,8 +553,10 @@ class Wm::Gui::Session_component : public Rpc_object, Window_registry &_window_registry; Tslab _top_level_view_alloc; Tslab _child_view_alloc; + Tslab _view_ref_alloc; List _top_level_views { }; List _child_views { }; + View_ids _view_ids { }; Input::Session_component _input_session { _env, _ram }; Input::Session_capability _input_session_cap; Click_handler &_click_handler; @@ -526,6 +570,18 @@ class Wm::Gui::Session_component : public Rpc_object, Point _virtual_pointer_pos { }; unsigned _key_cnt = 0; + auto _with_view(View_handle handle, auto const &fn, auto const &missing_fn) + -> decltype(missing_fn()) + { + return _view_ids.apply(handle, + [&] (View_ref &view_ref) { + return view_ref.with_view( + [&] (View &view) { return fn(view); }, + [&] /* view vanished */ { return missing_fn(); }); + }, + [&] /* ID does not exist */ { return missing_fn(); }); + } + /* * Command buffer */ @@ -535,13 +591,6 @@ class Wm::Gui::Session_component : public Rpc_object, Command_buffer &_command_buffer = *_command_ds.local_addr(); - /* - * View handle registry - */ - using View_handle_registry = Genode::Handle_registry; - - View_handle_registry _view_handle_registry; - /* * Input */ @@ -687,29 +736,6 @@ class Wm::Gui::Session_component : public Rpc_object, _input_session.submit(Input::Absolute_motion { pos.x, pos.y }); } - View &_create_view_object() - { - Top_level_view *view = new (_top_level_view_alloc) - Top_level_view(_real_gui, _has_alpha, - _window_registry, *this); - - view->resizeable(_mode_sigh.valid()); - - _top_level_views.insert(view); - return *view; - } - - View &_create_child_view_object(View_handle parent_handle) - { - Weak_ptr parent_ptr = _view_handle_registry.lookup(parent_handle); - - Child_view *view = new (_child_view_alloc) - Child_view(_real_gui, _has_alpha, parent_ptr); - - _child_views.insert(view); - return *view; - } - void _destroy_top_level_view(Top_level_view &view) { _top_level_views.remove(&view); @@ -724,62 +750,51 @@ class Wm::Gui::Session_component : public Rpc_object, Genode::destroy(&_child_view_alloc, &view); } - void _destroy_view_object(View &view) - { - if (Top_level_view *v = dynamic_cast(&view)) - _destroy_top_level_view(*v); - - if (Child_view *v = dynamic_cast(&view)) - _destroy_child_view(*v); - } - void _execute_command(Command const &command) { + auto with_this = [&] (auto const &args, auto const &fn) + { + Session_component::_with_view(args.view, + [&] (View &view) { fn(view, args); }, + [&] /* ignore operations on non-existing views */ { }); + }; + switch (command.opcode) { case Command::GEOMETRY: - { - Locked_ptr view(_view_handle_registry.lookup(command.geometry.view)); - if (view.valid()) - view->geometry(command.geometry.rect); - return; - } + + with_this(command.geometry, [&] (View &view, Command::Geometry const &args) { + view.geometry(args.rect); }); + return; case Command::OFFSET: - { - Locked_ptr view(_view_handle_registry.lookup(command.offset.view)); - if (view.valid()) - view->buffer_offset(command.offset.offset); - return; - } + with_this(command.offset, [&] (View &view, Command::Offset const &args) { + view.buffer_offset(args.offset); }); + return; case Command::FRONT: - { - Locked_ptr view(_view_handle_registry.lookup(command.front.view)); - if (view.valid()) - view->stack(Weak_ptr(), true); - return; - } + + with_this(command.front, [&] (View &view, auto const &) { + view.stack(Weak_ptr(), true); }); + return; case Command::FRONT_OF: - { - Locked_ptr view(_view_handle_registry.lookup(command.front_of.view)); - if (!view.valid()) - return; - if (!command.front_of.neighbor.valid()) - return; - - /* stack view relative to neighbor */ - view->stack(_view_handle_registry.lookup(command.front_of.neighbor), - true); - return; - } + with_this(command.front_of, [&] (View &view, Command::Front_of const &args) { + Session_component::_with_view(args.neighbor, + [&] (View &neighbor) { + if (&view != &neighbor) + view.stack(neighbor.weak_ptr(), true); }, + [&] { }); + }); + return; case Command::TITLE: - { - char sanitized_title[command.title.title.capacity()]; + + with_this(command.title, [&] (View &view, Command::Title const &args) { + + char sanitized_title[args.title.capacity()]; Genode::copy_cstring(sanitized_title, command.title.title.string(), sizeof(sanitized_title)); @@ -788,12 +803,9 @@ class Wm::Gui::Session_component : public Rpc_object, if (*c == '"') *c = '\''; - Locked_ptr view(_view_handle_registry.lookup(command.title.view)); - if (view.valid()) - view->title(sanitized_title); - - return; - } + view.title(sanitized_title); + }); + return; case Command::BACK: case Command::BEHIND_OF: @@ -824,10 +836,10 @@ class Wm::Gui::Session_component : public Rpc_object, _window_registry(window_registry), _top_level_view_alloc(&session_alloc), _child_view_alloc(&session_alloc), + _view_ref_alloc(&session_alloc), _input_session_cap(env.ep().manage(_input_session)), _click_handler(click_handler), - _pointer_state(pointer_tracker), - _view_handle_registry(session_alloc) + _pointer_state(pointer_tracker) { _gui_input.sigh(_input_handler); _real_gui.session.mode_sigh(_mode_handler); @@ -836,11 +848,14 @@ class Wm::Gui::Session_component : public Rpc_object, ~Session_component() { + while (_view_ids.apply_any([&] (View_ref &view_ref) { + destroy(_view_ref_alloc, &view_ref); })); + while (Top_level_view *view = _top_level_views.first()) - _destroy_view_object(*view); + _destroy_top_level_view(*view); while (Child_view *view = _child_views.first()) - _destroy_view_object(*view); + _destroy_child_view(*view); _env.ep().dissolve(_input_session); } @@ -958,92 +973,160 @@ class Wm::Gui::Session_component : public Rpc_object, return _input_session_cap; } - Create_view_result create_view() override + template + Create_view_result _create_view_with_id(auto &dealloc, auto const &create_fn) { + Create_view_error error { }; + + VIEW *view_ptr = nullptr; try { - View &view = _create_view_object(); - _env.ep().manage(view); - return _view_handle_registry.alloc(view); + view_ptr = create_fn(); } - catch (Out_of_ram) { return Create_view_error::OUT_OF_RAM; } - catch (Out_of_caps) { return Create_view_error::OUT_OF_CAPS; } + catch (Out_of_ram) { error = Create_view_error::OUT_OF_RAM; } + catch (Out_of_caps) { error = Create_view_error::OUT_OF_CAPS; } + if (!view_ptr) + return error; + + View_ref *view_ref_ptr = nullptr; + try { + view_ref_ptr = + new (_view_ref_alloc) View_ref(view_ptr->weak_ptr(), _view_ids); + } + catch (Out_of_ram) { error = Create_view_error::OUT_OF_RAM; } + catch (Out_of_caps) { error = Create_view_error::OUT_OF_CAPS; } + if (!view_ref_ptr) { + destroy(dealloc, view_ptr); + return error; + } + + _env.ep().manage(*view_ptr); + + return view_ref_ptr->id.id(); } - Create_child_view_result create_child_view(View_handle parent) override + Create_view_result create_view() override { - try { - View &view = _create_child_view_object(parent); - _env.ep().manage(view); - return _view_handle_registry.alloc(view); + Top_level_view *view_ptr = nullptr; + + Create_view_result const result = + _create_view_with_id(_top_level_view_alloc, + [&] { + view_ptr = new (_top_level_view_alloc) + Top_level_view(_real_gui, _has_alpha, + _window_registry, *this); + return view_ptr; + }); + + if (result.ok() && view_ptr) { + + view_ptr->init_real_gui_view(); + + view_ptr->resizeable(_mode_sigh.valid()); + _top_level_views.insert(view_ptr); } - catch (Out_of_ram) { return Create_child_view_error::OUT_OF_RAM; } - catch (Out_of_caps) { return Create_child_view_error::OUT_OF_CAPS; } - catch (View_handle_registry::Lookup_failed) { - return View_handle(); } + return result; + } + + Create_child_view_result create_child_view(View_handle const parent) override + { + using Error = Create_child_view_error; + return _with_view(parent, + [&] (View &parent) -> Create_child_view_result { + + Child_view *view_ptr = nullptr; + + Create_view_result const result = + _create_view_with_id(_child_view_alloc, + [&] { + view_ptr = new (_child_view_alloc) + Child_view(_real_gui, _has_alpha, parent.weak_ptr()); + return view_ptr; + }); + + return result.convert( + [&] (View_handle handle) { + if (view_ptr) + _child_views.insert(view_ptr); + return handle; + }, + [&] (Create_view_error e) { + switch (e) { + case Create_view_error::OUT_OF_RAM: return Error::OUT_OF_RAM; + case Create_view_error::OUT_OF_CAPS: return Error::OUT_OF_CAPS; + }; + return Error::INVALID; + } + ); + }, + [&] () -> Create_child_view_result { return Error::INVALID; }); } void destroy_view(View_handle handle) override { - try { - /* - * Lookup the view but release the lock to prevent the view - * destructor from taking the lock a second time. The locking - * aspect of the weak ptr is not needed within the wm because - * the component is single-threaded. - */ - View *view_ptr = nullptr; - { - Locked_ptr view(_view_handle_registry.lookup(handle)); - if (view.valid()) - view_ptr = view.operator->(); - } - - if (view_ptr) - _destroy_view_object(*view_ptr); - - _view_handle_registry.free(handle); - } catch (View_handle_registry::Lookup_failed) { } + _with_view(handle, + [&] (View &view) { + for (Child_view *v = _child_views.first(); v; v = v->next()) + if (&view == v) { + _destroy_child_view(*v); + return; + } + for (Top_level_view *v = _top_level_views.first(); v; v = v->next()) + if (&view == v) { + _destroy_top_level_view(*v); + return; + } + }, + [&] /* ID exists but view vanished */ { } + ); + release_view_handle(handle); } Alloc_view_handle_result alloc_view_handle(View_capability view_cap) override { - try { - return _env.ep().rpc_ep().apply(view_cap, [&] (View *view) { - return (view) ? _view_handle_registry.alloc(*view) - : View_handle(); }); - } - catch (Out_of_ram) { return Alloc_view_handle_error::OUT_OF_RAM; } - catch (Out_of_caps) { return Alloc_view_handle_error::OUT_OF_CAPS; } + return _env.ep().rpc_ep().apply(view_cap, + [&] (View *view_ptr) -> Alloc_view_handle_result { + if (!view_ptr) + return Alloc_view_handle_error::INVALID; + try { + View_ref &view_ref = *new (_view_ref_alloc) + View_ref(view_ptr->weak_ptr(), _view_ids); + return view_ref.id.id(); + } + catch (Out_of_ram) { return Alloc_view_handle_error::OUT_OF_RAM; } + catch (Out_of_caps) { return Alloc_view_handle_error::OUT_OF_CAPS; } + }); } View_handle_result view_handle(View_capability view_cap, View_handle handle) override { - try { - _env.ep().rpc_ep().apply(view_cap, [&] (View *view) { - if (view) - _view_handle_registry.alloc(*view, handle); }); - return View_handle_result::OK; - } - catch (Out_of_ram) { return View_handle_result::OUT_OF_RAM; } - catch (Out_of_caps) { return View_handle_result::OUT_OF_CAPS; } + /* prevent ID conflict in 'View_ids::Element' constructor */ + release_view_handle(handle); + + return _env.ep().rpc_ep().apply(view_cap, + [&] (View *view_ptr) -> View_handle_result { + if (!view_ptr) + return View_handle_result::INVALID; + try { + new (_view_ref_alloc) View_ref(view_ptr->weak_ptr(), _view_ids, handle); + return View_handle_result::OK; + } + catch (Out_of_ram) { return View_handle_result::OUT_OF_RAM; } + catch (Out_of_caps) { return View_handle_result::OUT_OF_CAPS; } + }); } View_capability view_capability(View_handle handle) override { - Locked_ptr view(_view_handle_registry.lookup(handle)); - - return view.valid() ? view->cap() : View_capability(); + return _with_view(handle, + [&] (View &view) { return view.cap(); }, + [&] /* view does not exist */ { return View_capability(); }); } void release_view_handle(View_handle handle) override { - try { - _view_handle_registry.free(handle); } - - catch (View_handle_registry::Lookup_failed) { - Genode::warning("view lookup failed while releasing view handle"); - return; - } + _view_ids.apply(handle, + [&] (View_ref &view_ref) { destroy(_view_ref_alloc, &view_ref); }, + [&] { }); } Genode::Dataspace_capability command_dataspace() override @@ -1053,12 +1136,8 @@ class Wm::Gui::Session_component : public Rpc_object, void execute() override { - for (unsigned i = 0; i < _command_buffer.num(); i++) { - try { - _execute_command(_command_buffer.get(i)); } - catch (View_handle_registry::Lookup_failed) { - Genode::warning("view lookup failed during command execution"); } - } + for (unsigned i = 0; i < _command_buffer.num(); i++) + _execute_command(_command_buffer.get(i)); /* propagate window-list changes to the layouter */ _window_registry.flush(); diff --git a/repos/os/include/decorator/window.h b/repos/os/include/decorator/window.h index fea4b34a33..e5e0dd886e 100644 --- a/repos/os/include/decorator/window.h +++ b/repos/os/include/decorator/window.h @@ -18,6 +18,7 @@ #include #include #include +#include #include /* decorator includes */ @@ -109,7 +110,7 @@ class Decorator::Window_base : private Genode::List_model::Element /* * View immediately behind the window */ - View_handle _neighbor { }; + Genode::Constructible _neighbor { }; Genode::List_element _abandoned { this }; @@ -138,18 +139,20 @@ class Decorator::Window_base : private Genode::List_model::Element void stacking_neighbor(View_handle neighbor) { - _neighbor = neighbor; + _neighbor.construct(neighbor); _stacked = true; } bool back_most() const { - return _stacked && !_neighbor.valid(); + return _stacked && !_neighbor.constructed(); } bool in_front_of(Window_base const &neighbor) const { - return _stacked && (_neighbor == neighbor.frontmost_view()); + return _stacked + && _neighbor.constructed() + && (*_neighbor == neighbor.frontmost_view()); } void geometry(Rect geometry) { _geometry = geometry; } diff --git a/repos/os/include/gui_session/connection.h b/repos/os/include/gui_session/connection.h index 3ef2514e9d..8b4c2a1928 100644 --- a/repos/os/include/gui_session/connection.h +++ b/repos/os/include/gui_session/connection.h @@ -50,7 +50,6 @@ class Gui::Connection : private Genode::Connection */ Input::Session_client input { _env.rm(), _client.input() }; - using View_handle = Session::View_handle; using Genode::Connection::cap; using Genode::Connection::upgrade; using Genode::Connection::upgrade_ram; diff --git a/repos/os/include/gui_session/gui_session.h b/repos/os/include/gui_session/gui_session.h index a298493f21..da32cd1cf7 100644 --- a/repos/os/include/gui_session/gui_session.h +++ b/repos/os/include/gui_session/gui_session.h @@ -14,9 +14,9 @@ #ifndef _INCLUDE__GUI_SESSION__GUI_SESSION_H_ #define _INCLUDE__GUI_SESSION__GUI_SESSION_H_ +#include #include #include -#include #include #include @@ -28,7 +28,16 @@ namespace Gui { struct View; struct Session; + /* + * View capabilities are used as tokens to share views between sessions. + * There is no RPC interface associated with a view. View operations refer + * to views via session-local IDs. + */ + struct View : Interface { GENODE_RPC_INTERFACE(); }; + struct View_ref : Interface { }; + using View_capability = Capability; + using View_handle = Id_space::Id; using Rect = Surface_base::Rect; using Point = Surface_base::Point; @@ -52,14 +61,7 @@ struct Gui::Session : Genode::Session static constexpr unsigned CAP_QUOTA = Framebuffer::Session::CAP_QUOTA + Input::Session::CAP_QUOTA + 3; - /** - * Session-local view handle - * - * When issuing commands to nitpicker via the 'execute' method, views - * are referenced by session-local handles. - */ - using View_handle = Handle; - + using View_handle = Gui::View_handle; struct Command { diff --git a/repos/os/src/server/nitpicker/gui_session.cc b/repos/os/src/server/nitpicker/gui_session.cc index 7843d3db94..ef0169968d 100644 --- a/repos/os/src/server/nitpicker/gui_session.cc +++ b/repos/os/src/server/nitpicker/gui_session.cc @@ -16,18 +16,6 @@ using namespace Nitpicker; -bool Gui_session::_views_are_equal(View_handle v1, View_handle v2) -{ - if (!v1.valid() || !v2.valid()) - return false; - - Weak_ptr v1_ptr = _view_handle_registry.lookup(v1); - Weak_ptr v2_ptr = _view_handle_registry.lookup(v2); - - return v1_ptr == v2_ptr; -} - - View_owner &Gui_session::forwarded_focus() { Gui_session *next_focus = this; @@ -60,102 +48,75 @@ View_owner &Gui_session::forwarded_focus() void Gui_session::_execute_command(Command const &command) { + auto with_this = [&] (auto const &args, auto const &fn) + { + Gui_session::_with_view(args.view, + [&] (View &view) { fn(view, args); }, + [&] /* ignore operations on non-existing views */ { }); + }; + switch (command.opcode) { case Command::GEOMETRY: - { - Command::Geometry const &cmd = command.geometry; - Locked_ptr view(_view_handle_registry.lookup(cmd.view)); - if (!view.valid()) - return; - Point pos = cmd.rect.p1(); + with_this(command.geometry, [&] (View &view, Command::Geometry const &args) { + Point pos = args.rect.p1(); /* transpose position of top-level views by vertical session offset */ - if (view->top_level()) + if (view.top_level()) pos = _phys_pos(pos, _view_stack.size()); - if (view.valid()) - _view_stack.geometry(*view, Rect(pos, cmd.rect.area)); - - return; - } + _view_stack.geometry(view, Rect(pos, args.rect.area)); + }); + return; case Command::OFFSET: - { - Command::Offset const &cmd = command.offset; - Locked_ptr view(_view_handle_registry.lookup(cmd.view)); - if (view.valid()) - _view_stack.buffer_offset(*view, cmd.offset); - - return; - } + with_this(command.offset, [&] (View &view, Command::Offset const &args) { + _view_stack.buffer_offset(view, args.offset); }); + return; case Command::FRONT: - { - Command::Front const &cmd = command.front; - Locked_ptr view(_view_handle_registry.lookup(cmd.view)); - if (view.valid()) - _view_stack.stack(*view, nullptr, true); - return; - } + with_this(command.front, [&] (View &view, auto const &) { + _view_stack.stack(view, nullptr, true); }); + return; case Command::BACK: - { - Command::Back const &cmd = command.back; - Locked_ptr view(_view_handle_registry.lookup(cmd.view)); - if (view.valid()) - _view_stack.stack(*view, nullptr, false); - return; - } + with_this(command.back, [&] (View &view, auto const &) { + _view_stack.stack(view, nullptr, false); }); + return; case Command::FRONT_OF: - { - Command::Front_of const &cmd = command.front_of; - if (_views_are_equal(cmd.view, cmd.neighbor)) - return; - Locked_ptr view(_view_handle_registry.lookup(cmd.view)); - if (!view.valid()) - return; - - /* stack view relative to neighbor */ - Locked_ptr neighbor(_view_handle_registry.lookup(cmd.neighbor)); - if (neighbor.valid()) - _view_stack.stack(*view, &(*neighbor), false); - return; - } + with_this(command.front_of, [&] (View &view, Command::Front_of const &args) { + Gui_session::_with_view(args.neighbor, + [&] (View &neighbor) { + if (&view != &neighbor) + _view_stack.stack(view, &neighbor, false); }, + [&] { }); + }); + return; case Command::BEHIND_OF: - { - Command::Behind_of const &cmd = command.behind_of; - if (_views_are_equal(cmd.view, cmd.neighbor)) - return; - Locked_ptr view(_view_handle_registry.lookup(cmd.view)); - if (!view.valid()) - return; - - /* stack view relative to neighbor */ - Locked_ptr neighbor(_view_handle_registry.lookup(cmd.neighbor)); - if (neighbor.valid()) - _view_stack.stack(*view, &(*neighbor), true); - return; - } + with_this(command.behind_of, [&] (View &view, Command::Behind_of const &args) { + Gui_session::_with_view(args.neighbor, + [&] (View &neighbor) { + if (&view != &neighbor) + _view_stack.stack(view, &neighbor, false); }, + [&] /* neighbor view does not exist */ { }); + }); + return; case Command::BACKGROUND: - { - Command::Background const &cmd = command.background; - if (_provides_default_bg) { - Locked_ptr view(_view_handle_registry.lookup(cmd.view)); - if (!view.valid()) - return; - view->background(true); - _view_stack.default_background(*view); + with_this(command.background, [&] (View &view, auto const &) { + + if (_provides_default_bg) { + view.background(true); + _view_stack.default_background(view); return; } @@ -164,29 +125,19 @@ void Gui_session::_execute_command(Command const &command) _background->background(false); /* assign session background */ - Locked_ptr view(_view_handle_registry.lookup(cmd.view)); - if (!view.valid()) - return; - - _background = &(*view); + _background = &view; /* switch background view to background mode */ if (background()) - view->background(true); - - return; - } + view.background(true); + }); + return; case Command::TITLE: - { - Command::Title const &cmd = command.title; - Locked_ptr view(_view_handle_registry.lookup(cmd.view)); - if (view.valid()) - _view_stack.title(*view, cmd.title.string()); - - return; - } + with_this(command.title, [&] (View &view, Command::Title const &args) { + _view_stack.title(view, args.title.string()); }); + return; case Command::NOP: return; @@ -247,47 +198,72 @@ void Gui_session::_adopt_new_view(View &view) } -Gui_session::Create_view_result Gui_session::create_view() +Gui_session::Create_view_result Gui_session::_create_view_with_id(auto const &create_fn) { + Create_view_error error { }; try { - View &view = *new (_view_alloc) - View(*this, _texture, { .transparent = false, .background = false }, nullptr); - - _adopt_new_view(view); - - return _view_handle_registry.alloc(view); + View &view = create_fn(); + try { + View_ref &view_ref = + *new (_view_ref_alloc) View_ref(view.weak_ptr(), _view_ids); + _adopt_new_view(view); + return view_ref.id.id(); + } + catch (Out_of_ram) { error = Create_view_error::OUT_OF_RAM; } + catch (Out_of_caps) { error = Create_view_error::OUT_OF_CAPS; } + destroy(_view_alloc, &view); } - catch (Out_of_ram) { return Create_view_error::OUT_OF_RAM; } - catch (Out_of_caps) { return Create_view_error::OUT_OF_CAPS; } + catch (Out_of_ram) { error = Create_view_error::OUT_OF_RAM; } + catch (Out_of_caps) { error = Create_view_error::OUT_OF_CAPS; } + return error; } -Gui_session::Create_child_view_result Gui_session::create_child_view(View_handle const parent_handle) +Gui_session::Create_view_result Gui_session::create_view() { - View *parent_view_ptr = nullptr; + return _create_view_with_id([&] () -> View & { + return *new (_view_alloc) + View(*this, _texture, + { .transparent = false, .background = false }, + nullptr); + }); +} - try { - Locked_ptr parent(_view_handle_registry.lookup(parent_handle)); - if (parent.valid()) - parent_view_ptr = &(*parent); - } - catch (View_handle_registry::Lookup_failed) { } - if (!parent_view_ptr) - return Create_child_view_error::INVALID; +Gui_session::Create_child_view_result Gui_session::create_child_view(View_handle const parent) +{ + using Error = Create_child_view_error; - try { - View &view = *new (_view_alloc) - View(*this, _texture, { .transparent = false, .background = false }, parent_view_ptr); + return _with_view(parent, + [&] (View &parent) -> Create_child_view_result { - parent_view_ptr->add_child(view); + View *view_ptr = nullptr; + Create_view_result const result = _create_view_with_id( + [&] () -> View & { + view_ptr = new (_view_alloc) + View(*this, _texture, + { .transparent = false, .background = false }, + &parent); + return *view_ptr; + }); - _adopt_new_view(view); - - return _view_handle_registry.alloc(view); - } - catch (Out_of_ram) { return Create_child_view_error::OUT_OF_RAM; } - catch (Out_of_caps) { return Create_child_view_error::OUT_OF_CAPS; } + return result.convert( + [&] (View_handle handle) { + if (view_ptr) + parent.add_child(*view_ptr); + return handle; + }, + [&] (Create_view_error e) { + switch (e) { + case Create_view_error::OUT_OF_RAM: return Error::OUT_OF_RAM; + case Create_view_error::OUT_OF_CAPS: return Error::OUT_OF_CAPS; + }; + return Error::INVALID; + }); + }, + [&] /* parent view does not exist */ () -> Create_child_view_result { + return Error::INVALID; } + ); } @@ -320,36 +296,22 @@ void Gui_session::apply_session_policy(Xml_node config, } -void Gui_session::destroy_view(View_handle handle) +void Gui_session::destroy_view(View_handle const handle) { /* - * Search view object given the handle - * - * We cannot look up the view directly from the - * '_view_handle_registry' because we would obtain a weak - * pointer to the view object. If we called the object's - * destructor from the corresponding locked pointer, the - * call of 'lock_for_destruction' in the view's destructor - * would attempt to take the lock again. + * Search among the session's own views the one with the given handle */ - for (Session_view_list_elem *v = _view_list.first(); v; v = v->next()) { - - auto handle_matches = [&] (View const &view) - { - try { return _view_handle_registry.has_handle(view, handle); } - - /* 'Handle_registry::has_handle' may throw */ - catch (...) { return false; }; - }; - - View &view = *static_cast(v); - - if (handle_matches(view)) { - _destroy_view(view); - _view_handle_registry.free(handle); - break; - } - } + _with_view(handle, + [&] (View &view) { + for (Session_view_list_elem *v = _view_list.first(); v; v = v->next()) + if (&view == v) { + _destroy_view(view); + break; + } + }, + [&] /* ID exists but view vanished */ { } + ); + release_view_handle(handle); _hover_updater.update_hover(); } @@ -358,65 +320,62 @@ void Gui_session::destroy_view(View_handle handle) Gui_session::Alloc_view_handle_result Gui_session::alloc_view_handle(View_capability view_cap) { - try { - return _env.ep().rpc_ep().apply(view_cap, [&] (View *view_ptr) -> Alloc_view_handle_result { + return _env.ep().rpc_ep().apply(view_cap, + [&] (View *view_ptr) -> Alloc_view_handle_result { if (!view_ptr) return Alloc_view_handle_error::INVALID; - return _view_handle_registry.alloc(*view_ptr); }); - } - catch (View_handle_registry::Out_of_memory) { return Alloc_view_handle_error::OUT_OF_RAM; } - catch (Out_of_ram) { return Alloc_view_handle_error::OUT_OF_RAM; } - catch (Out_of_caps) { return Alloc_view_handle_error::OUT_OF_RAM; } + try { + View_ref &view_ref = *new (_view_ref_alloc) + View_ref(view_ptr->weak_ptr(), _view_ids); + return view_ref.id.id(); + } + catch (Out_of_ram) { return Alloc_view_handle_error::OUT_OF_RAM; } + catch (Out_of_caps) { return Alloc_view_handle_error::OUT_OF_CAPS; } + }); } Gui_session::View_handle_result Gui_session::view_handle(View_capability view_cap, View_handle handle) { - try { - return _env.ep().rpc_ep().apply(view_cap, [&] (View *view_ptr) -> View_handle_result { + /* prevent ID conflict in 'View_ids::Element' constructor */ + release_view_handle(handle); + + return _env.ep().rpc_ep().apply(view_cap, + [&] (View *view_ptr) -> View_handle_result { if (!view_ptr) return View_handle_result::INVALID; - _view_handle_registry.alloc(*view_ptr, handle); - return View_handle_result::OK; + try { + new (_view_ref_alloc) View_ref(view_ptr->weak_ptr(), _view_ids, handle); + return View_handle_result::OK; + } + catch (Out_of_ram) { return View_handle_result::OUT_OF_RAM; } + catch (Out_of_caps) { return View_handle_result::OUT_OF_CAPS; } }); - } - catch (View_handle_registry::Out_of_memory) { return View_handle_result::OUT_OF_RAM; } - catch (Out_of_ram) { return View_handle_result::OUT_OF_RAM; } - catch (Out_of_caps) { return View_handle_result::OUT_OF_RAM; } } View_capability Gui_session::view_capability(View_handle handle) { - try { - Locked_ptr view(_view_handle_registry.lookup(handle)); - return view.valid() ? view->cap() : View_capability(); - } - catch (View_handle_registry::Lookup_failed) { return View_capability(); } + return _with_view(handle, + [&] (View &view) { return view.cap(); }, + [&] /* view does not exist */ { return View_capability(); }); } void Gui_session::release_view_handle(View_handle handle) { - try { - _view_handle_registry.free(handle); } - - catch (View_handle_registry::Lookup_failed) { - warning("view lookup failed while releasing view handle"); - return; - } + _view_ids.apply(handle, + [&] (View_ref &view_ref) { destroy(_view_ref_alloc, &view_ref); }, + [&] { }); } void Gui_session::execute() { - for (unsigned i = 0; i < _command_buffer.num(); i++) { - try { - _execute_command(_command_buffer.get(i)); } - catch (View_handle_registry::Lookup_failed) { - warning("view lookup failed during command execution"); } - } + for (unsigned i = 0; i < _command_buffer.num(); i++) + _execute_command(_command_buffer.get(i)); + _hover_updater.update_hover(); } diff --git a/repos/os/src/server/nitpicker/gui_session.h b/repos/os/src/server/nitpicker/gui_session.h index 8800127684..469acc5d8e 100644 --- a/repos/os/src/server/nitpicker/gui_session.h +++ b/repos/os/src/server/nitpicker/gui_session.h @@ -5,7 +5,7 @@ */ /* - * Copyright (C) 2006-2017 Genode Labs GmbH + * Copyright (C) 2006-2024 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. @@ -48,6 +48,37 @@ class Nitpicker::Gui_session : public Session_object, { private: + using View_ids = Id_space; + + struct View_ref : Gui::View_ref + { + Weak_ptr _weak_ptr; + + View_ids::Element id; + + View_ref(Weak_ptr view, View_ids &ids) + : _weak_ptr(view), id(*this, ids) { } + + View_ref(Weak_ptr view, View_ids &ids, View_handle id) + : _weak_ptr(view), id(*this, ids, id) { } + + auto with_view(auto const &fn, auto const &missing_fn) -> decltype(missing_fn()) + { + /* + * Release the lock before calling 'fn' to allow the nesting of + * 'with_view' calls. The locking aspect of the weak ptr is not + * needed here because the component is single-threaded. + */ + View *ptr = nullptr; + { + Locked_ptr view(_weak_ptr); + if (view.valid()) + ptr = view.operator->(); + } + return ptr ? fn(*ptr) : missing_fn(); + } + }; + friend class List; using Gui::Session::Label; @@ -105,6 +136,8 @@ class Nitpicker::Gui_session : public Session_object, Tslab _view_alloc { &_session_alloc }; + Tslab _view_ref_alloc { &_session_alloc }; + /* capabilities for sub sessions */ Framebuffer::Session_capability _framebuffer_session_cap; Input::Session_capability _input_session_cap; @@ -122,9 +155,7 @@ class Nitpicker::Gui_session : public Session_object, Command_buffer &_command_buffer = *_command_ds.local_addr(); - using View_handle_registry = Handle_registry; - - View_handle_registry _view_handle_registry; + View_ids _view_ids { }; Reporter &_focus_reporter; @@ -156,6 +187,20 @@ class Nitpicker::Gui_session : public Session_object, void _adopt_new_view(View &); + Create_view_result _create_view_with_id(auto const &); + + auto _with_view(View_handle handle, auto const &fn, auto const &missing_fn) + -> decltype(missing_fn()) + { + return _view_ids.apply(handle, + [&] (View_ref &view_ref) { + return view_ref.with_view( + [&] (View &view) { return fn(view); }, + [&] /* view vanished */ { return missing_fn(); }); + }, + [&] /* ID does not exist */ { return missing_fn(); }); + } + public: Gui_session(Env &env, @@ -182,7 +227,6 @@ class Nitpicker::Gui_session : public Session_object, _framebuffer_session_cap(_env.ep().manage(_framebuffer_session_component)), _input_session_cap(_env.ep().manage(_input_session_component)), _provides_default_bg(provides_default_bg), - _view_handle_registry(_session_alloc), _focus_reporter(focus_reporter) { } @@ -191,6 +235,9 @@ class Nitpicker::Gui_session : public Session_object, _env.ep().dissolve(_framebuffer_session_component); _env.ep().dissolve(_input_session_component); + while (_view_ids.apply_any([&] (View_ref &view_ref) { + destroy(_view_ref_alloc, &view_ref); })); + destroy_all_views(); } diff --git a/repos/os/src/server/nitpicker/view.h b/repos/os/src/server/nitpicker/view.h index 03b2c09550..e7c7d9cecc 100644 --- a/repos/os/src/server/nitpicker/view.h +++ b/repos/os/src/server/nitpicker/view.h @@ -5,7 +5,7 @@ */ /* - * Copyright (C) 2006-2017 Genode Labs GmbH + * Copyright (C) 2006-2024 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. @@ -19,6 +19,8 @@ #include #include #include +#include +#include /* local includes */ #include @@ -54,16 +56,6 @@ namespace Nitpicker { } -namespace Gui { - - /* - * We use view capabilities as mere tokens to pass views between sessions. - * There is no RPC interface associated with a view. - */ - struct View : Interface { GENODE_RPC_INTERFACE(); }; -} - - class Nitpicker::View : private Same_buffer_list_elem, private Session_view_list_elem, private View_stack_elem, @@ -76,7 +68,6 @@ class Nitpicker::View : private Same_buffer_list_elem, using Title = String<32>; using Weak_object::weak_ptr; - using Weak_object::weak_ptr_const; private: