rpm/nodejs-electron
SHA256
1
0
Fork 0
forked from pool/nodejs-electron
Code Pull Requests Activity
Files
devel
nodejs-electron/bad-font-gc11.patch

412 lines
16 KiB
Diff
Raw Permalink Blame History

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
@@ -64,14 +64,20 @@ namespace blink {
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_);
}
static hb_bool_t HarfBuzzGetGlyph(hb_font_t* hb_font,
@@ -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;
}
@@ -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;
@@ -90,7 +91,11 @@ class HarfBuzzFace final : public Garbag
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_;
};
} // namespace blink
--- 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
View Git Blame Copy Permalink