From 81add73955f0d521f79e136d4fdc7c8e13d3050d Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 12 Nov 2020 18:23:05 +0100 Subject: LibWeb: Make Frame point weakly to Page This patch makes Page weakable and allows page-less frames to exist. Page is single-owner, and Frame is multiple-owner, so it's not sound for Frame to assume its containing Page will stick around for its own entire lifetime. Fixes #3976. --- Libraries/LibWeb/CSS/StyleValue.cpp | 3 +- Libraries/LibWeb/DOM/Document.cpp | 28 ++++++++++---- Libraries/LibWeb/DOM/Document.h | 3 ++ Libraries/LibWeb/DOM/Window.cpp | 5 +-- Libraries/LibWeb/HTML/HTMLFormElement.cpp | 3 +- Libraries/LibWeb/HTML/HTMLInputElement.cpp | 6 +-- Libraries/LibWeb/Layout/LayoutWidget.cpp | 2 +- Libraries/LibWeb/Loader/FrameLoader.cpp | 9 +++-- Libraries/LibWeb/Page/EventHandler.cpp | 60 +++++++++++++++++------------- Libraries/LibWeb/Page/Frame.cpp | 16 +++++--- Libraries/LibWeb/Page/Frame.h | 6 +-- Libraries/LibWeb/Page/Page.h | 2 +- 12 files changed, 87 insertions(+), 56 deletions(-) diff --git a/Libraries/LibWeb/CSS/StyleValue.cpp b/Libraries/LibWeb/CSS/StyleValue.cpp index c744e29053..79054484b3 100644 --- a/Libraries/LibWeb/CSS/StyleValue.cpp +++ b/Libraries/LibWeb/CSS/StyleValue.cpp @@ -172,7 +172,8 @@ Color IdentifierStyleValue::to_color(const DOM::Document& document) const if (id() == CSS::ValueID::VendorSpecificLink) return document.link_color(); - auto palette = document.frame()->page().palette(); + ASSERT(document.page()); + auto palette = document.page()->palette(); switch (id()) { case CSS::ValueID::VendorSpecificPaletteDesktopBackground: return palette.color(ColorRole::DesktopBackground); diff --git a/Libraries/LibWeb/DOM/Document.cpp b/Libraries/LibWeb/DOM/Document.cpp index 4f2084fe0e..43f3486362 100644 --- a/Libraries/LibWeb/DOM/Document.cpp +++ b/Libraries/LibWeb/DOM/Document.cpp @@ -343,8 +343,10 @@ void Document::layout() m_layout_root->layout(); m_layout_root->set_needs_display(); - if (frame()->is_main_frame()) - frame()->page().client().page_did_layout(); + if (frame()->is_main_frame()) { + if (auto* page = this->page()) + page->client().page_did_layout(); + } } void Document::update_style() @@ -446,27 +448,27 @@ Color Document::link_color() const { if (m_link_color.has_value()) return m_link_color.value(); - if (!frame()) + if (!page()) return Color::Blue; - return frame()->page().palette().link(); + return page()->palette().link(); } Color Document::active_link_color() const { if (m_active_link_color.has_value()) return m_active_link_color.value(); - if (!frame()) + if (!page()) return Color::Red; - return frame()->page().palette().active_link(); + return page()->palette().active_link(); } Color Document::visited_link_color() const { if (m_visited_link_color.has_value()) return m_visited_link_color.value(); - if (!frame()) + if (!page()) return Color::Magenta; - return frame()->page().palette().visited_link(); + return page()->palette().visited_link(); } static JS::VM& main_thread_vm() @@ -596,4 +598,14 @@ void Document::set_ready_state(const String& ready_state) dispatch_event(Event::create("readystatechange")); } +Page* Document::page() +{ + return m_frame ? m_frame->page() : nullptr; +} + +const Page* Document::page() const +{ + return m_frame ? m_frame->page() : nullptr; +} + } diff --git a/Libraries/LibWeb/DOM/Document.h b/Libraries/LibWeb/DOM/Document.h index b179254867..f6699bb2c6 100644 --- a/Libraries/LibWeb/DOM/Document.h +++ b/Libraries/LibWeb/DOM/Document.h @@ -99,6 +99,9 @@ public: Frame* frame() { return m_frame.ptr(); } const Frame* frame() const { return m_frame.ptr(); } + Page* page(); + const Page* page() const; + Color background_color(const Gfx::Palette&) const; RefPtr background_image() const; diff --git a/Libraries/LibWeb/DOM/Window.cpp b/Libraries/LibWeb/DOM/Window.cpp index cb76b91bea..cdf88d6b3d 100644 --- a/Libraries/LibWeb/DOM/Window.cpp +++ b/Libraries/LibWeb/DOM/Window.cpp @@ -61,9 +61,8 @@ void Window::set_wrapper(Badge, Bindings::WindowObject& void Window::alert(const String& message) { - if (!m_document.frame()) - return; - m_document.frame()->page().client().page_did_request_alert(message); + if (auto* page = m_document.page()) + page->client().page_did_request_alert(message); } bool Window::confirm(const String& message) diff --git a/Libraries/LibWeb/HTML/HTMLFormElement.cpp b/Libraries/LibWeb/HTML/HTMLFormElement.cpp index ebafd9a465..b709538cff 100644 --- a/Libraries/LibWeb/HTML/HTMLFormElement.cpp +++ b/Libraries/LibWeb/HTML/HTMLFormElement.cpp @@ -105,7 +105,8 @@ void HTMLFormElement::submit(RefPtr submitter) request.set_body(body); } - document().frame()->page().load(request); + if (auto* page = document().page()) + page->load(request); } } diff --git a/Libraries/LibWeb/HTML/HTMLInputElement.cpp b/Libraries/LibWeb/HTML/HTMLInputElement.cpp index bf721ed071..3ab1443e6e 100644 --- a/Libraries/LibWeb/HTML/HTMLInputElement.cpp +++ b/Libraries/LibWeb/HTML/HTMLInputElement.cpp @@ -62,9 +62,9 @@ void HTMLInputElement::did_click_button(Badge) RefPtr HTMLInputElement::create_layout_node(const CSS::StyleProperties* parent_style) { - ASSERT(document().frame()); - auto& frame = *document().frame(); - auto& page_view = const_cast(static_cast(frame.page().client())); + ASSERT(document().page()); + auto& page = *document().page(); + auto& page_view = const_cast(static_cast(page.client())); if (type() == "hidden") return nullptr; diff --git a/Libraries/LibWeb/Layout/LayoutWidget.cpp b/Libraries/LibWeb/Layout/LayoutWidget.cpp index fe79e0aa67..a80394a8a9 100644 --- a/Libraries/LibWeb/Layout/LayoutWidget.cpp +++ b/Libraries/LibWeb/Layout/LayoutWidget.cpp @@ -60,7 +60,7 @@ void LayoutWidget::did_set_rect() void LayoutWidget::update_widget() { auto adjusted_widget_position = absolute_rect().location().to_type(); - auto& page_view = static_cast(frame().page().client()); + auto& page_view = static_cast(frame().page()->client()); adjusted_widget_position.move_by(-page_view.horizontal_scrollbar().value(), -page_view.vertical_scrollbar().value()); widget().move_to(adjusted_widget_position); } diff --git a/Libraries/LibWeb/Loader/FrameLoader.cpp b/Libraries/LibWeb/Loader/FrameLoader.cpp index 087ab4fbc2..8e11a24331 100644 --- a/Libraries/LibWeb/Loader/FrameLoader.cpp +++ b/Libraries/LibWeb/Loader/FrameLoader.cpp @@ -160,8 +160,10 @@ bool FrameLoader::load(const LoadRequest& request, Type type) set_resource(ResourceLoader::the().load_resource(Resource::Type::Generic, request)); - if (type == Type::Navigation) - frame().page().client().page_did_start_loading(url); + if (type == Type::Navigation) { + if (auto* page = frame().page()) + page->client().page_did_start_loading(url); + } if (type == Type::IFrame) return true; @@ -184,7 +186,8 @@ bool FrameLoader::load(const LoadRequest& request, Type type) return; } dbg() << "Decoded favicon, " << bitmap->size(); - frame().page().client().page_did_change_favicon(*bitmap); + if (auto* page = frame().page()) + page->client().page_did_change_favicon(*bitmap); }); } diff --git a/Libraries/LibWeb/Page/EventHandler.cpp b/Libraries/LibWeb/Page/EventHandler.cpp index c75d41c0bb..ad10e0b77d 100644 --- a/Libraries/LibWeb/Page/EventHandler.cpp +++ b/Libraries/LibWeb/Page/EventHandler.cpp @@ -125,7 +125,6 @@ bool EventHandler::handle_mousedown(const Gfx::IntPoint& position, unsigned butt } NonnullRefPtr document = *m_frame.document(); - auto& page_client = m_frame.page().client(); auto result = layout_root()->hit_test(position, HitTestType::Exact); if (!result.layout_node) @@ -148,7 +147,8 @@ bool EventHandler::handle_mousedown(const Gfx::IntPoint& position, unsigned butt return false; } - m_frame.page().set_focused_frame({}, m_frame); + if (auto* page = m_frame.page()) + page->set_focused_frame({}, m_frame); auto offset = compute_mouse_event_offset(position, *result.layout_node); node->dispatch_event(UIEvents::MouseEvent::create("mousedown", offset.x(), offset.y())); @@ -158,7 +158,8 @@ bool EventHandler::handle_mousedown(const Gfx::IntPoint& position, unsigned butt if (button == GUI::MouseButton::Right && is(*node)) { auto& image_element = downcast(*node); auto image_url = image_element.document().complete_url(image_element.src()); - page_client.page_did_request_image_context_menu(m_frame.to_main_frame_position(position), image_url, "", modifiers, image_element.bitmap()); + if (auto* page = m_frame.page()) + page->client().page_did_request_image_context_menu(m_frame.to_main_frame_position(position), image_url, "", modifiers, image_element.bitmap()); return true; } @@ -176,16 +177,19 @@ bool EventHandler::handle_mousedown(const Gfx::IntPoint& position, unsigned butt m_frame.scroll_to_anchor(anchor); } else { if (m_frame.is_main_frame()) { - page_client.page_did_click_link(url, link->target(), modifiers); + if (auto* page = m_frame.page()) + page->client().page_did_click_link(url, link->target(), modifiers); } else { // FIXME: Handle different targets! m_frame.loader().load(url, FrameLoader::Type::Navigation); } } } else if (button == GUI::MouseButton::Right) { - page_client.page_did_request_link_context_menu(m_frame.to_main_frame_position(position), url, link->target(), modifiers); + if (auto* page = m_frame.page()) + page->client().page_did_request_link_context_menu(m_frame.to_main_frame_position(position), url, link->target(), modifiers); } else if (button == GUI::MouseButton::Middle) { - page_client.page_did_middle_click_link(url, link->target(), modifiers); + if (auto* page = m_frame.page()) + page->client().page_did_middle_click_link(url, link->target(), modifiers); } } else { if (button == GUI::MouseButton::Left) { @@ -197,7 +201,8 @@ bool EventHandler::handle_mousedown(const Gfx::IntPoint& position, unsigned butt m_in_mouse_selection = true; } } else if (button == GUI::MouseButton::Right) { - page_client.page_did_request_context_menu(m_frame.to_main_frame_position(position)); + if (auto* page = m_frame.page()) + page->client().page_did_request_context_menu(m_frame.to_main_frame_position(position)); } } return true; @@ -214,7 +219,6 @@ bool EventHandler::handle_mousemove(const Gfx::IntPoint& position, unsigned butt } auto& document = *m_frame.document(); - auto& page_client = m_frame.page().client(); bool hovered_node_changed = false; bool is_hovering_link = false; @@ -227,7 +231,8 @@ bool EventHandler::handle_mousemove(const Gfx::IntPoint& position, unsigned butt document.set_hovered_node(result.layout_node->node()); result.layout_node->handle_mousemove({}, position, buttons, modifiers); // FIXME: It feels a bit aggressive to always update the cursor like this. - page_client.page_did_request_cursor_change(Gfx::StandardCursor::None); + if (auto* page = m_frame.page()) + page->client().page_did_request_cursor_change(Gfx::StandardCursor::None); return true; } @@ -262,28 +267,31 @@ bool EventHandler::handle_mousemove(const Gfx::IntPoint& position, unsigned butt layout_root()->set_selection_end({ hit.layout_node, hit.index_in_node }); } dump_selection("MouseMove"); - page_client.page_did_change_selection(); + if (auto* page = m_frame.page()) + page->client().page_did_change_selection(); } } - if (is_hovering_link) - page_client.page_did_request_cursor_change(Gfx::StandardCursor::Hand); - else if (is_hovering_text) - page_client.page_did_request_cursor_change(Gfx::StandardCursor::IBeam); - else - page_client.page_did_request_cursor_change(Gfx::StandardCursor::None); - - if (hovered_node_changed) { - RefPtr hovered_html_element = document.hovered_node() ? document.hovered_node()->enclosing_html_element() : nullptr; - if (hovered_html_element && !hovered_html_element->title().is_null()) { - page_client.page_did_enter_tooltip_area(m_frame.to_main_frame_position(position), hovered_html_element->title()); - } else { - page_client.page_did_leave_tooltip_area(); - } + if (auto* page = m_frame.page()) { if (is_hovering_link) - page_client.page_did_hover_link(document.complete_url(hovered_link_element->href())); + page->client().page_did_request_cursor_change(Gfx::StandardCursor::Hand); + else if (is_hovering_text) + page->client().page_did_request_cursor_change(Gfx::StandardCursor::IBeam); else - page_client.page_did_unhover_link(); + page->client().page_did_request_cursor_change(Gfx::StandardCursor::None); + + if (hovered_node_changed) { + RefPtr hovered_html_element = document.hovered_node() ? document.hovered_node()->enclosing_html_element() : nullptr; + if (hovered_html_element && !hovered_html_element->title().is_null()) { + page->client().page_did_enter_tooltip_area(m_frame.to_main_frame_position(position), hovered_html_element->title()); + } else { + page->client().page_did_leave_tooltip_area(); + } + if (is_hovering_link) + page->client().page_did_hover_link(document.complete_url(hovered_link_element->href())); + else + page->client().page_did_unhover_link(); + } } return true; } diff --git a/Libraries/LibWeb/Page/Frame.cpp b/Libraries/LibWeb/Page/Frame.cpp index ff501d0d73..f7b5a7ed78 100644 --- a/Libraries/LibWeb/Page/Frame.cpp +++ b/Libraries/LibWeb/Page/Frame.cpp @@ -36,7 +36,7 @@ namespace Web { Frame::Frame(DOM::Element& host_element, Frame& main_frame) - : m_page(main_frame.page()) + : m_page(*main_frame.page()) , m_main_frame(main_frame) , m_loader(*this) , m_event_handler({}, *this) @@ -72,7 +72,7 @@ void Frame::setup() bool Frame::is_focused_frame() const { - return this == &page().focused_frame(); + return m_page && &m_page->focused_frame() == this; } void Frame::set_document(DOM::Document* document) @@ -89,10 +89,12 @@ void Frame::set_document(DOM::Document* document) if (m_document) { m_document->attach_to_frame({}, *this); - page().client().page_did_change_title(m_document->title()); + if (m_page) + m_page->client().page_did_change_title(m_document->title()); } - page().client().page_did_set_document_in_main_frame(m_document); + if (m_page) + m_page->client().page_did_set_document_in_main_frame(m_document); } void Frame::set_size(const Gfx::IntSize& size) @@ -120,7 +122,8 @@ void Frame::set_needs_display(const Gfx::IntRect& rect) return; if (is_main_frame()) { - page().client().page_did_invalidate(to_main_frame_rect(rect)); + if (m_page) + m_page->client().page_did_invalidate(to_main_frame_rect(rect)); return; } @@ -168,7 +171,8 @@ void Frame::scroll_to_anchor(const String& fragment) float_rect.move_by(-padding_box.left, -padding_box.top); } - page().client().page_did_request_scroll_into_view(enclosing_int_rect(float_rect)); + if (m_page) + m_page->client().page_did_request_scroll_into_view(enclosing_int_rect(float_rect)); } Gfx::IntRect Frame::to_main_frame_rect(const Gfx::IntRect& a_rect) diff --git a/Libraries/LibWeb/Page/Frame.h b/Libraries/LibWeb/Page/Frame.h index bb0145d540..2696454182 100644 --- a/Libraries/LibWeb/Page/Frame.h +++ b/Libraries/LibWeb/Page/Frame.h @@ -55,8 +55,8 @@ public: void set_document(DOM::Document*); - Page& page() { return m_page; } - const Page& page() const { return m_page; } + Page* page() { return m_page; } + const Page* page() const { return m_page; } const Gfx::IntSize& size() const { return m_size; } void set_size(const Gfx::IntSize&); @@ -100,7 +100,7 @@ private: void setup(); - Page& m_page; + WeakPtr m_page; Frame& m_main_frame; FrameLoader m_loader; diff --git a/Libraries/LibWeb/Page/Page.h b/Libraries/LibWeb/Page/Page.h index 0b9094d7f2..8fa1719d2e 100644 --- a/Libraries/LibWeb/Page/Page.h +++ b/Libraries/LibWeb/Page/Page.h @@ -40,7 +40,7 @@ namespace Web { class PageClient; -class Page { +class Page : public Weakable { AK_MAKE_NONCOPYABLE(Page); AK_MAKE_NONMOVABLE(Page); -- cgit v1.2.3