Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
home:hillwood:branches:network:chromium
chromium
chromium-125-debian-bad-font-gc11.patch
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File chromium-125-debian-bad-font-gc11.patch of Package chromium
Revert the following commit: commit 2eefeabb12fb7e92f2508116a5ed959c57659be1 Author: Ian Kilpatrick <ikilpatrick@chromium.org> Date: Tue Feb 20 17:40:39 2024 +0000 [gc] Make HarfBuzzFontData & friends gc'd. Previously we had a HbFontCacheEntry which was used to hold onto the HarfBuzzFontData, and a hb_font_t. HarfBuzzFontData is used for holding data specific for various harfbuzz callbacks, but we can also hold onto the hb_font_t there. There should be no user-visible behaviour change. Bug: 41490008 Change-Id: Icaa7ad3b2f75e9807b88014a9a15406cb76eb52e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5302175 Reviewed-by: Dominik Röttsches <drott@chromium.org> Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1262752} --- a/third_party/blink/renderer/platform/fonts/font_global_context.cc +++ b/third_party/blink/renderer/platform/fonts/font_global_context.cc @@ -8,6 +8,7 @@ #include "third_party/blink/renderer/platform/fonts/font_cache.h" #include "third_party/blink/renderer/platform/fonts/font_unique_name_lookup.h" #include "third_party/blink/renderer/platform/fonts/shaping/harfbuzz_face.h" +#include "third_party/blink/renderer/platform/fonts/shaping/harfbuzz_font_cache.h" #include "third_party/blink/renderer/platform/privacy_budget/identifiability_digest_helpers.h" #include "third_party/blink/renderer/platform/wtf/thread_specific.h" @@ -50,6 +51,15 @@ FontUniqueNameLookup* FontGlobalContext: return Get().font_unique_name_lookup_.get(); } +HarfBuzzFontCache& FontGlobalContext::GetHarfBuzzFontCache() { + std::unique_ptr<HarfBuzzFontCache>& global_context_harfbuzz_font_cache = + Get().harfbuzz_font_cache_; + if (!global_context_harfbuzz_font_cache) { + global_context_harfbuzz_font_cache = std::make_unique<HarfBuzzFontCache>(); + } + return *global_context_harfbuzz_font_cache; +} + IdentifiableToken FontGlobalContext::GetOrComputeTypefaceDigest( const FontPlatformData& source) { SkTypeface* typeface = source.Typeface(); --- a/third_party/blink/renderer/platform/fonts/font_global_context.h +++ b/third_party/blink/renderer/platform/fonts/font_global_context.h @@ -9,7 +9,6 @@ #include "base/types/pass_key.h" #include "third_party/blink/public/common/privacy_budget/identifiable_token.h" #include "third_party/blink/renderer/platform/fonts/font_cache.h" -#include "third_party/blink/renderer/platform/fonts/shaping/harfbuzz_font_cache.h" #include "third_party/blink/renderer/platform/platform_export.h" #include "third_party/blink/renderer/platform/text/layout_locale.h" #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h" @@ -34,19 +33,14 @@ class PLATFORM_EXPORT FontGlobalContext static FontGlobalContext& Get(); static FontGlobalContext* TryGet(); - void Trace(Visitor* visitor) const { - visitor->Trace(font_cache_); - visitor->Trace(harfbuzz_font_cache_); - } + void Trace(Visitor* visitor) const { visitor->Trace(font_cache_); } FontGlobalContext(const FontGlobalContext&) = delete; FontGlobalContext& operator=(const FontGlobalContext&) = delete; static inline FontCache& GetFontCache() { return Get().font_cache_; } - static HarfBuzzFontCache& GetHarfBuzzFontCache() { - return Get().harfbuzz_font_cache_; - } + static HarfBuzzFontCache& GetHarfBuzzFontCache(); static FontUniqueNameLookup* GetFontUniqueNameLookup(); @@ -62,7 +56,7 @@ class PLATFORM_EXPORT FontGlobalContext private: FontCache font_cache_; - HarfBuzzFontCache harfbuzz_font_cache_; + std::unique_ptr<HarfBuzzFontCache> harfbuzz_font_cache_; std::unique_ptr<FontUniqueNameLookup> font_unique_name_lookup_; base::HashingLRUCache<SkTypefaceID, IdentifiableToken> typeface_digest_cache_; base::HashingLRUCache<SkTypefaceID, IdentifiableToken> --- a/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_face.cc +++ b/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_face.cc @@ -65,14 +65,20 @@ HarfBuzzFace::HarfBuzzFace(const FontPlatformData* platform_data, uint64_t unique_id) - : platform_data_(platform_data), - harfbuzz_font_data_(FontGlobalContext::GetHarfBuzzFontCache().GetOrCreate( - unique_id, - platform_data)) {} + : platform_data_(platform_data), unique_id_(unique_id) { + HbFontCacheEntry* const cache_entry = + FontGlobalContext::GetHarfBuzzFontCache().RefOrNew(unique_id_, + platform_data); + unscaled_font_ = cache_entry->HbFont(); + harfbuzz_font_data_ = cache_entry->HbFontData(); +} + +HarfBuzzFace::~HarfBuzzFace() { + FontGlobalContext::GetHarfBuzzFontCache().Remove(unique_id_); +} void HarfBuzzFace::Trace(Visitor* visitor) const { visitor->Trace(platform_data_); - visitor->Trace(harfbuzz_font_data_); } bool HarfBuzzFace::ignore_variation_selectors_ = false; @@ -234,17 +240,14 @@ bool HarfBuzzFace::HasSpaceInLigaturesOr hb::unique_ptr<hb_set_t> glyphs(hb_set_create()); - hb_font_t* unscaled_font = harfbuzz_font_data_->unscaled_font_.get(); - // Check whether computing is needed and compute for gpos/gsub. if (features & kKerning && harfbuzz_font_data_->space_in_gpos_ == HarfBuzzFontData::SpaceGlyphInOpenTypeTables::kUnknown) { - if (space == kInvalidCodepoint && !GetSpaceGlyph(unscaled_font, space)) { + if (space == kInvalidCodepoint && !GetSpaceGlyph(unscaled_font_, space)) return false; - } // Compute for gpos. - hb_face_t* face = hb_font_get_face(unscaled_font); + hb_face_t* face = hb_font_get_face(unscaled_font_); DCHECK(face); harfbuzz_font_data_->space_in_gpos_ = hb_ot_layout_has_positioning(face) && @@ -258,11 +261,10 @@ bool HarfBuzzFace::HasSpaceInLigaturesOr if (features & kLigatures && harfbuzz_font_data_->space_in_gsub_ == HarfBuzzFontData::SpaceGlyphInOpenTypeTables::kUnknown) { - if (space == kInvalidCodepoint && !GetSpaceGlyph(unscaled_font, space)) { + if (space == kInvalidCodepoint && !GetSpaceGlyph(unscaled_font_, space)) return false; - } // Compute for gpos. - hb_face_t* face = hb_font_get_face(unscaled_font); + hb_face_t* face = hb_font_get_face(unscaled_font_); DCHECK(face); harfbuzz_font_data_->space_in_gsub_ = hb_ot_layout_has_substitution(face) && @@ -280,14 +282,14 @@ bool HarfBuzzFace::HasSpaceInLigaturesOr } unsigned HarfBuzzFace::UnitsPerEmFromHeadTable() { - hb_face_t* face = hb_font_get_face(harfbuzz_font_data_->unscaled_font_.get()); + hb_face_t* face = hb_font_get_face(unscaled_font_); return hb_face_get_upem(face); } Glyph HarfBuzzFace::HbGlyphForCharacter(UChar32 character) { hb_codepoint_t glyph = 0; - HarfBuzzGetNominalGlyph(harfbuzz_font_data_->unscaled_font_.get(), - harfbuzz_font_data_, character, &glyph, nullptr); + HarfBuzzGetNominalGlyph(unscaled_font_, harfbuzz_font_data_, character, + &glyph, nullptr); return glyph; } @@ -329,7 +331,7 @@ hb_codepoint_t HarfBuzzFace::HarfBuzzGet UChar32 variation_selector) { DCHECK(RuntimeEnabledFeatures::FontVariationSequencesEnabled()); hb_codepoint_t glyph = 0; - HarfBuzzGetGlyph(harfbuzz_font_data_->unscaled_font_.get(), + HarfBuzzGetGlyph(unscaled_font_, harfbuzz_font_data_, character, variation_selector, &glyph, nullptr); return glyph; @@ -444,10 +446,9 @@ static hb::unique_ptr<hb_face_t> CreateF return face; } -namespace { - -HarfBuzzFontData* CreateHarfBuzzFontData(hb_face_t* face, - SkTypeface* typeface) { +static scoped_refptr<HbFontCacheEntry> CreateHbFontCacheEntry( + hb_face_t* face, + SkTypeface* typeface) { hb::unique_ptr<hb_font_t> ot_font(hb_font_create(face)); hb_ot_font_set_funcs(ot_font.get()); @@ -466,26 +467,25 @@ HarfBuzzFontData* CreateHarfBuzzFontData // Creating a sub font means that non-available functions // are found from the parent. hb_font_t* const unscaled_font = hb_font_create_sub_font(ot_font.get()); - HarfBuzzFontData* data = - MakeGarbageCollected<HarfBuzzFontData>(unscaled_font); + scoped_refptr<HbFontCacheEntry> cache_entry = + HbFontCacheEntry::Create(unscaled_font); hb_font_set_funcs(unscaled_font, - HarfBuzzSkiaFontFuncs::Get().GetFunctions(typeface), data, - nullptr); - return data; + HarfBuzzSkiaFontFuncs::Get().GetFunctions(typeface), + cache_entry->HbFontData(), nullptr); + return cache_entry; } -} // namespace - -HarfBuzzFontData* HarfBuzzFontCache::GetOrCreate( +HbFontCacheEntry* HarfBuzzFontCache::RefOrNew( uint64_t unique_id, const FontPlatformData* platform_data) { const auto& result = font_map_.insert(unique_id, nullptr); if (result.is_new_entry) { hb::unique_ptr<hb_face_t> face = CreateFace(platform_data); result.stored_value->value = - CreateHarfBuzzFontData(face.get(), platform_data->Typeface()); + CreateHbFontCacheEntry(face.get(), platform_data->Typeface()); } - return result.stored_value->value.Get(); + result.stored_value->value->AddRef(); + return result.stored_value->value.get(); } static_assert( @@ -516,18 +516,17 @@ hb_font_t* HarfBuzzFace::GetScaledFont(s vertical_layout); int scale = SkiaScalarToHarfBuzzPosition(platform_data_->size()); - hb_font_t* unscaled_font = harfbuzz_font_data_->unscaled_font_.get(); - hb_font_set_scale(unscaled_font, scale, scale); + hb_font_set_scale(unscaled_font_, scale, scale); // See contended discussion in https://github.com/harfbuzz/harfbuzz/pull/1484 // Setting ptem here is critical for HarfBuzz to know where to lookup spacing // offset in the AAT trak table, the unit pt in ptem here means "CoreText" // points. After discussion on the pull request and with Apple developers, the // meaning of HarfBuzz' hb_font_set_ptem API was changed to expect the // equivalent of CSS pixels here. - hb_font_set_ptem(unscaled_font, specified_size > 0 ? specified_size - : platform_data_->size()); + hb_font_set_ptem(unscaled_font_, specified_size > 0 ? specified_size + : platform_data_->size()); - return unscaled_font; + return unscaled_font_; } hb_font_t* HarfBuzzFace::GetScaledFont() const { --- a/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_face.h +++ b/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_face.h @@ -55,6 +55,7 @@ class HarfBuzzFace final : public Garbag HarfBuzzFace(const FontPlatformData* platform_data, uint64_t); HarfBuzzFace(const HarfBuzzFace&) = delete; HarfBuzzFace& operator=(const HarfBuzzFace&) = delete; + ~HarfBuzzFace(); void Trace(Visitor*) const; @@ -106,7 +106,11 @@ void PrepareHarfBuzzFontData(); Member<const FontPlatformData> platform_data_; - Member<HarfBuzzFontData> harfbuzz_font_data_; + const uint64_t unique_id_; + // TODO(crbug.com/1489080): When briefly given MiraclePtr protection, + // these members were both found dangling. + hb_font_t* unscaled_font_; + HarfBuzzFontData* harfbuzz_font_data_; static bool ignore_variation_selectors_; }; --- a/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_font_cache.cc +++ b/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_font_cache.cc @@ -8,8 +8,38 @@ namespace blink { -void HarfBuzzFontCache::Trace(Visitor* visitor) const { - visitor->Trace(font_map_); +HbFontCacheEntry::HbFontCacheEntry(hb_font_t* font) + : hb_font_(hb::unique_ptr<hb_font_t>(font)), + hb_font_data_(std::make_unique<HarfBuzzFontData>()) {} + +HbFontCacheEntry::~HbFontCacheEntry() = default; + +scoped_refptr<HbFontCacheEntry> HbFontCacheEntry::Create(hb_font_t* hb_font) { + DCHECK(hb_font); + return base::AdoptRef(new HbFontCacheEntry(hb_font)); +} + +HarfBuzzFontCache::HarfBuzzFontCache() = default; +HarfBuzzFontCache::~HarfBuzzFontCache() = default; + +// See "harfbuzz_face.cc" for |HarfBuzzFontCache::GetOrCreateFontData()| +// implementation. + +void HarfBuzzFontCache::Remove(uint64_t unique_id) { + auto it = font_map_.find(unique_id); + // TODO(https://crbug.com/1417160): In tests such as FontObjectThreadedTest + // that test taking down FontGlobalContext an object may not be found due to + // existing issues with refcounting of font objects at thread destruction + // time. + if (it == font_map_.end()) { + return; + } + DCHECK(!it.Get()->value->HasOneRef()); + it.Get()->value->Release(); + if (!it.Get()->value->HasOneRef()) { + return; + } + font_map_.erase(it); } } // namespace blink --- a/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_font_cache.h +++ b/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_font_cache.h @@ -6,9 +6,12 @@ #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_FONTS_SHAPING_HARFBUZZ_FONT_CACHE_H_ #include "third_party/blink/renderer/platform/fonts/font_metrics.h" -#include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_map.h" -#include "third_party/blink/renderer/platform/heap/garbage_collected.h" -#include "third_party/blink/renderer/platform/heap/member.h" +#include "third_party/blink/renderer/platform/fonts/unicode_range_set.h" + +#include <hb.h> +#include <hb-cplusplus.hh> + +#include <memory> namespace blink { @@ -22,21 +25,39 @@ struct HarfBuzzFontData; // FIXME, crbug.com/609099: We should fix the FontCache to only keep one // FontPlatformData object independent of size, then consider using this here. -class HarfBuzzFontCache final { - DISALLOW_NEW(); +class HbFontCacheEntry : public RefCounted<HbFontCacheEntry> { + USING_FAST_MALLOC(HbFontCacheEntry); + + public: + static scoped_refptr<HbFontCacheEntry> Create(hb_font_t* hb_font); + + hb_font_t* HbFont() { return hb_font_.get(); } + HarfBuzzFontData* HbFontData() { return hb_font_data_.get(); } + + ~HbFontCacheEntry(); + private: + explicit HbFontCacheEntry(hb_font_t* font); + + hb::unique_ptr<hb_font_t> hb_font_; + std::unique_ptr<HarfBuzzFontData> hb_font_data_; +}; + +class HarfBuzzFontCache final { public: - void Trace(Visitor* visitor) const; - // See "harfbuzz_face.cc" for |HarfBuzzFontCache::GetOrCreateFontData()| - // implementation. - HarfBuzzFontData* GetOrCreate(uint64_t unique_id, - const FontPlatformData* platform_data); + HarfBuzzFontCache(); + ~HarfBuzzFontCache(); + + HbFontCacheEntry* RefOrNew(uint64_t unique_id, + const FontPlatformData* platform_data); + void Remove(uint64_t unique_id); private: - HeapHashMap<uint64_t, - WeakMember<HarfBuzzFontData>, - IntWithZeroKeyHashTraits<uint64_t>> - font_map_; + using HbFontDataMap = HashMap<uint64_t, + scoped_refptr<HbFontCacheEntry>, + IntWithZeroKeyHashTraits<uint64_t>>; + + HbFontDataMap font_map_; }; } // namespace blink --- a/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_font_data.h +++ b/third_party/blink/renderer/platform/fonts/shaping/harfbuzz_font_data.h @@ -22,18 +22,15 @@ const unsigned kInvalidFallbackMetricsVa // The HarfBuzzFontData struct carries user-pointer data for // |hb_font_t| callback functions/operations. It contains metrics and OpenType // layout information related to a font scaled to a particular size. -struct HarfBuzzFontData final : public GarbageCollected<HarfBuzzFontData> { +struct HarfBuzzFontData final { + USING_FAST_MALLOC(HarfBuzzFontData); + public: - explicit HarfBuzzFontData(hb_font_t* unscaled_font) - : unscaled_font_(hb::unique_ptr<hb_font_t>(unscaled_font)), - vertical_data_(nullptr), - range_set_(nullptr) {} + HarfBuzzFontData() : vertical_data_(nullptr), range_set_(nullptr) {} HarfBuzzFontData(const HarfBuzzFontData&) = delete; HarfBuzzFontData& operator=(const HarfBuzzFontData&) = delete; - void Trace(Visitor*) const {} - // The vertical origin and vertical advance functions in HarfBuzzFace require // the ascent and height metrics as fallback in case no specific vertical // layout information is found from the font. @@ -81,7 +78,6 @@ struct HarfBuzzFontData final : public G return vertical_data_; } - const hb::unique_ptr<hb_font_t> unscaled_font_; SkFont font_; // Capture these scaled fallback metrics from FontPlatformData so that a
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
