From 3fe1b97ad00207a9aeac8fb69311891efe684bd5 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Sat, 30 May 2026 22:08:20 +0200 Subject: [PATCH] Compositor: Tighten internal state invariants Several compositor helpers still treated internally-owned state as optional after their callers had already established the necessary preconditions. That made invalid parent/child surface state, screenshot traversal state, and scrollbar capture indices look like normal no-op paths. --- Services/Compositor/CompositorState.cpp | 15 +++++++-------- Services/Compositor/ContextState.cpp | 9 +++------ Services/Compositor/ContextState.h | 2 +- .../Compositor/ViewportScrollbarController.cpp | 13 +------------ 4 files changed, 12 insertions(+), 27 deletions(-) diff --git a/Services/Compositor/CompositorState.cpp b/Services/Compositor/CompositorState.cpp index 4ba8879149..868ee85668 100644 --- a/Services/Compositor/CompositorState.cpp +++ b/Services/Compositor/CompositorState.cpp @@ -342,8 +342,7 @@ void CompositorState::flush_descendant_surfaces_for_screenshot(Web::Compositor:: // list, whose embedded-content commands read those child surfaces. So, flush any descendant with a deferred // present synchronously (deepest-first) — to capture a complete frame instead of a stale/blank iframe. auto* context = context_if_present(context_id); - if (!context) - return; + VERIFY(context); for (auto& child : context->child_contexts()) present_subtree_for_screenshot(child.child_context_id); } @@ -351,8 +350,7 @@ void CompositorState::flush_descendant_surfaces_for_screenshot(Web::Compositor:: bool CompositorState::present_subtree_for_screenshot(Web::Compositor::CompositorContextId context_id) { auto* context = context_if_present(context_id); - if (!context) - return false; + VERIFY(context); bool needs_present = context->needs_synchronous_present_for_screenshot(); for (auto& child : context->child_contexts()) { @@ -383,7 +381,8 @@ bool CompositorState::request_screenshot(Web::Compositor::CompositorContextId co return false; flush_descendant_surfaces_for_screenshot(context_id); - return context->paint_screenshot(*m_display_list_player, target_bitmap); + context->paint_screenshot(*m_display_list_player, target_bitmap); + return true; } void CompositorState::presented_bitmap_ready_to_paint(Web::Compositor::CompositorContextId context_id, i32 bitmap_id) @@ -445,8 +444,9 @@ void CompositorState::cancel_pending_async_presents_for_context(Web::Compositor: void CompositorState::schedule_gpu_completion_check() { - if (!m_skia_backend_context || m_pending_async_presents.is_empty()) + if (!m_skia_backend_context) return; + VERIFY(!m_pending_async_presents.is_empty()); if (!m_gpu_completion_timer) { m_gpu_completion_timer = Core::Timer::create_repeating(gpu_completion_check_interval_ms, [this] { @@ -572,8 +572,7 @@ bool CompositorState::apply_context_update_result( void CompositorState::publish_backing_stores(Web::Compositor::CompositorContextId context_id, ContextState& context, BackingStoreManager::Publication&& publication) { VERIFY(m_client); - if (!context.presents_to_client()) - return; + VERIFY(context.presents_to_client()); m_client->did_allocate_backing_stores(context_id, publication.front_bitmap_id, move(publication.front_shared_image), publication.back_bitmap_id, move(publication.back_shared_image)); } diff --git a/Services/Compositor/ContextState.cpp b/Services/Compositor/ContextState.cpp index 2b5b5e2b23..30b2bb3c9c 100644 --- a/Services/Compositor/ContextState.cpp +++ b/Services/Compositor/ContextState.cpp @@ -513,8 +513,7 @@ void ContextState::did_submit_prepared_frame(Gfx::IntRect viewport_rect) Optional ContextState::present_synchronously(Web::Painting::DisplayListPlayerSkia& display_list_player) { auto* publish_mode = m_presentation_mode.get_pointer(); - if (!publish_mode) - return {}; + VERIFY(publish_mode); if (!can_render_frame()) return {}; // Don't race an async present already in flight for this context; its own completion will publish. @@ -546,15 +545,13 @@ bool ContextState::can_paint_screenshot(Gfx::ShareableBitmap& target_bitmap) con return m_display_list && target_bitmap.is_valid() && target_bitmap.bitmap(); } -bool ContextState::paint_screenshot(Web::Painting::DisplayListPlayerSkia& display_list_player, Gfx::ShareableBitmap& target_bitmap) +void ContextState::paint_screenshot(Web::Painting::DisplayListPlayerSkia& display_list_player, Gfx::ShareableBitmap& target_bitmap) { - if (!can_paint_screenshot(target_bitmap)) - return false; + VERIFY(can_paint_screenshot(target_bitmap)); auto target_surface = Gfx::PaintingSurface::wrap_bitmap(*target_bitmap.bitmap()); paint_current_display_list(display_list_player, *target_surface); display_list_player.flush(*target_surface); - return true; } bool ContextState::acknowledge_presented_bitmap(i32 bitmap_id) diff --git a/Services/Compositor/ContextState.h b/Services/Compositor/ContextState.h index fffb06da5a..19c3920948 100644 --- a/Services/Compositor/ContextState.h +++ b/Services/Compositor/ContextState.h @@ -153,7 +153,7 @@ public: void did_submit_prepared_frame(Gfx::IntRect); Optional present_synchronously(Web::Painting::DisplayListPlayerSkia&); bool can_paint_screenshot(Gfx::ShareableBitmap&) const; - bool paint_screenshot(Web::Painting::DisplayListPlayerSkia&, Gfx::ShareableBitmap&); + void paint_screenshot(Web::Painting::DisplayListPlayerSkia&, Gfx::ShareableBitmap&); bool acknowledge_presented_bitmap(i32 bitmap_id); void did_finish_gpu_present(i32 bitmap_id); diff --git a/Services/Compositor/ViewportScrollbarController.cpp b/Services/Compositor/ViewportScrollbarController.cpp index a227f16696..a45b421368 100644 --- a/Services/Compositor/ViewportScrollbarController.cpp +++ b/Services/Compositor/ViewportScrollbarController.cpp @@ -29,7 +29,7 @@ static ViewportScrollbarIdentity viewport_scrollbar_identity(Web::Compositor::Vi static Optional viewport_scrollbar_identity_at(ReadonlySpan scrollbars, Optional scrollbar_index) { - if (!scrollbar_index.has_value() || *scrollbar_index >= scrollbars.size()) + if (!scrollbar_index.has_value()) return {}; return viewport_scrollbar_identity(scrollbars[*scrollbar_index]); } @@ -159,10 +159,6 @@ Optional ViewportScrollbarController::capture if (!m_captured_scrollbar_index.has_value()) return {}; auto scrollbar_index = *m_captured_scrollbar_index; - if (scrollbar_index >= m_scrollbars.size()) { - m_captured_scrollbar_index.clear(); - return {}; - } auto const& scrollbar = m_scrollbars[scrollbar_index]; auto primary_position = position.primary_offset_for_orientation(orientation_for_scrollbar(scrollbar)); return Drag { scrollbar_index, primary_position, m_thumb_grab_position }; @@ -174,10 +170,6 @@ Optional ViewportScrollbarController::release return {}; auto scrollbar_index = *m_captured_scrollbar_index; auto thumb_grab_position = m_thumb_grab_position; - if (scrollbar_index >= m_scrollbars.size()) { - m_captured_scrollbar_index.clear(); - return {}; - } auto const& scrollbar = m_scrollbars[scrollbar_index]; auto primary_position = position.primary_offset_for_orientation(orientation_for_scrollbar(scrollbar)); m_captured_scrollbar_index.clear(); @@ -196,9 +188,6 @@ bool ViewportScrollbarController::set_hovered_scrollbar(Optional scrollb Optional ViewportScrollbarController::scroll_delta_for_drag(Web::Compositor::AsyncScrollTree const& async_scroll_tree, Web::Painting::ScrollStateSnapshot const& scroll_state_snapshot, Drag const& drag) const { - if (drag.scrollbar_index >= m_scrollbars.size()) - return {}; - auto const& scrollbar = m_scrollbars[drag.scrollbar_index]; auto expanded = is_expanded(drag.scrollbar_index); auto scroll_size = scrollbar_scroll_size(scrollbar, expanded);