File chromium-125-debian-bad-font-gc11.patch of Package chromium

Overview Repositories Revisions Requests Users Attributes Meta

File chromium-125-debian-bad-font-gc11.patch of Package chromium (Revision 421049c7479897d1e85aa94e2be26df9)

Currently displaying revision 421049c7479897d1e85aa94e2be26df9 , Show latest

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

Places

File chromium-125-debian-bad-font-gc11.patch of Package chromium (Revision 421049c7479897d1e85aa94e2be26df9)

Places