From 1df1efa3921841fb5fc7fc15e8112eed4375de9f Mon Sep 17 00:00:00 2001
From: dsinclair <dsinclair@chromium.org>
Date: Wed, 7 Sep 2016 09:55:37 -0700
Subject: Fixup CPDFSDK_PageView and CPDF_Page interactions.

There are several issues when CPDFSDK_PageView and CPDF_Page interact,
especially around deletion. This Cl fixes up several places where things
go wrong working with these objects.

BUG=chromium:632709

Review-Url: https://codereview.chromium.org/2319663002
---
 fpdfsdk/fpdfview.cpp         | 17 ++++++++++++++---
 fpdfsdk/fsdk_mgr.cpp         | 16 +++++++++++-----
 fpdfsdk/include/fsdk_mgr.h   |  5 +++--
 fpdfsdk/javascript/Field.cpp | 11 +++++++++--
 4 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/fpdfsdk/fpdfview.cpp b/fpdfsdk/fpdfview.cpp
index dee71ac9e2..1c0af1dc10 100644
--- a/fpdfsdk/fpdfview.cpp
+++ b/fpdfsdk/fpdfview.cpp
@@ -673,9 +673,20 @@ DLLEXPORT void STDCALL FPDF_ClosePage(FPDF_PAGE page) {
 #else   // PDF_ENABLE_XFA
   CPDFSDK_PageView* pPageView =
       static_cast<CPDFSDK_PageView*>(pPage->GetView());
-  if (pPageView && pPageView->IsLocked()) {
-    pPageView->TakeOverPage();
-    return;
+  if (pPageView) {
+    if (pPageView->IsLocked()) {
+      pPageView->TakePageOwnership();
+      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->GetSDKDocument()->RemovePageView(pPage);
+    // If the page was owned then the pageview will have deleted the page.
+    if (owned)
+      return;
   }
   delete pPage;
 #endif  // PDF_ENABLE_XFA
diff --git a/fpdfsdk/fsdk_mgr.cpp b/fpdfsdk/fsdk_mgr.cpp
index 0fc61dac7a..eb3f51e97c 100644
--- a/fpdfsdk/fsdk_mgr.cpp
+++ b/fpdfsdk/fsdk_mgr.cpp
@@ -299,7 +299,7 @@ CPDFSDK_PageView* CPDFSDK_Document::GetPageView(int nIndex) {
     return nullptr;
 
   auto it = m_pageMap.find(pTempPage);
-  return it->second;
+  return it != m_pageMap.end() ? it->second : nullptr;
 }
 
 void CPDFSDK_Document::ProcJavascriptFun() {
@@ -482,7 +482,7 @@ CPDFSDK_PageView::CPDFSDK_PageView(CPDFSDK_Document* pSDKDoc,
       m_pSDKDoc(pSDKDoc),
       m_CaptureWidget(nullptr),
 #ifndef PDF_ENABLE_XFA
-      m_bTakeOverPage(FALSE),
+      m_bOwnsPage(false),
 #endif  // PDF_ENABLE_XFA
       m_bEnterWidget(FALSE),
       m_bExitWidget(FALSE),
@@ -505,6 +505,13 @@ CPDFSDK_PageView::CPDFSDK_PageView(CPDFSDK_Document* pSDKDoc,
 }
 
 CPDFSDK_PageView::~CPDFSDK_PageView() {
+#ifndef PDF_ENABLE_XFA
+  // The call to |ReleaseAnnot| can cause the page pointed to by |m_page| to
+  // be freed, which will cause issues if we try to cleanup the pageview pointer
+  // in |m_page|. So, reset the pageview pointer before doing anything else.
+  m_page->SetView(nullptr);
+#endif  // PDF_ENABLE_XFA
+
   CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
   CPDFSDK_AnnotHandlerMgr* pAnnotHandlerMgr = pEnv->GetAnnotHandlerMgr();
   for (CPDFSDK_Annot* pAnnot : m_fxAnnotArray)
@@ -512,11 +519,10 @@ CPDFSDK_PageView::~CPDFSDK_PageView() {
 
   m_fxAnnotArray.clear();
   m_pAnnotList.reset();
+
 #ifndef PDF_ENABLE_XFA
-  m_page->SetView(nullptr);
-  if (m_bTakeOverPage) {
+  if (m_bOwnsPage)
     delete m_page;
-  }
 #endif  // PDF_ENABLE_XFA
 }
 
diff --git a/fpdfsdk/include/fsdk_mgr.h b/fpdfsdk/include/fsdk_mgr.h
index 924cd444e0..f37e64fe41 100644
--- a/fpdfsdk/include/fsdk_mgr.h
+++ b/fpdfsdk/include/fsdk_mgr.h
@@ -605,7 +605,8 @@ class CPDFSDK_PageView final : public CPDF_Page::View {
   void SetLock(FX_BOOL bLocked) { m_bLocked = bLocked; }
   FX_BOOL IsLocked() { return m_bLocked; }
 #ifndef PDF_ENABLE_XFA
-  void TakeOverPage() { m_bTakeOverPage = TRUE; }
+  bool OwnsPage() const { return m_bOwnsPage; }
+  void TakePageOwnership() { m_bOwnsPage = true; }
 #endif  // PDF_ENABLE_XFA
 
  private:
@@ -618,7 +619,7 @@ class CPDFSDK_PageView final : public CPDF_Page::View {
   CPDFSDK_Document* const m_pSDKDoc;
   CPDFSDK_Annot* m_CaptureWidget;
 #ifndef PDF_ENABLE_XFA
-  FX_BOOL m_bTakeOverPage;
+  bool m_bOwnsPage;
 #endif  // PDF_ENABLE_XFA
   FX_BOOL m_bEnterWidget;
   FX_BOOL m_bExitWidget;
diff --git a/fpdfsdk/javascript/Field.cpp b/fpdfsdk/javascript/Field.cpp
index 3f5e5e1b0f..64c7735dcf 100644
--- a/fpdfsdk/javascript/Field.cpp
+++ b/fpdfsdk/javascript/Field.cpp
@@ -260,11 +260,12 @@ void Field::UpdateFormField(CPDFSDK_Document* pDocument,
                             FX_BOOL bChangeMark,
                             FX_BOOL bResetAP,
                             FX_BOOL bRefresh) {
-  std::vector<CPDFSDK_Widget*> widgets;
   CPDFSDK_InterForm* pInterForm = pDocument->GetInterForm();
-  pInterForm->GetWidgets(pFormField, &widgets);
 
   if (bResetAP) {
+    std::vector<CPDFSDK_Widget*> widgets;
+    pInterForm->GetWidgets(pFormField, &widgets);
+
     int nFieldType = pFormField->GetFieldType();
     if (nFieldType == FIELDTYPE_COMBOBOX || nFieldType == FIELDTYPE_TEXTFIELD) {
       for (CPDFSDK_Annot* pAnnot : widgets) {
@@ -285,6 +286,12 @@ void Field::UpdateFormField(CPDFSDK_Document* pDocument,
   }
 
   if (bRefresh) {
+    // Refresh the widget list. The calls in |bResetAP| may have caused widgets
+    // to be removed from the list. We need to call |GetWidgets| again to be
+    // sure none of the widgets have been deleted.
+    std::vector<CPDFSDK_Widget*> widgets;
+    pInterForm->GetWidgets(pFormField, &widgets);
+
     for (CPDFSDK_Widget* pWidget : widgets) {
       CPDFSDK_Document* pDoc = pWidget->GetInterForm()->GetDocument();
       pDoc->UpdateAllViews(nullptr, pWidget);
-- 
cgit v1.2.3