From 0c5b61b7e1443abefefd7ba35378ad14da22189f Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Sat, 1 Feb 2025 19:33:18 +0100 Subject: [PATCH] LibWeb: Fix infinite repaint loop when cached display list is used Before this change, `m_needs_repaint` was reset in `Document::record_display_list()` only when the cached display list was absent. This meant that if the last triggered repaint used the cached display list, we would keep repainting indefinitely until the display list was invalidated (We schedule a task that checks if repainting is required 60/s). This change also moves `m_needs_repaint` from Document to TraversableNavigable as we only ever need to repaint a document that belongs to traversable. --- Libraries/LibWeb/DOM/Document.cpp | 5 +---- Libraries/LibWeb/DOM/Document.h | 3 --- Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp | 14 +++++++------- Libraries/LibWeb/HTML/Navigable.cpp | 7 ------- Libraries/LibWeb/HTML/Navigable.h | 2 -- Libraries/LibWeb/HTML/TraversableNavigable.cpp | 2 ++ Libraries/LibWeb/HTML/TraversableNavigable.h | 5 +++++ Services/WebContent/ConnectionFromClient.cpp | 2 +- Services/WebContent/PageClient.cpp | 2 +- 9 files changed, 17 insertions(+), 25 deletions(-) diff --git a/Libraries/LibWeb/DOM/Document.cpp b/Libraries/LibWeb/DOM/Document.cpp index e54f7492d67..2717b7552a8 100644 --- a/Libraries/LibWeb/DOM/Document.cpp +++ b/Libraries/LibWeb/DOM/Document.cpp @@ -6093,8 +6093,6 @@ void Document::set_needs_display(CSSPixelRect const&, InvalidateDisplayList shou // FIXME: Ignore updates outside the visible viewport rect. // This requires accounting for fixed-position elements in the input rect, which we don't do yet. - m_needs_repaint = true; - if (should_invalidate_display_list == InvalidateDisplayList::Yes) { invalidate_display_list(); } @@ -6104,6 +6102,7 @@ void Document::set_needs_display(CSSPixelRect const&, InvalidateDisplayList shou return; if (navigable->is_traversable()) { + navigable->traversable_navigable()->set_needs_repaint(); Web::HTML::main_thread_event_loop().schedule(); return; } @@ -6188,8 +6187,6 @@ RefPtr Document::record_display_list(PaintConfig config) display_list->set_device_pixels_per_css_pixel(page().client().device_pixels_per_css_pixel()); display_list->set_scroll_state(viewport_paintable.scroll_state()); - m_needs_repaint = false; - m_cached_display_list = display_list; m_cached_display_list_paint_config = config; diff --git a/Libraries/LibWeb/DOM/Document.h b/Libraries/LibWeb/DOM/Document.h index 13bbda26c26..e231b004d48 100644 --- a/Libraries/LibWeb/DOM/Document.h +++ b/Libraries/LibWeb/DOM/Document.h @@ -735,7 +735,6 @@ public: GC::Ptr cached_navigable(); void set_cached_navigable(GC::Ptr); - [[nodiscard]] bool needs_repaint() const { return m_needs_repaint; } void set_needs_display(InvalidateDisplayList = InvalidateDisplayList::Yes); void set_needs_display(CSSPixelRect const&, InvalidateDisplayList = InvalidateDisplayList::Yes); @@ -1101,8 +1100,6 @@ private: // NOTE: This is WeakPtr, not GCPtr, on purpose. We don't want the document to keep some old detached navigable alive. WeakPtr m_cached_navigable; - bool m_needs_repaint { false }; - bool m_enable_cookies_on_file_domains { false }; Optional m_cached_display_list_paint_config; diff --git a/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp b/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp index 0816431c39a..9c33acefc35 100644 --- a/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp +++ b/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp @@ -411,13 +411,13 @@ void EventLoop::update_the_rendering() for (auto& document : docs) { document->page().client().process_screenshot_requests(); auto navigable = document->navigable(); - if (navigable && document->needs_repaint()) { - auto* browsing_context = document->browsing_context(); - auto& page = browsing_context->page(); - if (navigable->is_traversable()) { - VERIFY(page.client().is_ready_to_paint()); - page.client().paint_next_frame(); - } + if (!navigable->is_traversable()) + continue; + auto traversable = navigable->traversable_navigable(); + if (traversable && traversable->needs_repaint()) { + auto& page = traversable->page(); + VERIFY(page.client().is_ready_to_paint()); + page.client().paint_next_frame(); } } diff --git a/Libraries/LibWeb/HTML/Navigable.cpp b/Libraries/LibWeb/HTML/Navigable.cpp index b979f238cec..d0949bfcc5c 100644 --- a/Libraries/LibWeb/HTML/Navigable.cpp +++ b/Libraries/LibWeb/HTML/Navigable.cpp @@ -2198,13 +2198,6 @@ void Navigable::perform_scroll_of_viewport(CSSPixelPoint new_position) HTML::main_thread_event_loop().schedule(); } -void Navigable::set_needs_display(InvalidateDisplayList should_invalidate_display_list) -{ - if (auto document = active_document(); document) { - document->set_needs_display(should_invalidate_display_list); - } -} - // https://html.spec.whatwg.org/#rendering-opportunity bool Navigable::has_a_rendering_opportunity() const { diff --git a/Libraries/LibWeb/HTML/Navigable.h b/Libraries/LibWeb/HTML/Navigable.h index 5e9217b5e18..6b3052ac7ee 100644 --- a/Libraries/LibWeb/HTML/Navigable.h +++ b/Libraries/LibWeb/HTML/Navigable.h @@ -173,8 +173,6 @@ public: virtual void set_viewport_size(CSSPixelSize); void perform_scroll_of_viewport(CSSPixelPoint position); - void set_needs_display(InvalidateDisplayList = InvalidateDisplayList::Yes); - // https://html.spec.whatwg.org/#rendering-opportunity [[nodiscard]] bool has_a_rendering_opportunity() const; diff --git a/Libraries/LibWeb/HTML/TraversableNavigable.cpp b/Libraries/LibWeb/HTML/TraversableNavigable.cpp index a0a240d4c81..c4e6229dbb1 100644 --- a/Libraries/LibWeb/HTML/TraversableNavigable.cpp +++ b/Libraries/LibWeb/HTML/TraversableNavigable.cpp @@ -1429,6 +1429,8 @@ NonnullRefPtr TraversableNavigable::painting_surface_for_b void TraversableNavigable::paint(DevicePixelRect const& content_rect, Painting::BackingStore& target, PaintOptions paint_options) { + m_needs_repaint = false; + auto document = active_document(); if (!document) return; diff --git a/Libraries/LibWeb/HTML/TraversableNavigable.h b/Libraries/LibWeb/HTML/TraversableNavigable.h index dce622d9337..d0d2f23eccb 100644 --- a/Libraries/LibWeb/HTML/TraversableNavigable.h +++ b/Libraries/LibWeb/HTML/TraversableNavigable.h @@ -113,6 +113,9 @@ public: void set_viewport_size(CSSPixelSize) override; + bool needs_repaint() const { return m_needs_repaint; } + void set_needs_repaint() { m_needs_repaint = true; } + private: TraversableNavigable(GC::Ref); @@ -161,6 +164,8 @@ private: RefPtr m_skia_backend_context; OwnPtr m_skia_player; HashMap> m_bitmap_to_surface; + + bool m_needs_repaint { true }; }; struct BrowsingContextAndDocument { diff --git a/Services/WebContent/ConnectionFromClient.cpp b/Services/WebContent/ConnectionFromClient.cpp index 3f71114e32e..859ebd4e68b 100644 --- a/Services/WebContent/ConnectionFromClient.cpp +++ b/Services/WebContent/ConnectionFromClient.cpp @@ -374,7 +374,7 @@ void ConnectionFromClient::debug_request(u64 page_id, ByteString const& request, if (request == "set-line-box-borders") { bool state = argument == "on"; page->set_should_show_line_box_borders(state); - page->page().top_level_traversable()->set_needs_display(); + page->page().top_level_traversable()->set_needs_repaint(); return; } diff --git a/Services/WebContent/PageClient.cpp b/Services/WebContent/PageClient.cpp index 8a6f3d02649..8e6e2a26600 100644 --- a/Services/WebContent/PageClient.cpp +++ b/Services/WebContent/PageClient.cpp @@ -879,6 +879,6 @@ Web::DisplayListPlayerType PageClient::display_list_player_type() const void PageClient::queue_screenshot_task(Optional node_id) { m_screenshot_tasks.enqueue({ node_id }); - page().top_level_traversable()->set_needs_display(); + page().top_level_traversable()->set_needs_repaint(); } }