From 0aafd22b5fb8932ad3d3babd85c56a4ef7837030 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Fri, 11 Apr 2025 20:00:56 +0200 Subject: [PATCH] LibWeb: Save ScrollState snapshot in DisplayList to avoid race condition With this change we save a copy of of scroll state at the time of recording a display list, instead of actual ScrollState pointer that could be modifed by the main thread while display list is beings rasterized on the rendering thread, which leads to a frame painted with inconsistent scroll state. Fixes https://github.com/LadybirdBrowser/ladybird/issues/4288 --- Libraries/LibWeb/CMakeLists.txt | 1 + .../CSS/StyleValues/CursorStyleValue.cpp | 3 +- Libraries/LibWeb/DOM/Document.cpp | 4 +-- Libraries/LibWeb/HTML/RenderingThread.cpp | 6 ++-- Libraries/LibWeb/HTML/RenderingThread.h | 3 +- .../LibWeb/HTML/TraversableNavigable.cpp | 3 +- Libraries/LibWeb/Painting/Command.h | 4 +-- Libraries/LibWeb/Painting/DisplayList.cpp | 7 ++-- Libraries/LibWeb/Painting/DisplayList.h | 8 ++--- .../LibWeb/Painting/DisplayListPlayerSkia.cpp | 5 +-- .../LibWeb/Painting/DisplayListRecorder.cpp | 4 +-- .../LibWeb/Painting/DisplayListRecorder.h | 2 +- .../NavigableContainerViewportPaintable.cpp | 3 +- Libraries/LibWeb/Painting/SVGMaskable.cpp | 3 +- Libraries/LibWeb/Painting/ScrollFrame.h | 1 + Libraries/LibWeb/Painting/ScrollState.cpp | 20 +++++++++++ Libraries/LibWeb/Painting/ScrollState.h | 34 ++++++++++++++++++- Libraries/LibWeb/SVG/SVGDecodedImageData.cpp | 3 +- 18 files changed, 85 insertions(+), 29 deletions(-) create mode 100644 Libraries/LibWeb/Painting/ScrollState.cpp diff --git a/Libraries/LibWeb/CMakeLists.txt b/Libraries/LibWeb/CMakeLists.txt index b9f3eab6734..94af3e8fd4a 100644 --- a/Libraries/LibWeb/CMakeLists.txt +++ b/Libraries/LibWeb/CMakeLists.txt @@ -691,6 +691,7 @@ set(SOURCES Painting/SVGPaintable.cpp Painting/SVGSVGPaintable.cpp Painting/ScrollFrame.cpp + Painting/ScrollState.cpp Painting/ShadowPainting.cpp Painting/StackingContext.cpp Painting/TableBordersPainting.cpp diff --git a/Libraries/LibWeb/CSS/StyleValues/CursorStyleValue.cpp b/Libraries/LibWeb/CSS/StyleValues/CursorStyleValue.cpp index 439921dd79c..21ab3348a19 100644 --- a/Libraries/LibWeb/CSS/StyleValues/CursorStyleValue.cpp +++ b/Libraries/LibWeb/CSS/StyleValues/CursorStyleValue.cpp @@ -94,7 +94,8 @@ Optional CursorStyleValue::make_image_cursor(Layout::NodeWithS case DisplayListPlayerType::SkiaCPU: { auto painting_surface = Gfx::PaintingSurface::wrap_bitmap(bitmap); Painting::DisplayListPlayerSkia display_list_player; - display_list_player.execute(*display_list, painting_surface); + Painting::ScrollStateSnapshot scroll_state_snapshot; + display_list_player.execute(*display_list, scroll_state_snapshot, painting_surface); break; } } diff --git a/Libraries/LibWeb/DOM/Document.cpp b/Libraries/LibWeb/DOM/Document.cpp index a159978c1f7..64ff0637b70 100644 --- a/Libraries/LibWeb/DOM/Document.cpp +++ b/Libraries/LibWeb/DOM/Document.cpp @@ -6296,8 +6296,9 @@ void Document::invalidate_display_list() RefPtr Document::record_display_list(PaintConfig config) { - if (m_cached_display_list && m_cached_display_list_paint_config == config) + if (m_cached_display_list && m_cached_display_list_paint_config == config) { return m_cached_display_list; + } auto display_list = Painting::DisplayList::create(); Painting::DisplayListRecorder display_list_recorder(display_list); @@ -6354,7 +6355,6 @@ RefPtr Document::record_display_list(PaintConfig config) viewport_paintable.paint_all_phases(context); 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_cached_display_list = display_list; m_cached_display_list_paint_config = config; diff --git a/Libraries/LibWeb/HTML/RenderingThread.cpp b/Libraries/LibWeb/HTML/RenderingThread.cpp index 41effe7809b..89432dc462b 100644 --- a/Libraries/LibWeb/HTML/RenderingThread.cpp +++ b/Libraries/LibWeb/HTML/RenderingThread.cpp @@ -57,17 +57,17 @@ void RenderingThread::rendering_thread_loop() } auto painting_surface = painting_surface_for_backing_store(task->backing_store); - m_skia_player->execute(*task->display_list, painting_surface); + m_skia_player->execute(*task->display_list, task->scroll_state_snapshot, painting_surface); m_main_thread_event_loop.deferred_invoke([callback = move(task->callback)] { callback(); }); } } -void RenderingThread::enqueue_rendering_task(NonnullRefPtr display_list, NonnullRefPtr backing_store, Function&& callback) +void RenderingThread::enqueue_rendering_task(NonnullRefPtr display_list, Painting::ScrollStateSnapshot&& scroll_state_snapshot, NonnullRefPtr backing_store, Function&& callback) { Threading::MutexLocker const locker { m_rendering_task_mutex }; - m_rendering_tasks.enqueue(Task { move(display_list), move(backing_store), move(callback) }); + m_rendering_tasks.enqueue(Task { move(display_list), move(scroll_state_snapshot), move(backing_store), move(callback) }); m_rendering_task_ready_wake_condition.signal(); } diff --git a/Libraries/LibWeb/HTML/RenderingThread.h b/Libraries/LibWeb/HTML/RenderingThread.h index 2aecc153093..f5e0373d68f 100644 --- a/Libraries/LibWeb/HTML/RenderingThread.h +++ b/Libraries/LibWeb/HTML/RenderingThread.h @@ -28,7 +28,7 @@ public: void start(DisplayListPlayerType); void set_skia_player(OwnPtr&& player) { m_skia_player = move(player); } void set_skia_backend_context(RefPtr context) { m_skia_backend_context = move(context); } - void enqueue_rendering_task(NonnullRefPtr, NonnullRefPtr, Function&& callback); + void enqueue_rendering_task(NonnullRefPtr, Painting::ScrollStateSnapshot&&, NonnullRefPtr, Function&& callback); void clear_bitmap_to_surface_cache(); private: @@ -46,6 +46,7 @@ private: struct Task { NonnullRefPtr display_list; + Painting::ScrollStateSnapshot scroll_state_snapshot; NonnullRefPtr backing_store; Function callback; }; diff --git a/Libraries/LibWeb/HTML/TraversableNavigable.cpp b/Libraries/LibWeb/HTML/TraversableNavigable.cpp index ecae592a888..886858a7896 100644 --- a/Libraries/LibWeb/HTML/TraversableNavigable.cpp +++ b/Libraries/LibWeb/HTML/TraversableNavigable.cpp @@ -1426,7 +1426,8 @@ RefPtr TraversableNavigable::record_display_list(DevicePi void TraversableNavigable::start_display_list_rendering(NonnullRefPtr display_list, NonnullRefPtr backing_store, Function&& callback) { - m_rendering_thread.enqueue_rendering_task(move(display_list), move(backing_store), move(callback)); + auto scroll_state_snapshot = active_document()->paintable()->scroll_state().snapshot(); + m_rendering_thread.enqueue_rendering_task(move(display_list), move(scroll_state_snapshot), move(backing_store), move(callback)); } } diff --git a/Libraries/LibWeb/Painting/Command.h b/Libraries/LibWeb/Painting/Command.h index ac0fd887d6d..f8b3dbb4d37 100644 --- a/Libraries/LibWeb/Painting/Command.h +++ b/Libraries/LibWeb/Painting/Command.h @@ -8,13 +8,11 @@ #include #include -#include #include #include #include #include #include -#include #include #include #include @@ -34,6 +32,7 @@ #include #include #include +#include namespace Web::Painting { @@ -392,6 +391,7 @@ struct AddMask { struct PaintNestedDisplayList { RefPtr display_list; + ScrollStateSnapshot scroll_state_snapshot; Gfx::IntRect rect; [[nodiscard]] Gfx::IntRect bounding_rect() const { return rect; } diff --git a/Libraries/LibWeb/Painting/DisplayList.cpp b/Libraries/LibWeb/Painting/DisplayList.cpp index 0494a10fe7d..36ef0073c2f 100644 --- a/Libraries/LibWeb/Painting/DisplayList.cpp +++ b/Libraries/LibWeb/Painting/DisplayList.cpp @@ -35,18 +35,18 @@ static bool command_is_clip_or_mask(Command const& command) }); } -void DisplayListPlayer::execute(DisplayList& display_list, RefPtr surface) +void DisplayListPlayer::execute(DisplayList& display_list, ScrollStateSnapshot const& scroll_state, RefPtr surface) { if (surface) { surface->lock_context(); } - execute_impl(display_list, surface); + execute_impl(display_list, scroll_state, surface); if (surface) { surface->unlock_context(); } } -void DisplayListPlayer::execute_impl(DisplayList& display_list, RefPtr surface) +void DisplayListPlayer::execute_impl(DisplayList& display_list, ScrollStateSnapshot const& scroll_state, RefPtr surface) { if (surface) m_surfaces.append(*surface); @@ -56,7 +56,6 @@ void DisplayListPlayer::execute_impl(DisplayList& display_list, RefPtr); + void execute(DisplayList&, ScrollStateSnapshot const&, RefPtr); protected: Gfx::PaintingSurface& surface() const { return m_surfaces.last(); } - void execute_impl(DisplayList&, RefPtr); + void execute_impl(DisplayList&, ScrollStateSnapshot const& scroll_state, RefPtr); private: virtual void flush() = 0; @@ -93,9 +93,6 @@ public: AK::SegmentedVector const& commands() const { return m_commands; } - void set_scroll_state(ScrollState scroll_state) { m_scroll_state = move(scroll_state); } - ScrollState const& scroll_state() const { return m_scroll_state; } - void set_device_pixels_per_css_pixel(double device_pixels_per_css_pixel) { m_device_pixels_per_css_pixel = device_pixels_per_css_pixel; } double device_pixels_per_css_pixel() const { return m_device_pixels_per_css_pixel; } @@ -103,7 +100,6 @@ private: DisplayList() = default; AK::SegmentedVector m_commands; - ScrollState m_scroll_state; double m_device_pixels_per_css_pixel; }; diff --git a/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp b/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp index 89e96654024..c440c34a9d4 100644 --- a/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp +++ b/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp @@ -972,7 +972,8 @@ void DisplayListPlayerSkia::add_mask(AddMask const& command) auto mask_surface = Gfx::PaintingSurface::create_with_size(m_context, rect.size(), Gfx::BitmapFormat::BGRA8888, Gfx::AlphaType::Premultiplied); - execute_impl(*command.display_list, mask_surface); + ScrollStateSnapshot scroll_state_snapshot; + execute_impl(*command.display_list, scroll_state_snapshot, mask_surface); SkMatrix mask_matrix; mask_matrix.setTranslate(rect.x(), rect.y()); @@ -985,7 +986,7 @@ void DisplayListPlayerSkia::paint_nested_display_list(PaintNestedDisplayList con { auto& canvas = surface().canvas(); canvas.translate(command.rect.x(), command.rect.y()); - execute_impl(*command.display_list, {}); + execute_impl(*command.display_list, command.scroll_state_snapshot, {}); } void DisplayListPlayerSkia::paint_scrollbar(PaintScrollBar const& command) diff --git a/Libraries/LibWeb/Painting/DisplayListRecorder.cpp b/Libraries/LibWeb/Painting/DisplayListRecorder.cpp index 8e4c219c919..e00e7ac04f4 100644 --- a/Libraries/LibWeb/Painting/DisplayListRecorder.cpp +++ b/Libraries/LibWeb/Painting/DisplayListRecorder.cpp @@ -24,9 +24,9 @@ void DisplayListRecorder::append(Command&& command) m_command_list.append(move(command), scroll_frame_id); } -void DisplayListRecorder::paint_nested_display_list(RefPtr display_list, Gfx::IntRect rect) +void DisplayListRecorder::paint_nested_display_list(RefPtr display_list, ScrollStateSnapshot&& scroll_state_snapshot, Gfx::IntRect rect) { - append(PaintNestedDisplayList { move(display_list), rect }); + append(PaintNestedDisplayList { move(display_list), move(scroll_state_snapshot), rect }); } void DisplayListRecorder::add_rounded_rect_clip(CornerRadii corner_radii, Gfx::IntRect border_rect, CornerClip corner_clip) diff --git a/Libraries/LibWeb/Painting/DisplayListRecorder.h b/Libraries/LibWeb/Painting/DisplayListRecorder.h index 0da68e1ea24..86a0c022379 100644 --- a/Libraries/LibWeb/Painting/DisplayListRecorder.h +++ b/Libraries/LibWeb/Painting/DisplayListRecorder.h @@ -129,7 +129,7 @@ public: void push_stacking_context(PushStackingContextParams params); void pop_stacking_context(); - void paint_nested_display_list(RefPtr display_list, Gfx::IntRect rect); + void paint_nested_display_list(RefPtr display_list, ScrollStateSnapshot&&, Gfx::IntRect rect); void add_rounded_rect_clip(CornerRadii corner_radii, Gfx::IntRect border_rect, CornerClip corner_clip); void add_mask(RefPtr display_list, Gfx::IntRect rect); diff --git a/Libraries/LibWeb/Painting/NavigableContainerViewportPaintable.cpp b/Libraries/LibWeb/Painting/NavigableContainerViewportPaintable.cpp index 097334e6147..bd0c675f66c 100644 --- a/Libraries/LibWeb/Painting/NavigableContainerViewportPaintable.cpp +++ b/Libraries/LibWeb/Painting/NavigableContainerViewportPaintable.cpp @@ -60,7 +60,8 @@ void NavigableContainerViewportPaintable::paint(PaintContext& context, PaintPhas paint_config.should_show_line_box_borders = context.should_show_line_box_borders(); paint_config.has_focus = context.has_focus(); auto display_list = const_cast(hosted_document)->record_display_list(paint_config); - context.display_list_recorder().paint_nested_display_list(display_list, context.enclosing_device_rect(absolute_rect).to_type()); + auto scroll_state_snapshot = hosted_document->paintable()->scroll_state().snapshot(); + context.display_list_recorder().paint_nested_display_list(display_list, move(scroll_state_snapshot), context.enclosing_device_rect(absolute_rect).to_type()); context.display_list_recorder().restore(); diff --git a/Libraries/LibWeb/Painting/SVGMaskable.cpp b/Libraries/LibWeb/Painting/SVGMaskable.cpp index 7185c380587..4d9e066bed1 100644 --- a/Libraries/LibWeb/Painting/SVGMaskable.cpp +++ b/Libraries/LibWeb/Painting/SVGMaskable.cpp @@ -100,7 +100,8 @@ RefPtr SVGMaskable::calculate_mask_of_svg(PaintContext& co StackingContext::paint_svg(paint_context, paintable, PaintPhase::Foreground); auto painting_surface = Gfx::PaintingSurface::wrap_bitmap(*mask_bitmap); DisplayListPlayerSkia display_list_player; - display_list_player.execute(display_list, painting_surface); + ScrollStateSnapshot scroll_state_snapshot; + display_list_player.execute(display_list, scroll_state_snapshot, painting_surface); return mask_bitmap; }; RefPtr mask_bitmap = {}; diff --git a/Libraries/LibWeb/Painting/ScrollFrame.h b/Libraries/LibWeb/Painting/ScrollFrame.h index f5c0c9ac408..c017173d086 100644 --- a/Libraries/LibWeb/Painting/ScrollFrame.h +++ b/Libraries/LibWeb/Painting/ScrollFrame.h @@ -6,6 +6,7 @@ #pragma once +#include #include #include diff --git a/Libraries/LibWeb/Painting/ScrollState.cpp b/Libraries/LibWeb/Painting/ScrollState.cpp new file mode 100644 index 00000000000..c817b325dff --- /dev/null +++ b/Libraries/LibWeb/Painting/ScrollState.cpp @@ -0,0 +1,20 @@ +/* + * Copyright (c) 2025, Aliaksandr Kalenik + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include + +namespace Web::Painting { + +ScrollStateSnapshot ScrollStateSnapshot::create(Vector> const& scroll_frames) +{ + ScrollStateSnapshot snapshot; + snapshot.entries.ensure_capacity(scroll_frames.size()); + for (auto const& scroll_frame : scroll_frames) + snapshot.entries.append({ scroll_frame->cumulative_offset(), scroll_frame->own_offset() }); + return snapshot; +} + +} diff --git a/Libraries/LibWeb/Painting/ScrollState.h b/Libraries/LibWeb/Painting/ScrollState.h index ddadbc550a0..dde14bf1ea4 100644 --- a/Libraries/LibWeb/Painting/ScrollState.h +++ b/Libraries/LibWeb/Painting/ScrollState.h @@ -1,15 +1,42 @@ /* - * Copyright (c) 2024, Aliaksandr Kalenik + * Copyright (c) 2024-2025, Aliaksandr Kalenik * * SPDX-License-Identifier: BSD-2-Clause */ #pragma once +#include #include namespace Web::Painting { +class ScrollStateSnapshot { +public: + static ScrollStateSnapshot create(Vector> const& scroll_frames); + + CSSPixelPoint cumulative_offset_for_frame_with_id(size_t id) const + { + if (id >= entries.size()) + return {}; + return entries[id].cumulative_offset; + } + + CSSPixelPoint own_offset_for_frame_with_id(size_t id) const + { + if (id >= entries.size()) + return {}; + return entries[id].own_offset; + } + +private: + struct Entry { + CSSPixelPoint cumulative_offset; + CSSPixelPoint own_offset; + }; + Vector entries; +}; + class ScrollState { public: NonnullRefPtr create_scroll_frame_for(PaintableBox const& paintable_box, RefPtr parent) @@ -56,6 +83,11 @@ public: } } + ScrollStateSnapshot snapshot() const + { + return ScrollStateSnapshot::create(m_scroll_frames); + } + private: Vector> m_scroll_frames; }; diff --git a/Libraries/LibWeb/SVG/SVGDecodedImageData.cpp b/Libraries/LibWeb/SVG/SVGDecodedImageData.cpp index a7d6fba1cb3..60158f38bac 100644 --- a/Libraries/LibWeb/SVG/SVGDecodedImageData.cpp +++ b/Libraries/LibWeb/SVG/SVGDecodedImageData.cpp @@ -106,7 +106,8 @@ RefPtr SVGDecodedImageData::render(Gfx::IntSize size) const case DisplayListPlayerType::SkiaCPU: { auto painting_surface = Gfx::PaintingSurface::wrap_bitmap(*bitmap); Painting::DisplayListPlayerSkia display_list_player; - display_list_player.execute(*display_list, painting_surface); + Painting::ScrollStateSnapshot scroll_state_snapshot; + display_list_player.execute(*display_list, scroll_state_snapshot, painting_surface); break; } default: