From 34e4bceb4d0b37378c8d89547e51c28576865fde Mon Sep 17 00:00:00 2001 From: Ben Wagner Date: Tue, 17 Mar 2026 14:57:54 -0400 Subject: [PATCH] Fix ownership of mapped_font_files_ values Change mapped_font_files_ values from `raw_ptr` to `std::weak_ptr`. This allows the values in the map to safely expire while still in the map. When the values are finally destroyed they are still removed from the map to prevent accumulation or expired values. Change-Id: I96a2690c2a0c648838e4411075e90810d901a72e --- .../services/font/public/cpp/font_loader.cc | 25 +++++++++++-------- .../services/font/public/cpp/font_loader.h | 4 +-- .../font/public/cpp/font_service_thread.cc | 8 +++--- .../font/public/cpp/font_service_thread.h | 3 ++- .../font/public/cpp/mapped_font_file.cc | 22 ++++++++++------ .../font/public/cpp/mapped_font_file.h | 8 +++--- 6 files changed, 40 insertions(+), 30 deletions(-) diff --git a/components/services/font/public/cpp/font_loader.cc b/components/services/font/public/cpp/font_loader.cc index adad666abf1c5..b9280f0e8ad3c 100644 --- a/components/services/font/public/cpp/font_loader.cc +++ b/components/services/font/public/cpp/font_loader.cc @@ -59,26 +59,31 @@ SkStreamAsset* FontLoader::openStream(const FontIdentity& identity) { { base::AutoLock lock(mapped_font_files_lock_); auto mapped_font_files_it = mapped_font_files_.find(identity.fID); - if (mapped_font_files_it != mapped_font_files_.end()) - return mapped_font_files_it->second->CreateMemoryStream(); + if (mapped_font_files_it != mapped_font_files_.end()) { + std::shared_ptr mff = + mapped_font_files_it->second.lock(); + if (mff) { + return internal::MappedFontFile::CreateMemoryStream(std::move(mff)); + } else { + mapped_font_files_.erase(mapped_font_files_it); + } + } } - scoped_refptr mapped_font_file = + std::shared_ptr mapped_font_file = thread_->OpenStream(identity); if (!mapped_font_file) return nullptr; - // Get notified with |mapped_font_file| is destroyed. + // Get notified when |mapped_font_file| is destroyed. mapped_font_file->set_observer(this); { base::AutoLock lock(mapped_font_files_lock_); - auto mapped_font_files_it = - mapped_font_files_ - .insert(std::make_pair(mapped_font_file->font_id(), - mapped_font_file.get())) - .first; - return mapped_font_files_it->second->CreateMemoryStream(); + mapped_font_files_.insert(std::make_pair(mapped_font_file->font_id(), + std::weak_ptr(mapped_font_file))); + return internal::MappedFontFile::CreateMemoryStream( + std::move(mapped_font_file)); } } diff --git a/components/services/font/public/cpp/font_loader.h b/components/services/font/public/cpp/font_loader.h index 827a991841882..74b6174c7413a 100644 --- a/components/services/font/public/cpp/font_loader.h +++ b/components/services/font/public/cpp/font_loader.h @@ -7,6 +7,7 @@ #include +#include #include #include "base/containers/lru_cache.h" @@ -114,8 +115,7 @@ class FontLoader : public SkFontConfigInterface, base::Lock mapped_font_files_lock_; // Maps font identity ID to the memory-mapped file with font data. - std::unordered_map> + std::unordered_map> mapped_font_files_ GUARDED_BY(mapped_font_files_lock_); // Lock preventing multiple threads from modifying the |typeface_cache_| diff --git a/components/services/font/public/cpp/font_service_thread.cc b/components/services/font/public/cpp/font_service_thread.cc index 2c2553144ad21..b060823c6c32a 100644 --- a/components/services/font/public/cpp/font_service_thread.cc +++ b/components/services/font/public/cpp/font_service_thread.cc @@ -150,7 +150,7 @@ void FontServiceThread::MatchFontWithFallback( } #endif // BUILDFLAG(ENABLE_PDF) -scoped_refptr FontServiceThread::OpenStream( +std::shared_ptr FontServiceThread::OpenStream( const SkFontConfigInterface::FontIdentity& identity) { DCHECK(!task_runner_->RunsTasksInCurrentSequence()); @@ -173,9 +173,9 @@ scoped_refptr FontServiceThread::OpenStream( return nullptr; } - // Converts the file to out internal type. - scoped_refptr mapped_font_file = - new MappedFontFile(identity.fID); + // Converts the file to our internal type. + std::shared_ptr mapped_font_file = + std::make_shared(identity.fID); if (!mapped_font_file->Initialize(std::move(stream_file))) return nullptr; diff --git a/components/services/font/public/cpp/font_service_thread.h b/components/services/font/public/cpp/font_service_thread.h index b7d926fe6c702..2b7df5f81f5f2 100644 --- a/components/services/font/public/cpp/font_service_thread.h +++ b/components/services/font/public/cpp/font_service_thread.h @@ -7,6 +7,7 @@ #include +#include #include #include @@ -55,7 +56,7 @@ class FontServiceThread : public base::RefCountedThreadSafe { SkFontConfigInterface::FontIdentity* out_font_identity, SkString* out_family_name, SkFontStyle* out_style); - scoped_refptr OpenStream( + std::shared_ptr OpenStream( const SkFontConfigInterface::FontIdentity& identity); bool FallbackFontForCharacter( diff --git a/components/services/font/public/cpp/mapped_font_file.cc b/components/services/font/public/cpp/mapped_font_file.cc index b75aaf7907c21..301dd2cbd7b4f 100644 --- a/components/services/font/public/cpp/mapped_font_file.cc +++ b/components/services/font/public/cpp/mapped_font_file.cc @@ -23,14 +23,20 @@ bool MappedFontFile::Initialize(base::File file) { return mapped_font_file_.Initialize(std::move(file)); } -SkMemoryStream* MappedFontFile::CreateMemoryStream() { - DCHECK(mapped_font_file_.IsValid()); - sk_sp data = - SkData::MakeWithProc(mapped_font_file_.data(), mapped_font_file_.length(), - &MappedFontFile::ReleaseProc, this); - if (!data) +SkMemoryStream* MappedFontFile::CreateMemoryStream( + std::shared_ptr mff) { + DCHECK(mff->mapped_font_file_.IsValid()); + std::unique_ptr> umff = + std::make_unique>(std::move(mff)); + sk_sp data = SkData::MakeWithProc( + (*umff)->mapped_font_file_.data(), (*umff)->mapped_font_file_.length(), + &MappedFontFile::ReleaseProc, umff.get()); + if (data) { + // Ownership successfully passed + umff.release(); + } else { return nullptr; - AddRef(); + } return new SkMemoryStream(std::move(data)); } @@ -42,7 +48,7 @@ MappedFontFile::~MappedFontFile() { // static void MappedFontFile::ReleaseProc(const void* ptr, void* context) { base::ScopedAllowBlocking allow_munmap; - static_cast(context)->Release(); + delete static_cast*>(context); } } // namespace internal diff --git a/components/services/font/public/cpp/mapped_font_file.h b/components/services/font/public/cpp/mapped_font_file.h index 160abed4882fc..a50dba0dec51b 100644 --- a/components/services/font/public/cpp/mapped_font_file.h +++ b/components/services/font/public/cpp/mapped_font_file.h @@ -19,7 +19,7 @@ namespace internal { // Owns the memory of the mmaped file that we get back from the font_service. // // This class is an implementation detail and shouldn't be used by consumers. -class MappedFontFile : public base::RefCountedThreadSafe { +class MappedFontFile { public: class Observer { public: @@ -30,6 +30,7 @@ class MappedFontFile : public base::RefCountedThreadSafe { }; explicit MappedFontFile(uint32_t font_id); + ~MappedFontFile(); uint32_t font_id() const { return font_id_; } @@ -37,12 +38,9 @@ class MappedFontFile : public base::RefCountedThreadSafe { bool Initialize(base::File file); - SkMemoryStream* CreateMemoryStream(); + static SkMemoryStream* CreateMemoryStream(std::shared_ptr); private: - friend class base::RefCountedThreadSafe; - - ~MappedFontFile(); static void ReleaseProc(const void* ptr, void* context); -- 2.53.0.851.ga537e3e6e9-goog