commit 4e60bb4bde2c854b23115f42b093d394628b0c7e
author Lei Zhang <thestig@chromium.org> Tue Jun 11 08:13:00 2024 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Jun 11 08:13:00 2024 +0000
tree d25fe0e60c9b338f31745b084097f001a18ff271
parent 33ece3a423882159e695f021c7c722f8b3d16dfa

Avoid addition calls to free resources during CPDF_Document destruction

First, mark some CPDF_Document members as const. Since the members that
are now const are accessed in the ctor, they appaear to be guaranteed to
be non-null. Thus conditionals that check if they are null or not should
be removable.

Actually removing those checks shows this circular call pattern:
~CPDF_Document -> ~CPDF_DocPageData -> ~CPDF_Font ->
CPDF_Document::MaybePurgeFontFileStreamAcc(). At which point, the
"always" non-null CPDF_Document member is null. This issue started
happening due to CL [1] making the CPDF_DocPageData more effective at
holding on to CPDF_Font objects.

To stop CPDF_Font's dtor from trying to CPDF_DocPageData to destroy
CPDF_Font, use the existing CPDF_Font::WillBeDestroyed() to remember
whether CPDF_DocPageData told CPDF_Font about the impending destruction.

[1] https://pdfium-review.googlesource.com/119870

Change-Id: I85515e4e9cae4f4d58f5dea8c32bef714e36ec4c
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/120231
Reviewed-by: Thomas Sepez <tsepez@google.com>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>

core/fpdfapi/font/cpdf_font.cpp

diff --git a/core/fpdfapi/font/cpdf_font.cpp b/core/fpdfapi/font/cpdf_font.cpp
index bba8ff4..96a954b 100644
--- a/core/fpdfapi/font/cpdf_font.cpp
+++ b/core/fpdfapi/font/cpdf_font.cpp
@@ -54,8 +54,9 @@
       m_BaseFontName(m_pFontDict->GetByteStringFor("BaseFont")) {}
 
 CPDF_Font::~CPDF_Font() {
-  if (m_pFontFile)
+  if (!m_bWillBeDestroyed && m_pFontFile) {
     m_pDocument->MaybePurgeFontFileStreamAcc(std::move(m_pFontFile));
+  }
 }
 
 bool CPDF_Font::IsType1Font() const {
@@ -116,7 +117,9 @@
 }
 #endif
 
-void CPDF_Font::WillBeDestroyed() {}
+void CPDF_Font::WillBeDestroyed() {
+  m_bWillBeDestroyed = true;
+}
 
 bool CPDF_Font::IsVertWriting() const {
   const CPDF_CIDFont* pCIDFont = AsCIDFont();

core/fpdfapi/font/cpdf_font.h

diff --git a/core/fpdfapi/font/cpdf_font.h b/core/fpdfapi/font/cpdf_font.h
index edfbc9f..7cf8a07 100644
--- a/core/fpdfapi/font/cpdf_font.h
+++ b/core/fpdfapi/font/cpdf_font.h
@@ -171,6 +171,7 @@
   ByteString m_BaseFontName;
   mutable std::unique_ptr<CPDF_ToUnicodeMap> m_pToUnicodeMap;
   mutable bool m_bToUnicodeLoaded = false;
+  bool m_bWillBeDestroyed = false;
   int m_Flags = 0;
   int m_StemV = 0;
   int m_Ascent = 0;

core/fpdfapi/font/cpdf_type3font.cpp

diff --git a/core/fpdfapi/font/cpdf_type3font.cpp b/core/fpdfapi/font/cpdf_type3font.cpp
index 3cc2639..60d03cf 100644
--- a/core/fpdfapi/font/cpdf_type3font.cpp
+++ b/core/fpdfapi/font/cpdf_type3font.cpp
@@ -48,11 +48,14 @@
 }
 
 void CPDF_Type3Font::WillBeDestroyed() {
+  m_bWillBeDestroyed = true;
+
   // Last reference to |this| may be through one of its CPDF_Type3Chars.
   RetainPtr<CPDF_Font> protector(this);
   for (const auto& item : m_CacheMap) {
-    if (item.second)
+    if (item.second) {
       item.second->WillBeDestroyed();
+    }
   }
 }
 

core/fpdfapi/parser/cpdf_document.cpp

diff --git a/core/fpdfapi/parser/cpdf_document.cpp b/core/fpdfapi/parser/cpdf_document.cpp
index 3012690..4e2210b 100644
--- a/core/fpdfapi/parser/cpdf_document.cpp
+++ b/core/fpdfapi/parser/cpdf_document.cpp
@@ -451,13 +451,11 @@
 
 void CPDF_Document::MaybePurgeFontFileStreamAcc(
     RetainPtr<CPDF_StreamAcc>&& pStreamAcc) {
-  if (m_pDocPage)
-    m_pDocPage->MaybePurgeFontFileStreamAcc(std::move(pStreamAcc));
+  m_pDocPage->MaybePurgeFontFileStreamAcc(std::move(pStreamAcc));
 }
 
 void CPDF_Document::MaybePurgeImage(uint32_t objnum) {
-  if (m_pDocPage)
-    m_pDocPage->MaybePurgeImage(objnum);
+  m_pDocPage->MaybePurgeImage(objnum);
 }
 
 void CPDF_Document::CreateNewDoc() {

core/fpdfapi/parser/cpdf_document.h

diff --git a/core/fpdfapi/parser/cpdf_document.h b/core/fpdfapi/parser/cpdf_document.h
index ac162a6..a9a2922 100644
--- a/core/fpdfapi/parser/cpdf_document.h
+++ b/core/fpdfapi/parser/cpdf_document.h
@@ -219,8 +219,9 @@
   int m_iNextPageToTraverse = 0;
   uint32_t m_ParsedPageCount = 0;
 
-  std::unique_ptr<RenderDataIface> m_pDocRender;
-  std::unique_ptr<PageDataIface> m_pDocPage;  // Must be after |m_pDocRender|.
+  std::unique_ptr<RenderDataIface> const m_pDocRender;
+  // Must be after `m_pDocRender`.
+  std::unique_ptr<PageDataIface> const m_pDocPage;
   std::unique_ptr<JBig2_DocumentContext> m_pCodecContext;
   std::unique_ptr<LinkListIface> m_pLinksContext;
   std::set<uint32_t> m_ModifiedAPStreamIDs;