From c4b13589e9ee377c8b1fa530051989ecc84d5dd8 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 30 Jul 2025 14:38:41 +0200 Subject: [PATCH] LibWeb: Make StyleComputer and FontLoader GC-allocated This allows them to keep style sheets alive while loading fonts for them. Fixes some GC crashes seen on the WPT WOFF2 tests after 66a19b85501dad8c0bfc5577b8d0b835877ad0dc stopped FetchRecord leaks from keeping various other things alive. --- Libraries/LibWeb/CSS/CSSStyleSheet.cpp | 1 + Libraries/LibWeb/CSS/CSSStyleSheet.h | 4 +- Libraries/LibWeb/CSS/FontFace.cpp | 2 +- Libraries/LibWeb/CSS/StyleComputer.cpp | 61 ++++++++++++++++---------- Libraries/LibWeb/CSS/StyleComputer.h | 30 ++++++++----- Libraries/LibWeb/DOM/Document.cpp | 4 +- Libraries/LibWeb/DOM/Document.h | 2 +- 7 files changed, 65 insertions(+), 39 deletions(-) diff --git a/Libraries/LibWeb/CSS/CSSStyleSheet.cpp b/Libraries/LibWeb/CSS/CSSStyleSheet.cpp index f37c5ddd907..a3ad2c07977 100644 --- a/Libraries/LibWeb/CSS/CSSStyleSheet.cpp +++ b/Libraries/LibWeb/CSS/CSSStyleSheet.cpp @@ -128,6 +128,7 @@ void CSSStyleSheet::visit_edges(Cell::Visitor& visitor) visitor.visit(m_namespace_rules); visitor.visit(m_import_rules); visitor.visit(m_owning_documents_or_shadow_roots); + visitor.visit(m_associated_font_loaders); } // https://www.w3.org/TR/cssom/#dom-cssstylesheet-insertrule diff --git a/Libraries/LibWeb/CSS/CSSStyleSheet.h b/Libraries/LibWeb/CSS/CSSStyleSheet.h index e650e40f0d1..c6c92596b0b 100644 --- a/Libraries/LibWeb/CSS/CSSStyleSheet.h +++ b/Libraries/LibWeb/CSS/CSSStyleSheet.h @@ -91,7 +91,7 @@ public: void set_source_text(String); Optional source_text(Badge) const; - void add_associated_font_loader(WeakPtr font_loader) + void add_associated_font_loader(GC::Ref font_loader) { m_associated_font_loaders.append(font_loader); } @@ -126,7 +126,7 @@ private: bool m_disallow_modification { false }; Optional m_did_match; - Vector> m_associated_font_loaders; + Vector> m_associated_font_loaders; }; } diff --git a/Libraries/LibWeb/CSS/FontFace.cpp b/Libraries/LibWeb/CSS/FontFace.cpp index 914df675291..e5822143417 100644 --- a/Libraries/LibWeb/CSS/FontFace.cpp +++ b/Libraries/LibWeb/CSS/FontFace.cpp @@ -499,7 +499,7 @@ GC::Ref FontFace::load() {}, // FIXME: feature_settings {}, // FIXME: variation_settings }; - if (auto loader = style_computer.load_font_face(parsed_font_face, move(on_load)); loader.has_value()) + if (auto loader = style_computer.load_font_face(parsed_font_face, move(on_load))) loader->start_loading_next_url(); } else { // FIXME: Don't know how to load fonts in workers! They don't have a StyleComputer diff --git a/Libraries/LibWeb/CSS/StyleComputer.cpp b/Libraries/LibWeb/CSS/StyleComputer.cpp index 953d8628d2e..df3ade7d0b6 100644 --- a/Libraries/LibWeb/CSS/StyleComputer.cpp +++ b/Libraries/LibWeb/CSS/StyleComputer.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2023, Andreas Kling + * Copyright (c) 2018-2025, Andreas Kling * Copyright (c) 2021, the SerenityOS developers. * Copyright (c) 2021-2025, Sam Atkins * Copyright (c) 2024, Matthew Olsson @@ -94,6 +94,9 @@ namespace Web::CSS { +GC_DEFINE_ALLOCATOR(StyleComputer); +GC_DEFINE_ALLOCATOR(FontLoader); + struct FontFaceKey { NonnullRawPtr family_name; int weight { 0 }; @@ -184,11 +187,20 @@ StyleComputer::StyleComputer(DOM::Document& document) , m_default_font_metrics(16, Platform::FontPlugin::the().default_font(16)->pixel_metrics()) , m_root_element_font_metrics(m_default_font_metrics) { + m_ancestor_filter = make>(); m_qualified_layer_names_in_order.append({}); } StyleComputer::~StyleComputer() = default; +void StyleComputer::visit_edges(Visitor& visitor) +{ + Base::visit_edges(visitor); + visitor.visit(m_document); + visitor.visit(m_loaded_fonts); + visitor.visit(m_user_style_sheet); +} + FontLoader::FontLoader(StyleComputer& style_computer, GC::Ptr parent_style_sheet, FlyString family_name, Vector unicode_ranges, Vector urls, Function)> on_load) : m_style_computer(style_computer) , m_parent_style_sheet(parent_style_sheet) @@ -201,6 +213,14 @@ FontLoader::FontLoader(StyleComputer& style_computer, GC::Ptr par FontLoader::~FontLoader() = default; +void FontLoader::visit_edges(Visitor& visitor) +{ + Base::visit_edges(visitor); + visitor.visit(m_style_computer); + visitor.visit(m_parent_style_sheet); + visitor.visit(m_fetch_controller); +} + bool FontLoader::is_loading() const { return m_fetch_controller && !m_vector_font; @@ -228,34 +248,29 @@ void FontLoader::start_loading_next_url() // To fetch a font given a selected url for @font-face rule, fetch url, with stylesheet being rule’s parent // CSS style sheet, destination "font", CORS mode "cors", and processResponse being the following steps given // response res and null, failure or a byte stream stream: - auto style_sheet_or_document = m_parent_style_sheet ? StyleSheetOrDocument { *m_parent_style_sheet } : StyleSheetOrDocument { m_style_computer.document() }; + auto style_sheet_or_document = m_parent_style_sheet ? StyleSheetOrDocument { *m_parent_style_sheet } : StyleSheetOrDocument { m_style_computer->document() }; auto maybe_fetch_controller = fetch_a_style_resource(m_urls.take_first(), style_sheet_or_document, Fetch::Infrastructure::Request::Destination::Font, CorsMode::Cors, - [weak_loader = make_weak_ptr()](auto response, auto stream) { - // NB: If the FontLoader died before this fetch completed, nobody wants the data. - if (weak_loader.is_null()) - return; - auto& loader = *weak_loader; - + [loader = this](auto response, auto stream) { // 1. If stream is null, return. // 2. Load a font from stream according to its type. // NB: We need to fetch the next source if this one fails to fetch OR decode. So, first try to decode it. RefPtr typeface; if (auto* bytes = stream.template get_pointer()) { - if (auto maybe_typeface = loader.try_load_font(response, *bytes); !maybe_typeface.is_error()) + if (auto maybe_typeface = loader->try_load_font(response, *bytes); !maybe_typeface.is_error()) typeface = maybe_typeface.release_value(); } if (!typeface) { // NB: If we have other sources available, try the next one. - if (loader.m_urls.is_empty()) { - loader.font_did_load_or_fail(nullptr); + if (loader->m_urls.is_empty()) { + loader->font_did_load_or_fail(nullptr); } else { - loader.m_fetch_controller = nullptr; - loader.start_loading_next_url(); + loader->m_fetch_controller = nullptr; + loader->start_loading_next_url(); } } else { - loader.font_did_load_or_fail(move(typeface)); + loader->font_did_load_or_fail(move(typeface)); } }); @@ -270,7 +285,7 @@ void FontLoader::font_did_load_or_fail(RefPtr typeface) { if (typeface) { m_vector_font = typeface.release_nonnull(); - m_style_computer.did_load_font(m_family_name); + m_style_computer->did_load_font(m_family_name); if (m_on_load) m_on_load(m_vector_font); } else { @@ -3088,7 +3103,7 @@ void StyleComputer::did_load_font(FlyString const&) document().invalidate_style(DOM::StyleInvalidationReason::CSSFontLoaded); } -Optional StyleComputer::load_font_face(ParsedFontFace const& font_face, Function)> on_load) +GC::Ptr StyleComputer::load_font_face(ParsedFontFace const& font_face, Function)> on_load) { if (font_face.sources().is_empty()) { if (on_load) @@ -3116,14 +3131,14 @@ Optional StyleComputer::load_font_face(ParsedFontFace const& font_f return {}; } - auto loader = make(*this, font_face.parent_style_sheet(), font_face.font_family(), font_face.unicode_ranges(), move(urls), move(on_load)); + auto loader = heap().allocate(*this, font_face.parent_style_sheet(), font_face.font_family(), font_face.unicode_ranges(), move(urls), move(on_load)); auto& loader_ref = *loader; auto maybe_font_loaders_list = m_loaded_fonts.get(key); if (maybe_font_loaders_list.has_value()) { maybe_font_loaders_list->append(move(loader)); } else { FontLoaderList loaders; - loaders.append(move(loader)); + loaders.append(loader); m_loaded_fonts.set(OwnFontFaceKey(key), move(loaders)); } // Actual object owned by font loader list inside m_loaded_fonts, this isn't use-after-move/free @@ -3138,8 +3153,8 @@ void StyleComputer::load_fonts_from_sheet(CSSStyleSheet& sheet) auto const& font_face_rule = static_cast(*rule); if (!font_face_rule.is_valid()) continue; - if (auto font_loader = load_font_face(font_face_rule.font_face()); font_loader.has_value()) { - sheet.add_associated_font_loader(font_loader.value()); + if (auto font_loader = load_font_face(font_face_rule.font_face())) { + sheet.add_associated_font_loader(*font_loader); } } } @@ -3276,20 +3291,20 @@ static void for_each_element_hash(DOM::Element const& element, auto callback) void StyleComputer::reset_ancestor_filter() { - m_ancestor_filter.clear(); + m_ancestor_filter->clear(); } void StyleComputer::push_ancestor(DOM::Element const& element) { for_each_element_hash(element, [&](u32 hash) { - m_ancestor_filter.increment(hash); + m_ancestor_filter->increment(hash); }); } void StyleComputer::pop_ancestor(DOM::Element const& element) { for_each_element_hash(element, [&](u32 hash) { - m_ancestor_filter.decrement(hash); + m_ancestor_filter->decrement(hash); }); } diff --git a/Libraries/LibWeb/CSS/StyleComputer.h b/Libraries/LibWeb/CSS/StyleComputer.h index f0110a85806..57a38f096d8 100644 --- a/Libraries/LibWeb/CSS/StyleComputer.h +++ b/Libraries/LibWeb/CSS/StyleComputer.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2024, Andreas Kling + * Copyright (c) 2018-2025, Andreas Kling * Copyright (c) 2021-2024, Sam Atkins * * SPDX-License-Identifier: BSD-2-Clause @@ -126,7 +126,10 @@ struct RuleCache { class FontLoader; -class StyleComputer { +class StyleComputer final : public GC::Cell { + GC_CELL(StyleComputer, GC::Cell); + GC_DECLARE_ALLOCATOR(StyleComputer); + public: static void for_each_property_expanding_shorthands(PropertyID, CSSStyleValue const&, Function const& set_longhand_property); static NonnullRefPtr get_inherit_value(CSS::PropertyID, DOM::Element const*, Optional = {}); @@ -162,7 +165,7 @@ public: void did_load_font(FlyString const& family_name); - Optional load_font_face(ParsedFontFace const&, ESCAPING Function)> on_load = {}); + GC::Ptr load_font_face(ParsedFontFace const&, ESCAPING Function)> on_load = {}); void load_fonts_from_sheet(CSSStyleSheet&); void unload_fonts_from_sheet(CSSStyleSheet&); @@ -196,6 +199,8 @@ public: static NonnullRefPtr compute_value_of_custom_property(DOM::AbstractElement, FlyString const& custom_property, Optional = {}); private: + virtual void visit_edges(Visitor&) override; + enum class ComputeStyleMode { Normal, CreatePseudoElementStyleIfNeeded, @@ -283,9 +288,9 @@ private: OwnPtr m_author_rule_cache; OwnPtr m_user_rule_cache; OwnPtr m_user_agent_rule_cache; - GC::Root m_user_style_sheet; + GC::Ptr m_user_style_sheet; - using FontLoaderList = Vector>; + using FontLoaderList = Vector>; HashMap m_loaded_fonts; [[nodiscard]] Length::FontMetrics const& root_element_font_metrics_for_element(GC::Ptr) const; @@ -295,10 +300,13 @@ private: CSSPixelRect m_viewport_rect; - CountingBloomFilter m_ancestor_filter; + OwnPtr> m_ancestor_filter; }; -class FontLoader : public Weakable { +class FontLoader final : public GC::Cell { + GC_CELL(FontLoader, GC::Cell); + GC_DECLARE_ALLOCATOR(FontLoader); + public: FontLoader(StyleComputer& style_computer, GC::Ptr parent_style_sheet, FlyString family_name, Vector unicode_ranges, Vector urls, ESCAPING Function)> on_load = {}); @@ -313,17 +321,19 @@ public: bool is_loading() const; private: + virtual void visit_edges(Visitor&) override; + ErrorOr> try_load_font(Fetch::Infrastructure::Response const&, ByteBuffer const&); void font_did_load_or_fail(RefPtr); - StyleComputer& m_style_computer; + GC::Ref m_style_computer; GC::Ptr m_parent_style_sheet; FlyString m_family_name; Vector m_unicode_ranges; RefPtr m_vector_font; Vector m_urls; - GC::Root m_fetch_controller; + GC::Ptr m_fetch_controller; Function)> m_on_load; }; @@ -332,7 +342,7 @@ inline bool StyleComputer::should_reject_with_ancestor_filter(Selector const& se for (u32 hash : selector.ancestor_hashes()) { if (hash == 0) break; - if (!m_ancestor_filter.may_contain(hash)) + if (!m_ancestor_filter->may_contain(hash)) return true; } return false; diff --git a/Libraries/LibWeb/DOM/Document.cpp b/Libraries/LibWeb/DOM/Document.cpp index 8a62b3dbaff..7b47e8ef9dd 100644 --- a/Libraries/LibWeb/DOM/Document.cpp +++ b/Libraries/LibWeb/DOM/Document.cpp @@ -459,7 +459,7 @@ GC::Ref Document::create_for_fragment_parsing(JS::Realm& realm) Document::Document(JS::Realm& realm, const URL::URL& url, TemporaryDocumentForFragmentParsing temporary_document_for_fragment_parsing) : ParentNode(realm, *this, NodeType::DOCUMENT_NODE) , m_page(Bindings::principal_host_defined_page(realm)) - , m_style_computer(make(*this)) + , m_style_computer(realm.heap().allocate(*this)) , m_url(url) , m_temporary_document_for_fragment_parsing(temporary_document_for_fragment_parsing) , m_editing_host_manager(EditingHostManager::create(realm, *this)) @@ -557,7 +557,7 @@ void Document::visit_edges(Cell::Visitor& visitor) visitor.visit(m_appropriate_template_contents_owner_document); visitor.visit(m_pending_parsing_blocking_script); visitor.visit(m_history); - + visitor.visit(m_style_computer); visitor.visit(m_browsing_context); visitor.visit(m_applets); diff --git a/Libraries/LibWeb/DOM/Document.h b/Libraries/LibWeb/DOM/Document.h index 3c9b93f94dd..3664da98062 100644 --- a/Libraries/LibWeb/DOM/Document.h +++ b/Libraries/LibWeb/DOM/Document.h @@ -965,7 +965,7 @@ private: void run_csp_initialization() const; GC::Ref m_page; - OwnPtr m_style_computer; + GC::Ptr m_style_computer; GC::Ptr m_style_sheets; GC::Ptr m_active_favicon; GC::Ptr m_browsing_context;