From 806e08425a90abf84d79ae58ecfd0df11f3a2418 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Thu, 27 Apr 2023 09:19:20 -0400 Subject: [PATCH] LibWeb: Clear the mouse event tracking node when it stops wanting events This can occur if a mouse click on a mouse event tracking node causes a page navigation. As the old document is torn down, the event handler may have a stale reference to the tracking node. If a subsequent mouse event occurs on that node, we would crash trying to access the node's styled properties that are no longer valid. To fix this, when we are deciding what node to send the event to, and we have a mouse event tracking node, check if that node still wants the event. If not, clear the tracking node. --- .../Libraries/LibWeb/Page/EventHandler.cpp | 63 +++++++++---------- Userland/Libraries/LibWeb/Page/EventHandler.h | 7 +++ 2 files changed, 37 insertions(+), 33 deletions(-) diff --git a/Userland/Libraries/LibWeb/Page/EventHandler.cpp b/Userland/Libraries/LibWeb/Page/EventHandler.cpp index 922ab7132c..51ca89e1a3 100644 --- a/Userland/Libraries/LibWeb/Page/EventHandler.cpp +++ b/Userland/Libraries/LibWeb/Page/EventHandler.cpp @@ -170,12 +170,8 @@ bool EventHandler::handle_mousewheel(CSSPixelPoint position, unsigned button, un bool handled_event = false; JS::GCPtr paintable; - if (m_mouse_event_tracking_layout_node) { - paintable = m_mouse_event_tracking_layout_node->paintable(); - } else { - if (auto result = paint_root()->hit_test(position, Painting::HitTestType::Exact); result.has_value()) - paintable = result->paintable; - } + if (auto result = target_for_mouse_position(position); result.has_value()) + paintable = result->paintable; if (paintable) { paintable->handle_mousewheel({}, position, buttons, modifiers, wheel_delta_x, wheel_delta_y); @@ -218,12 +214,8 @@ bool EventHandler::handle_mouseup(CSSPixelPoint position, unsigned button, unsig bool handled_event = false; JS::GCPtr paintable; - if (m_mouse_event_tracking_layout_node) { - paintable = m_mouse_event_tracking_layout_node->paintable(); - } else { - if (auto result = paint_root()->hit_test(position, Painting::HitTestType::Exact); result.has_value()) - paintable = result->paintable; - } + if (auto result = target_for_mouse_position(position); result.has_value()) + paintable = result->paintable; if (paintable && paintable->wants_mouse_events()) { if (paintable->handle_mouseup({}, position, button, modifiers) == Painting::Paintable::DispatchEventOfSameName::No) @@ -337,14 +329,10 @@ bool EventHandler::handle_mousedown(CSSPixelPoint position, unsigned button, uns { JS::GCPtr paintable; - if (m_mouse_event_tracking_layout_node) { - paintable = m_mouse_event_tracking_layout_node->paintable(); - } else { - auto result = paint_root()->hit_test(position, Painting::HitTestType::Exact); - if (!result.has_value()) - return false; + if (auto result = target_for_mouse_position(position); result.has_value()) paintable = result->paintable; - } + else + return false; auto pointer_events = paintable->computed_values().pointer_events(); // FIXME: Handle other values for pointer-events. @@ -435,13 +423,10 @@ bool EventHandler::handle_mousemove(CSSPixelPoint position, unsigned buttons, un JS::GCPtr paintable; Optional start_index; - if (m_mouse_event_tracking_layout_node) { - paintable = m_mouse_event_tracking_layout_node->paintable(); - } else { - if (auto result = paint_root()->hit_test(position, Painting::HitTestType::Exact); result.has_value()) { - paintable = result->paintable; - start_index = result->index_in_node; - } + + if (auto result = target_for_mouse_position(position); result.has_value()) { + paintable = result->paintable; + start_index = result->index_in_node; } const HTML::HTMLAnchorElement* hovered_link_element = nullptr; @@ -548,14 +533,10 @@ bool EventHandler::handle_doubleclick(CSSPixelPoint position, unsigned button, u return false; JS::GCPtr paintable; - if (m_mouse_event_tracking_layout_node) { - paintable = m_mouse_event_tracking_layout_node->paintable(); - } else { - auto result = paint_root()->hit_test(position, Painting::HitTestType::Exact); - if (!result.has_value()) - return false; + if (auto result = target_for_mouse_position(position); result.has_value()) paintable = result->paintable; - } + else + return false; auto pointer_events = paintable->computed_values().pointer_events(); // FIXME: Handle other values for pointer-events. @@ -830,4 +811,20 @@ CSSPixelPoint EventHandler::compute_mouse_event_page_offset(CSSPixelPoint event_ // 3. Return the sum of offset and the value of the event’s clientX attribute. return event_client_offset.translated(scroll_offset); } + +Optional EventHandler::target_for_mouse_position(CSSPixelPoint position) +{ + if (m_mouse_event_tracking_layout_node) { + if (m_mouse_event_tracking_layout_node->paintable()->wants_mouse_events()) + return Target { m_mouse_event_tracking_layout_node->paintable(), {} }; + + m_mouse_event_tracking_layout_node = nullptr; + } + + if (auto result = paint_root()->hit_test(position, Painting::HitTestType::Exact); result.has_value()) + return Target { result->paintable.ptr(), result->index_in_node }; + + return {}; +} + } diff --git a/Userland/Libraries/LibWeb/Page/EventHandler.h b/Userland/Libraries/LibWeb/Page/EventHandler.h index 9992414630..f95d3a9384 100644 --- a/Userland/Libraries/LibWeb/Page/EventHandler.h +++ b/Userland/Libraries/LibWeb/Page/EventHandler.h @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -44,6 +45,12 @@ private: CSSPixelPoint compute_mouse_event_client_offset(CSSPixelPoint event_page_position) const; CSSPixelPoint compute_mouse_event_page_offset(CSSPixelPoint event_client_offset) const; + struct Target { + JS::GCPtr paintable; + Optional index_in_node; + }; + Optional target_for_mouse_position(CSSPixelPoint position); + Layout::Viewport* layout_root(); Layout::Viewport const* layout_root() const;