Observe Annot destruction in CPDFSDK_PageView::DeleteAnnot

Run test XFA-Only, since that's where the issue occurs, and the
textual output is different for non-xfa.  Fix a few lifetime issue
in pdfium_test.cc unearthed by this test (doc must outlive pages
now held in formfill info).


Bug: 735912
Change-Id: Icc9e6a967c32ece67d897117896c973bb16a1515
Reviewed-on: https://pdfium-review.googlesource.com/8510
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

5 files changed, 169 insertions, 43 deletions

diff --git a/fpdfsdk/cpdfsdk_pageview.cpp b/fpdfsdk/cpdfsdk_pageview.cpp
index 4780d87780..06761a5806 100644
--- a/fpdfsdk/cpdfsdk_pageview.cpp
+++ b/fpdfsdk/cpdfsdk_pageview.cpp
@@ -175,17 +175,23 @@ CPDFSDK_Annot* CPDFSDK_PageView::AddAnnot(CXFA_FFWidget* pPDFAnnot) {
 bool CPDFSDK_PageView::DeleteAnnot(CPDFSDK_Annot* pAnnot) {
   if (!pAnnot)
     return false;
+
   CPDFXFA_Page* pPage = pAnnot->GetPDFXFAPage();
   if (!pPage || (pPage->GetContext()->GetDocType() != XFA_DocType::Static &&
                  pPage->GetContext()->GetDocType() != XFA_DocType::Dynamic)) {
     return false;
   }
 
+  CPDFSDK_Annot::ObservedPtr pObserved(pAnnot);
   if (GetFocusAnnot() == pAnnot)
-    m_pFormFillEnv->KillFocusAnnot(0);
-  CPDFSDK_AnnotHandlerMgr* pAnnotHandler = m_pFormFillEnv->GetAnnotHandlerMgr();
-  if (pAnnotHandler)
-    pAnnotHandler->ReleaseAnnot(pAnnot);
+    m_pFormFillEnv->KillFocusAnnot(0);  // May invoke JS, invalidating pAnnot.
+
+  if (pObserved) {
+    CPDFSDK_AnnotHandlerMgr* pAnnotHandler =
+        m_pFormFillEnv->GetAnnotHandlerMgr();
+    if (pAnnotHandler)
+      pAnnotHandler->ReleaseAnnot(pObserved.Get());
+  }
 
   auto it = std::find(m_SDKAnnotArray.begin(), m_SDKAnnotArray.end(), pAnnot);
   if (it != m_SDKAnnotArray.end())
diff --git a/samples/pdfium_test.cc b/samples/pdfium_test.cc
index dab734e530..bb969b1c8d 100644
--- a/samples/pdfium_test.cc
+++ b/samples/pdfium_test.cc
@@ -10,6 +10,7 @@
 #include <bitset>
 #include <iterator>
 #include <map>
+#include <memory>
 #include <sstream>
 #include <string>
 #include <utility>
@@ -118,7 +119,7 @@ struct Options {
 struct FPDF_FORMFILLINFO_PDFiumTest : public FPDF_FORMFILLINFO {
   // Hold a map of the currently loaded pages in order to avoid them
   // to get loaded twice.
-  std::map<int, FPDF_PAGE> loaded_pages;
+  std::map<int, std::unique_ptr<void, FPDFPageDeleter>> loaded_pages;
 
   // Hold a pointer of FPDF_FORMHANDLE so that PDFium app hooks can
   // make use of it.
@@ -966,7 +967,7 @@ FPDF_PAGE GetPageForIndex(FPDF_FORMFILLINFO* param,
   auto& loaded_pages = form_fill_info->loaded_pages;
   auto iter = loaded_pages.find(index);
   if (iter != loaded_pages.end())
-    return iter->second;
+    return iter->second.get();
 
   FPDF_PAGE page = FPDF_LoadPage(doc, index);
   if (!page)
@@ -975,7 +976,7 @@ FPDF_PAGE GetPageForIndex(FPDF_FORMFILLINFO* param,
   FPDF_FORMHANDLE& form_handle = form_fill_info->form_handle;
   FORM_OnAfterLoadPage(page, form_handle);
   FORM_DoPageAAction(page, form_handle, FPDFPAGE_AACTION_OPEN);
-  loaded_pages[index] = page;
+  loaded_pages[index].reset(page);
   return page;
 }
 
@@ -1130,27 +1131,25 @@ bool RenderPage(const std::string& name,
                 const int page_index,
                 const Options& options,
                 const std::string& events) {
-  std::unique_ptr<void, FPDFPageDeleter> page(
-      GetPageForIndex(form_fill_info, doc, page_index));
-  if (!page.get())
+  FPDF_PAGE page = GetPageForIndex(form_fill_info, doc, page_index);
+  if (!page)
     return false;
   if (options.send_events)
-    SendPageEvents(form, page.get(), events);
+    SendPageEvents(form, page, events);
   if (options.output_format == OUTPUT_STRUCTURE) {
-    DumpPageStructure(page.get(), page_index);
+    DumpPageStructure(page, page_index);
     return true;
   }
 
-  std::unique_ptr<void, FPDFTextPageDeleter> text_page(
-      FPDFText_LoadPage(page.get()));
+  std::unique_ptr<void, FPDFTextPageDeleter> text_page(FPDFText_LoadPage(page));
 
   double scale = 1.0;
   if (!options.scale_factor_as_string.empty())
     std::stringstream(options.scale_factor_as_string) >> scale;
 
-  int width = static_cast<int>(FPDF_GetPageWidth(page.get()) * scale);
-  int height = static_cast<int>(FPDF_GetPageHeight(page.get()) * scale);
-  int alpha = FPDFPage_HasTransparency(page.get()) ? 1 : 0;
+  int width = static_cast<int>(FPDF_GetPageWidth(page) * scale);
+  int height = static_cast<int>(FPDF_GetPageHeight(page) * scale);
+  int alpha = FPDFPage_HasTransparency(page) ? 1 : 0;
   std::unique_ptr<void, FPDFBitmapDeleter> bitmap(
       FPDFBitmap_Create(width, height, alpha));
 
@@ -1162,24 +1161,23 @@ bool RenderPage(const std::string& name,
       // Note, client programs probably want to use this method instead of the
       // progressive calls. The progressive calls are if you need to pause the
       // rendering to update the UI, the PDF renderer will break when possible.
-      FPDF_RenderPageBitmap(bitmap.get(), page.get(), 0, 0, width, height, 0,
+      FPDF_RenderPageBitmap(bitmap.get(), page, 0, 0, width, height, 0,
                             FPDF_ANNOT);
     } else {
       IFSDK_PAUSE pause;
       pause.version = 1;
       pause.NeedToPauseNow = &NeedToPauseNow;
 
-      int rv = FPDF_RenderPageBitmap_Start(
-          bitmap.get(), page.get(), 0, 0, width, height, 0, FPDF_ANNOT, &pause);
+      int rv = FPDF_RenderPageBitmap_Start(bitmap.get(), page, 0, 0, width,
+                                           height, 0, FPDF_ANNOT, &pause);
       while (rv == FPDF_RENDER_TOBECOUNTINUED)
-        rv = FPDF_RenderPage_Continue(page.get(), &pause);
+        rv = FPDF_RenderPage_Continue(page, &pause);
     }
 
-    FPDF_FFLDraw(form, bitmap.get(), page.get(), 0, 0, width, height, 0,
-                 FPDF_ANNOT);
+    FPDF_FFLDraw(form, bitmap.get(), page, 0, 0, width, height, 0, FPDF_ANNOT);
 
     if (!options.render_oneshot)
-      FPDF_RenderPage_Close(page.get());
+      FPDF_RenderPage_Close(page);
 
     int stride = FPDFBitmap_GetStride(bitmap.get());
     const char* buffer =
@@ -1194,20 +1192,20 @@ bool RenderPage(const std::string& name,
         break;
 
       case OUTPUT_EMF:
-        WriteEmf(page.get(), name.c_str(), page_index);
+        WriteEmf(page, name.c_str(), page_index);
         break;
 
       case OUTPUT_PS2:
       case OUTPUT_PS3:
-        WritePS(page.get(), name.c_str(), page_index);
+        WritePS(page, name.c_str(), page_index);
         break;
 #endif
       case OUTPUT_TEXT:
-        WriteText(page.get(), name.c_str(), page_index);
+        WriteText(page, name.c_str(), page_index);
         break;
 
       case OUTPUT_ANNOT:
-        WriteAnnot(page.get(), name.c_str(), page_index);
+        WriteAnnot(page, name.c_str(), page_index);
         break;
 
       case OUTPUT_PNG:
@@ -1224,9 +1222,8 @@ bool RenderPage(const std::string& name,
       case OUTPUT_SKP: {
         std::unique_ptr<SkPictureRecorder> recorder(
             reinterpret_cast<SkPictureRecorder*>(
-                FPDF_RenderPageSkp(page.get(), width, height)));
-        FPDF_FFLRecord(form, recorder.get(), page.get(), 0, 0, width, height, 0,
-                       0);
+                FPDF_RenderPageSkp(page, width, height)));
+        FPDF_FFLRecord(form, recorder.get(), page, 0, 0, width, height, 0, 0);
         image_file_name = WriteSkp(name.c_str(), page_index, recorder.get());
       } break;
 #endif
@@ -1242,9 +1239,8 @@ bool RenderPage(const std::string& name,
     fprintf(stderr, "Page was too large to be rendered.\n");
   }
 
-  form_fill_info->loaded_pages.erase(page_index);
-  FORM_DoPageAAction(page.get(), form, FPDFPAGE_AACTION_CLOSE);
-  FORM_OnBeforeClosePage(page.get(), form);
+  FORM_DoPageAAction(page, form, FPDFPAGE_AACTION_CLOSE);
+  FORM_OnBeforeClosePage(page, form);
   return !!bitmap;
 }
 
@@ -1260,6 +1256,8 @@ void RenderPdf(const std::string& name,
   platform_callbacks.Doc_gotoPage = ExampleDocGotoPage;
   platform_callbacks.Doc_mail = ExampleDocMail;
 
+  // The document must outlive |form_callbacks.loaded_pages|.
+  std::unique_ptr<void, FPDFDocumentDeleter> doc;
   FPDF_FORMFILLINFO_PDFiumTest form_callbacks = {};
 #ifdef PDF_ENABLE_XFA
   form_callbacks.version = 2;
@@ -1285,7 +1283,6 @@ void RenderPdf(const std::string& name,
 
   int nRet = PDF_DATA_NOTAVAIL;
   bool bIsLinearized = false;
-  std::unique_ptr<void, FPDFDocumentDeleter> doc;
   std::unique_ptr<void, FPDFAvailDeleter> pdf_avail(
       FPDFAvail_Create(&file_avail, &file_access));
 
diff --git a/testing/SUPPRESSIONS b/testing/SUPPRESSIONS
index 8389289589..feb862e3b1 100644
--- a/testing/SUPPRESSIONS
+++ b/testing/SUPPRESSIONS
@@ -286,13 +286,8 @@ zh_file1.pdf mac * *
 zh_function_list.pdf mac * *
 zh_shared_document.pdf mac * *
 
-bug_492.in * nov8 *
-bug_679643.in * * noxfa
-bug_679642.in * * noxfa
-
-#
 # TODO(hnakashima): These might never have been run. Go over them and fix.
-#
+
 FRC_8.5_E&X.pdf * * *
 FRC_8.5_O&PO_GoToE.pdf * * *
 FRC_8.5_OpenAction&O_URI.pdf * * *
@@ -300,9 +295,8 @@ FRC_8.5_PC&C_GoToE_T_T.pdf * * *
 FRC_8.5_PC_GoToE_T_R&P&A.pdf * * *
 FRC_8.5_PO_GoToE_T_R&N.pdf * * *
 
-#
 # xfa_specific
-#
+
 Choose.pdf * * *
 data_binding.pdf * * *
 Date_FormCale.pdf * * *
@@ -315,3 +309,15 @@ TimeField.pdf win,linux * *
 Test_CheckBox.pdf * * *
 Test_DateField_locale_zh_HK.pdf mac,win * *
 
+#
+# JavaScript tests
+#
+bug_679642.in * * noxfa
+bug_679643.in * * noxfa
+bug_735912.in * * noxfa
+
+#
+# Pixel tests
+#
+bug_492.in * nov8 *
+
diff --git a/testing/resources/javascript/bug_735912.in b/testing/resources/javascript/bug_735912.in
new file mode 100644
index 0000000000..7f42d1b7c2
--- /dev/null
+++ b/testing/resources/javascript/bug_735912.in
@@ -0,0 +1,115 @@
+{{header}}
+{{object 1 0}} <<
+  /Type /Catalog
+  /Pages 2 0 R
+  /AcroForm 4 0 R
+  /OpenAction 30 0 R
+>>
+endobj
+{{object 2 0}} <<
+  /Type /Pages
+  /Count 1
+  /Kids [
+    3 0 R
+  ]
+>>
+endobj
+% Page number 0.
+{{object 3 0}} <<
+  /Type /Page
+  /Parent 2 0 R
+  /Annots [
+    8 0 R
+    ]
+  /AA << /C 36 0 R >>
+>>
+endobj
+% Forms
+{{object 4 0}} <<
+  /XFA [
+    (xdp:xdp) 43 0 R
+    (form) 49 0 R
+    (</xdp:xdp>) 50 0 R
+  ]
+  /Fields [
+    8 0 R
+    ]
+>>
+endobj
+% Fields
+{{object 8 0}} <<
+  /FT /Tx
+  /T (MyField)
+  /Type /Annot
+  /Subtype /Widget
+  /Rect [100 300 500 400]
+  /AA << /Bl 34 0 R >>
+>>
+endobj
+% JS Action
+{{object 30 0}} <<
+  /Type /Action
+  /S /JavaScript
+  /JS 31 0 R
+>>
+endobj
+% JS program to exexute
+{{object 31 0}} <<
+>>
+stream
+this.getField("MyField").setFocus();
+app.alert("focused");
+endstream
+endobj
+% Lost Focus AAction MyField
+{{object 34 0}} <<
+  /Type /Action
+  /S /JavaScript
+  /JS 35 0 R
+>>
+endobj
+% JS program to exexute
+{{object 35 0}} <<
+>>
+stream
+this.removeField("MyField");
+app.alert("removed");
+endstream
+endobj
+% Closing Page 0
+{{object 36 0}} <<
+  /Type /Action
+  /S /JavaScript
+  /JS 37 0 R
+>>
+endobj
+% JS program to exexute
+{{object 37 0}} <<
+>>
+stream
+this.removeField("MyField");
+endstream
+endobj
+{{object 43 0}} <<
+>>stream
+<?xml version="1.0" encoding="UTF-8"?>
+<xdp:xdp xmlns:xdp="http://ns.adobe.com/xdp/">
+endstream
+endobj
+{{object 49 0}} <<
+>>stream
+<config></config>
+<template></template>
+endstream
+endobj
+{{object 50 0}} <<
+>>stream
+</xdp:xdp>
+endstream
+endobj
+{{xref}}
+trailer <<
+  /Root 1 0 R
+>>
+{{startxref}}
+%%EOF
diff --git a/testing/resources/javascript/bug_735912_expected.txt b/testing/resources/javascript/bug_735912_expected.txt
new file mode 100644
index 0000000000..7d0f42f517
--- /dev/null
+++ b/testing/resources/javascript/bug_735912_expected.txt
@@ -0,0 +1,2 @@
+Alert: removed
+Alert: focused