Make CPDF_Page retainable.

Small step to reducing the differences between XFA and non-XFA. We
still use the RetainPtr pretty much as if it were an unique_ptr, in
that we're not yet caching pages and handing out multiple pointers
to the same page in the non-XFA case.

The one change is in page view cleanup, where we no longer need a
boolean and can take (sufficient) page ownership with a RetainPtr.

Tidy up some document.h -> page.h -> document.h circular inclusion
while we're at it.

NOTE: Wait for imminent branch to pass before landing. We'll want
this to bake a while.

Change-Id: I64a2f12ac3424ece1063d40583995b834117cf34
Reviewed-on: https://pdfium-review.googlesource.com/32790
Reviewed-by: dsinclair <dsinclair@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>

12 files changed, 65 insertions, 88 deletions

diff --git a/core/fpdfapi/edit/cpdf_pagecontentgenerator_unittest.cpp b/core/fpdfapi/edit/cpdf_pagecontentgenerator_unittest.cpp
index d97e8abf0b..1244b12331 100644
--- a/core/fpdfapi/edit/cpdf_pagecontentgenerator_unittest.cpp
+++ b/core/fpdfapi/edit/cpdf_pagecontentgenerator_unittest.cpp
@@ -54,8 +54,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessRect) {
   pPathObj->m_FillType = FXFILL_ALTERNATE;
   pPathObj->m_bStroke = true;
 
-  auto pTestPage = pdfium::MakeUnique<CPDF_Page>(nullptr, nullptr, false);
-  CPDF_PageContentGenerator generator(pTestPage.get());
+  auto pTestPage = pdfium::MakeRetain<CPDF_Page>(nullptr, nullptr, false);
+  CPDF_PageContentGenerator generator(pTestPage.Get());
   std::ostringstream buf;
   TestProcessPath(&generator, &buf, pPathObj.get());
   EXPECT_EQ("q 1 0 0 1 0 0 cm 10 5 3 25 re B* Q\n", ByteString(buf));
@@ -98,8 +98,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessPath) {
   pPathObj->m_FillType = FXFILL_WINDING;
   pPathObj->m_bStroke = false;
 
-  auto pTestPage = pdfium::MakeUnique<CPDF_Page>(nullptr, nullptr, false);
-  CPDF_PageContentGenerator generator(pTestPage.get());
+  auto pTestPage = pdfium::MakeRetain<CPDF_Page>(nullptr, nullptr, false);
+  CPDF_PageContentGenerator generator(pTestPage.Get());
   std::ostringstream buf;
   TestProcessPath(&generator, &buf, pPathObj.get());
   EXPECT_EQ(
@@ -129,8 +129,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessGraphics) {
   auto pDoc = pdfium::MakeUnique<CPDF_Document>(nullptr);
   pDoc->CreateNewDoc();
   CPDF_Dictionary* pPageDict = pDoc->CreateNewPage(0);
-  auto pTestPage = pdfium::MakeUnique<CPDF_Page>(pDoc.get(), pPageDict, false);
-  CPDF_PageContentGenerator generator(pTestPage.get());
+  auto pTestPage = pdfium::MakeRetain<CPDF_Page>(pDoc.get(), pPageDict, false);
+  CPDF_PageContentGenerator generator(pTestPage.Get());
   std::ostringstream buf;
   TestProcessPath(&generator, &buf, pPathObj.get());
   ByteString pathString(buf);
@@ -168,8 +168,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessStandardText) {
   auto pDoc = pdfium::MakeUnique<CPDF_Document>(nullptr);
   pDoc->CreateNewDoc();
   CPDF_Dictionary* pPageDict = pDoc->CreateNewPage(0);
-  auto pTestPage = pdfium::MakeUnique<CPDF_Page>(pDoc.get(), pPageDict, false);
-  CPDF_PageContentGenerator generator(pTestPage.get());
+  auto pTestPage = pdfium::MakeRetain<CPDF_Page>(pDoc.get(), pPageDict, false);
+  CPDF_PageContentGenerator generator(pTestPage.Get());
   auto pTextObj = pdfium::MakeUnique<CPDF_TextObject>();
   CPDF_Font* pFont = CPDF_Font::GetStockFont(pDoc.get(), "Times-Roman");
   pTextObj->m_TextState.SetFont(pFont);
@@ -231,8 +231,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessText) {
   auto pDoc = pdfium::MakeUnique<CPDF_Document>(nullptr);
   pDoc->CreateNewDoc();
   CPDF_Dictionary* pPageDict = pDoc->CreateNewPage(0);
-  auto pTestPage = pdfium::MakeUnique<CPDF_Page>(pDoc.get(), pPageDict, false);
-  CPDF_PageContentGenerator generator(pTestPage.get());
+  auto pTestPage = pdfium::MakeRetain<CPDF_Page>(pDoc.get(), pPageDict, false);
+  CPDF_PageContentGenerator generator(pTestPage.Get());
 
   std::ostringstream buf;
   {
diff --git a/core/fpdfapi/page/cpdf_page.h b/core/fpdfapi/page/cpdf_page.h
index 28a12f45b5..0bb99b73f1 100644
--- a/core/fpdfapi/page/cpdf_page.h
+++ b/core/fpdfapi/page/cpdf_page.h
@@ -10,7 +10,6 @@
 #include <memory>
 
 #include "core/fpdfapi/page/cpdf_pageobjectholder.h"
-#include "core/fpdfapi/parser/cpdf_document.h"
 #include "core/fxcrt/fx_coordinates.h"
 #include "core/fxcrt/fx_system.h"
 #include "core/fxcrt/retain_ptr.h"
@@ -23,19 +22,13 @@ class CPDF_Object;
 class CPDF_PageRenderCache;
 class CPDF_PageRenderContext;
 
-class CPDF_Page : public CPDF_PageObjectHolder {
+class CPDF_Page : public Retainable, public CPDF_PageObjectHolder {
  public:
   class View {};  // Caller implements as desired, empty here due to layering.
+  class Extension : public Retainable {};  // XFA page parent class, layering.
 
-  // XFA page parent class, layering.
-  class Extension : public Retainable {
-    virtual CPDF_Document::Extension* GetDocumentExtension() const = 0;
-  };
-
-  CPDF_Page(CPDF_Document* pDocument,
-            CPDF_Dictionary* pPageDict,
-            bool bPageCache);
-  ~CPDF_Page() override;
+  template <typename T, typename... Args>
+  friend RetainPtr<T> pdfium::MakeRetain(Args&&... args);
 
   // CPDF_PageObjectHolder:
   bool IsPage() const override;
@@ -71,6 +64,11 @@ class CPDF_Page : public CPDF_PageObjectHolder {
   void SetPageExtension(Extension* pExt) { m_pPageExtension = pExt; }
 
  private:
+  CPDF_Page(CPDF_Document* pDocument,
+            CPDF_Dictionary* pPageDict,
+            bool bPageCache);
+  ~CPDF_Page() override;
+
   void StartParse();
 
   CPDF_Object* GetPageAttr(const ByteString& name) const;
diff --git a/core/fpdfapi/parser/cpdf_document.h b/core/fpdfapi/parser/cpdf_document.h
index 356d341302..013eded765 100644
--- a/core/fpdfapi/parser/cpdf_document.h
+++ b/core/fpdfapi/parser/cpdf_document.h
@@ -14,6 +14,7 @@
 #include <vector>
 
 #include "core/fpdfapi/page/cpdf_image.h"
+#include "core/fpdfapi/page/cpdf_page.h"
 #include "core/fpdfapi/parser/cpdf_indirect_object_holder.h"
 #include "core/fpdfapi/parser/cpdf_object.h"
 #include "core/fpdfdoc/cpdf_linklist.h"
@@ -165,7 +166,7 @@ class CPDF_Document : public CPDF_IndirectObjectHolder {
   std::unique_ptr<CPDF_DocRenderData> m_pDocRender;
   std::unique_ptr<JBig2_DocumentContext> m_pCodecContext;
   std::unique_ptr<CPDF_LinkList> m_pLinksContext;
-  std::vector<uint32_t> m_PageList;
+  std::vector<uint32_t> m_PageList;  // Page number to page's dict objnum.
   UnownedPtr<Extension> m_pExtension;
 };
 
diff --git a/fpdfsdk/cpdfsdk_pageview.cpp b/fpdfsdk/cpdfsdk_pageview.cpp
index 2eab6ca32b..c86adf323d 100644
--- a/fpdfsdk/cpdfsdk_pageview.cpp
+++ b/fpdfsdk/cpdfsdk_pageview.cpp
@@ -32,15 +32,7 @@
 
 CPDFSDK_PageView::CPDFSDK_PageView(CPDFSDK_FormFillEnvironment* pFormFillEnv,
                                    UnderlyingPageType* page)
-    : m_page(page),
-      m_pFormFillEnv(pFormFillEnv),
-#ifndef PDF_ENABLE_XFA
-      m_bOwnsPage(false),
-#endif  // PDF_ENABLE_XFA
-      m_bOnWidget(false),
-      m_bValid(false),
-      m_bLocked(false),
-      m_bBeingDestroyed(false) {
+    : m_page(page), m_pFormFillEnv(pFormFillEnv) {
   CPDFSDK_InterForm* pInterForm = pFormFillEnv->GetInterForm();
   CPDF_InterForm* pPDFInterForm = pInterForm->GetInterForm();
 #ifdef PDF_ENABLE_XFA
@@ -67,11 +59,6 @@ CPDFSDK_PageView::~CPDFSDK_PageView() {
 
   m_SDKAnnotArray.clear();
   m_pAnnotList.reset();
-
-#ifndef PDF_ENABLE_XFA
-  if (m_bOwnsPage)
-    delete m_page;
-#endif  // PDF_ENABLE_XFA
 }
 
 void CPDFSDK_PageView::PageView_OnDraw(CFX_RenderDevice* pDevice,
diff --git a/fpdfsdk/cpdfsdk_pageview.h b/fpdfsdk/cpdfsdk_pageview.h
index e85b32aa78..afb200e43b 100644
--- a/fpdfsdk/cpdfsdk_pageview.h
+++ b/fpdfsdk/cpdfsdk_pageview.h
@@ -102,8 +102,7 @@ class CPDFSDK_PageView final : public CPDF_Page::View {
   bool IsBeingDestroyed() const { return m_bBeingDestroyed; }
 
 #ifndef PDF_ENABLE_XFA
-  bool OwnsPage() const { return m_bOwnsPage; }
-  void TakePageOwnership() { m_bOwnsPage = true; }
+  void TakePageOwnership() { m_pOwnsPage.Reset(m_page); }
 #endif  // PDF_ENABLE_XFA
 
  private:
@@ -126,12 +125,12 @@ class CPDFSDK_PageView final : public CPDF_Page::View {
   UnownedPtr<CPDFSDK_FormFillEnvironment> const m_pFormFillEnv;
   CPDFSDK_Annot::ObservedPtr m_pCaptureWidget;
 #ifndef PDF_ENABLE_XFA
-  bool m_bOwnsPage;
+  RetainPtr<CPDF_Page> m_pOwnsPage;
 #endif  // PDF_ENABLE_XFA
-  bool m_bOnWidget;
-  bool m_bValid;
-  bool m_bLocked;
-  bool m_bBeingDestroyed;
+  bool m_bOnWidget = false;
+  bool m_bValid = false;
+  bool m_bLocked = false;
+  bool m_bBeingDestroyed = false;
 };
 
 #endif  // FPDFSDK_CPDFSDK_PAGEVIEW_H_
diff --git a/fpdfsdk/fpdf_editpage.cpp b/fpdfsdk/fpdf_editpage.cpp
index ea4659f54a..39cf85fb95 100644
--- a/fpdfsdk/fpdf_editpage.cpp
+++ b/fpdfsdk/fpdf_editpage.cpp
@@ -196,9 +196,9 @@ FPDF_EXPORT FPDF_PAGE FPDF_CALLCONV FPDFPage_New(FPDF_DOCUMENT document,
   // Eventually, fallthru into non-XFA case once page type is consistent.
   return nullptr;
 #else  // PDF_ENABLE_XFA
-  auto pPage = pdfium::MakeUnique<CPDF_Page>(pDoc, pPageDict, true);
+  auto pPage = pdfium::MakeRetain<CPDF_Page>(pDoc, pPageDict, true);
   pPage->ParseContent();
-  return FPDFPageFromUnderlying(pPage.release());  // Caller takes ownership.
+  return FPDFPageFromUnderlying(pPage.Leak());  // Caller takes ownership.
 #endif  // PDF_ENABLE_XFA
 }
 
diff --git a/fpdfsdk/fpdf_flatten.cpp b/fpdfsdk/fpdf_flatten.cpp
index 03a4f60b5c..720fa9ff55 100644
--- a/fpdfsdk/fpdf_flatten.cpp
+++ b/fpdfsdk/fpdf_flatten.cpp
@@ -47,7 +47,7 @@ bool IsValidRect(const CFX_FloatRect& rect, const CFX_FloatRect& rcPage) {
 void GetContentsRect(CPDF_Document* pDoc,
                      CPDF_Dictionary* pDict,
                      std::vector<CFX_FloatRect>* pRectArray) {
-  auto pPDFPage = pdfium::MakeUnique<CPDF_Page>(pDoc, pDict, false);
+  auto pPDFPage = pdfium::MakeRetain<CPDF_Page>(pDoc, pDict, false);
   pPDFPage->ParseContent();
 
   for (const auto& pPageObject : *pPDFPage->GetPageObjectList()) {
diff --git a/fpdfsdk/fpdf_ppo.cpp b/fpdfsdk/fpdf_ppo.cpp
index 1790e17fb3..b73042466b 100644
--- a/fpdfsdk/fpdf_ppo.cpp
+++ b/fpdfsdk/fpdf_ppo.cpp
@@ -618,9 +618,9 @@ bool CPDF_NPageToOneExporter::ExportNPagesToOne(
       if (!pSrcPageDict)
         return false;
 
-      CPDF_Page srcPage(src(), pSrcPageDict, true);
+      auto srcPage = pdfium::MakeRetain<CPDF_Page>(src(), pSrcPageDict, true);
       NupPageSettings settings =
-          nupState.CalculateNewPagePosition(srcPage.GetPageSize());
+          nupState.CalculateNewPagePosition(srcPage->GetPageSize());
       AddSubPage(pSrcPageDict, settings, &objectNumberMap, &pageXObjectMap,
                  &xObjNameNumberMap, &bsContent);
     }
diff --git a/fpdfsdk/fpdf_view.cpp b/fpdfsdk/fpdf_view.cpp
index 6c4ad6020e..593a092fd3 100644
--- a/fpdfsdk/fpdf_view.cpp
+++ b/fpdfsdk/fpdf_view.cpp
@@ -355,9 +355,9 @@ FPDF_EXPORT FPDF_PAGE FPDF_CALLCONV FPDF_LoadPage(FPDF_DOCUMENT document,
   if (!pDict)
     return nullptr;
 
-  CPDF_Page* pPage = new CPDF_Page(pDoc, pDict, true);
+  auto pPage = pdfium::MakeRetain<CPDF_Page>(pDoc, pDict, true);
   pPage->ParseContent();
-  return FPDFPageFromUnderlying(pPage);
+  return FPDFPageFromUnderlying(pPage.Leak());
 #endif  // PDF_ENABLE_XFA
 }
 
@@ -723,35 +723,28 @@ FPDF_EXPORT FPDF_RECORDER FPDF_CALLCONV FPDF_RenderPageSkp(FPDF_PAGE page,
 #endif
 
 FPDF_EXPORT void FPDF_CALLCONV FPDF_ClosePage(FPDF_PAGE page) {
-  UnderlyingPageType* pPage = UnderlyingFromFPDFPage(page);
   if (!page)
     return;
-#ifdef PDF_ENABLE_XFA
-  // Take it back across the API and throw it away.
-  RetainPtr<CPDFXFA_Page>().Unleak(pPage);
-#else   // PDF_ENABLE_XFA
+
+  // Take it back across the API and hold for duration of this function.
+  RetainPtr<UnderlyingPageType> pPage;
+  pPage.Unleak(UnderlyingFromFPDFPage(page));
+
+#ifndef PDF_ENABLE_XFA
   CPDFSDK_PageView* pPageView =
       static_cast<CPDFSDK_PageView*>(pPage->GetView());
-  if (pPageView) {
-    // We're already destroying the pageview, so bail early.
-    if (pPageView->IsBeingDestroyed())
-      return;
-
-    if (pPageView->IsLocked()) {
-      pPageView->TakePageOwnership();
-      return;
-    }
+  if (!pPageView || pPageView->IsBeingDestroyed())
+    return;
 
-    bool owned = pPageView->OwnsPage();
-    // This will delete the |pPageView| object. We must cleanup the PageView
-    // first because it will attempt to reset the View on the |pPage| during
-    // destruction.
-    pPageView->GetFormFillEnv()->RemovePageView(pPage);
-    // If the page was owned then the pageview will have deleted the page.
-    if (owned)
-      return;
+  if (pPageView->IsLocked()) {
+    pPageView->TakePageOwnership();
+    return;
   }
-  delete pPage;
+
+  // This will delete the |pPageView| object. We must cleanup the PageView
+  // first because it will attempt to reset the View on the |pPage| during
+  // destruction.
+  pPageView->GetFormFillEnv()->RemovePageView(pPage.Get());
 #endif  // PDF_ENABLE_XFA
 }
 
@@ -970,9 +963,9 @@ FPDF_EXPORT int FPDF_CALLCONV FPDF_GetPageSizeByIndex(FPDF_DOCUMENT document,
   if (!pDict)
     return false;
 
-  CPDF_Page page(pDoc, pDict, true);
-  *width = page.GetPageWidth();
-  *height = page.GetPageHeight();
+  auto page = pdfium::MakeRetain<CPDF_Page>(pDoc, pDict, true);
+  *width = page->GetPageWidth();
+  *height = page->GetPageHeight();
   return true;
 }
 
diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_page.cpp b/fpdfsdk/fpdfxfa/cpdfxfa_page.cpp
index 170f17ea7a..4d7e3bc222 100644
--- a/fpdfsdk/fpdfxfa/cpdfxfa_page.cpp
+++ b/fpdfsdk/fpdfxfa/cpdfxfa_page.cpp
@@ -35,7 +35,7 @@ bool CPDFXFA_Page::LoadPDFPage() {
     return false;
 
   if (!m_pPDFPage || m_pPDFPage->GetFormDict() != pDict) {
-    m_pPDFPage = pdfium::MakeUnique<CPDF_Page>(pPDFDoc, pDict, true);
+    m_pPDFPage = pdfium::MakeRetain<CPDF_Page>(pPDFDoc, pDict, true);
     m_pPDFPage->ParseContent();
   }
   return true;
@@ -81,7 +81,7 @@ bool CPDFXFA_Page::LoadPDFPage(CPDF_Dictionary* pageDict) {
     return false;
 
   m_pPDFPage =
-      pdfium::MakeUnique<CPDF_Page>(m_pContext->GetPDFDoc(), pageDict, true);
+      pdfium::MakeRetain<CPDF_Page>(m_pContext->GetPDFDoc(), pageDict, true);
   m_pPDFPage->ParseContent();
   return true;
 }
diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_page.h b/fpdfsdk/fpdfxfa/cpdfxfa_page.h
index 4f4d6b0a22..2573b9cebd 100644
--- a/fpdfsdk/fpdfxfa/cpdfxfa_page.h
+++ b/fpdfsdk/fpdfxfa/cpdfxfa_page.h
@@ -29,11 +29,10 @@ class CPDFXFA_Page : public CPDF_Page::Extension {
   bool LoadPage();
   bool LoadPDFPage(CPDF_Dictionary* pageDict);
 
-  // CPDF_Page::Extension:
-  CPDF_Document::Extension* GetDocumentExtension() const override;
+  CPDF_Document::Extension* GetDocumentExtension() const;
 
   int GetPageIndex() const { return m_iPageIndex; }
-  CPDF_Page* GetPDFPage() const { return m_pPDFPage.get(); }
+  CPDF_Page* GetPDFPage() const { return m_pPDFPage.Get(); }
   CXFA_FFPageView* GetXFAPageView() const { return m_pXFAPageView; }
 
   void SetXFAPageView(CXFA_FFPageView* pPageView) {
@@ -61,7 +60,7 @@ class CPDFXFA_Page : public CPDF_Page::Extension {
   bool LoadXFAPageView();
 
  private:
-  std::unique_ptr<CPDF_Page> m_pPDFPage;
+  RetainPtr<CPDF_Page> m_pPDFPage;
   CXFA_FFPageView* m_pXFAPageView;
   UnownedPtr<CPDFXFA_Context> const m_pContext;
   const int m_iPageIndex;
diff --git a/fxjs/cjs_document.cpp b/fxjs/cjs_document.cpp
index b90794a88d..66577062a6 100644
--- a/fxjs/cjs_document.cpp
+++ b/fxjs/cjs_document.cpp
@@ -1256,12 +1256,12 @@ CJS_Return CJS_Document::getPageNthWord(
   if (!pPageDict)
     return CJS_Return(false);
 
-  CPDF_Page page(pDocument, pPageDict, true);
-  page.ParseContent();
+  auto page = pdfium::MakeRetain<CPDF_Page>(pDocument, pPageDict, true);
+  page->ParseContent();
 
   int nWords = 0;
   WideString swRet;
-  for (auto& pPageObj : *page.GetPageObjectList()) {
+  for (auto& pPageObj : *page->GetPageObjectList()) {
     if (pPageObj->IsText()) {
       CPDF_TextObject* pTextObj = pPageObj->AsText();
       int nObjWords = CountWords(pTextObj);
@@ -1305,11 +1305,11 @@ CJS_Return CJS_Document::getPageNumWords(
   if (!pPageDict)
     return CJS_Return(false);
 
-  CPDF_Page page(pDocument, pPageDict, true);
-  page.ParseContent();
+  auto page = pdfium::MakeRetain<CPDF_Page>(pDocument, pPageDict, true);
+  page->ParseContent();
 
   int nWords = 0;
-  for (auto& pPageObj : *page.GetPageObjectList()) {
+  for (auto& pPageObj : *page->GetPageObjectList()) {
     if (pPageObj->IsText())
       nWords += CountWords(pPageObj->AsText());
   }