Annotation deleted while retrieving it in JS

Widgets as returned from GetWidgets() can pop out of existence
unexpectedly, so always return observed pointers.  This extends
the same pattern used elsewhere in the file to all occurrences.

BUG=679642

Review-Url: https://codereview.chromium.org/2624933002

6 files changed, 190 insertions, 28 deletions

diff --git a/fpdfsdk/cpdfsdk_interform.cpp b/fpdfsdk/cpdfsdk_interform.cpp
index a2f07b488f..7b7718030b 100644
--- a/fpdfsdk/cpdfsdk_interform.cpp
+++ b/fpdfsdk/cpdfsdk_interform.cpp
@@ -117,7 +117,7 @@ CPDFSDK_Widget* CPDFSDK_InterForm::GetWidget(CPDF_FormControl* pControl) const {
 
 void CPDFSDK_InterForm::GetWidgets(
     const CFX_WideString& sFieldName,
-    std::vector<CPDFSDK_Widget*>* widgets) const {
+    std::vector<CPDFSDK_Annot::ObservedPtr>* widgets) const {
   for (int i = 0, sz = m_pInterForm->CountFields(sFieldName); i < sz; ++i) {
     CPDF_FormField* pFormField = m_pInterForm->GetField(i, sFieldName);
     ASSERT(pFormField);
@@ -127,13 +127,13 @@ void CPDFSDK_InterForm::GetWidgets(
 
 void CPDFSDK_InterForm::GetWidgets(
     CPDF_FormField* pField,
-    std::vector<CPDFSDK_Widget*>* widgets) const {
+    std::vector<CPDFSDK_Annot::ObservedPtr>* widgets) const {
   for (int i = 0, sz = pField->CountControls(); i < sz; ++i) {
     CPDF_FormControl* pFormCtrl = pField->GetControl(i);
     ASSERT(pFormCtrl);
     CPDFSDK_Widget* pWidget = GetWidget(pFormCtrl);
     if (pWidget)
-      widgets->push_back(pWidget);
+      widgets->emplace_back(pWidget);
   }
 }
 
diff --git a/fpdfsdk/cpdfsdk_interform.h b/fpdfsdk/cpdfsdk_interform.h
index 9f35d560e6..032399c84e 100644
--- a/fpdfsdk/cpdfsdk_interform.h
+++ b/fpdfsdk/cpdfsdk_interform.h
@@ -15,6 +15,7 @@
 #include "core/fpdfdoc/ipdf_formnotify.h"
 #include "core/fxcrt/fx_basic.h"
 #include "core/fxge/fx_dib.h"
+#include "fpdfsdk/cpdfsdk_widget.h"
 
 class CPDF_Dictionary;
 class CPDF_FormControl;
@@ -22,7 +23,6 @@ class CPDF_FormField;
 class CPDF_InterForm;
 class CPDF_Object;
 class CPDFSDK_FormFillEnvironment;
-class CPDFSDK_Widget;
 
 #ifdef PDF_ENABLE_XFA
 class CPDFSDK_XFAWidget;
@@ -42,9 +42,9 @@ class CPDFSDK_InterForm : public IPDF_FormNotify {
   CPDFSDK_Widget* GetSibling(CPDFSDK_Widget* pWidget, bool bNext) const;
   CPDFSDK_Widget* GetWidget(CPDF_FormControl* pControl) const;
   void GetWidgets(const CFX_WideString& sFieldName,
-                  std::vector<CPDFSDK_Widget*>* widgets) const;
+                  std::vector<CPDFSDK_Annot::ObservedPtr>* widgets) const;
   void GetWidgets(CPDF_FormField* pField,
-                  std::vector<CPDFSDK_Widget*>* widgets) const;
+                  std::vector<CPDFSDK_Annot::ObservedPtr>* widgets) const;
 
   void AddMap(CPDF_FormControl* pControl, CPDFSDK_Widget* pWidget);
   void RemoveMap(CPDF_FormControl* pControl);
diff --git a/fpdfsdk/javascript/Document.cpp b/fpdfsdk/javascript/Document.cpp
index 29f9764806..7e4dc260f2 100644
--- a/fpdfsdk/javascript/Document.cpp
+++ b/fpdfsdk/javascript/Document.cpp
@@ -506,12 +506,16 @@ bool Document::removeField(IJS_Context* cc,
   CJS_Runtime* pRuntime = pContext->GetJSRuntime();
   CFX_WideString sFieldName = params[0].ToCFXWideString(pRuntime);
   CPDFSDK_InterForm* pInterForm = m_pFormFillEnv->GetInterForm();
-  std::vector<CPDFSDK_Widget*> widgets;
+  std::vector<CPDFSDK_Annot::ObservedPtr> widgets;
   pInterForm->GetWidgets(sFieldName, &widgets);
   if (widgets.empty())
     return true;
 
-  for (CPDFSDK_Widget* pWidget : widgets) {
+  for (const auto& pAnnot : widgets) {
+    CPDFSDK_Widget* pWidget = static_cast<CPDFSDK_Widget*>(pAnnot.Get());
+    if (!pWidget)
+      continue;
+
     CFX_FloatRect rcAnnot = pWidget->GetRect();
     --rcAnnot.left;
     --rcAnnot.bottom;
diff --git a/fpdfsdk/javascript/Field.cpp b/fpdfsdk/javascript/Field.cpp
index f8ac479a42..e04cbd696f 100644
--- a/fpdfsdk/javascript/Field.cpp
+++ b/fpdfsdk/javascript/Field.cpp
@@ -265,24 +265,28 @@ void Field::UpdateFormField(CPDFSDK_FormFillEnvironment* pFormFillEnv,
   CPDFSDK_InterForm* pInterForm = pFormFillEnv->GetInterForm();
 
   if (bResetAP) {
-    std::vector<CPDFSDK_Widget*> widgets;
+    std::vector<CPDFSDK_Annot::ObservedPtr> widgets;
     pInterForm->GetWidgets(pFormField, &widgets);
 
     int nFieldType = pFormField->GetFieldType();
     if (nFieldType == FIELDTYPE_COMBOBOX || nFieldType == FIELDTYPE_TEXTFIELD) {
-      for (CPDFSDK_Annot* pAnnot : widgets) {
-        bool bFormatted = false;
-        CPDFSDK_Annot::ObservedPtr pObserved(pAnnot);
-        CFX_WideString sValue =
-            static_cast<CPDFSDK_Widget*>(pObserved.Get())->OnFormat(bFormatted);
+      for (auto& pObserved : widgets) {
         if (pObserved) {
-          static_cast<CPDFSDK_Widget*>(pObserved.Get())
-              ->ResetAppearance(bFormatted ? &sValue : nullptr, false);
+          bool bFormatted = false;
+          CFX_WideString sValue = static_cast<CPDFSDK_Widget*>(pObserved.Get())
+                                      ->OnFormat(bFormatted);
+          if (pObserved) {  // Not redundant, may be clobbered by OnFormat.
+            static_cast<CPDFSDK_Widget*>(pObserved.Get())
+                ->ResetAppearance(bFormatted ? &sValue : nullptr, false);
+          }
         }
       }
     } else {
-      for (CPDFSDK_Widget* pWidget : widgets) {
-        pWidget->ResetAppearance(nullptr, false);
+      for (auto& pObserved : widgets) {
+        if (pObserved) {
+          static_cast<CPDFSDK_Widget*>(pObserved.Get())
+              ->ResetAppearance(nullptr, false);
+        }
       }
     }
   }
@@ -291,16 +295,18 @@ void Field::UpdateFormField(CPDFSDK_FormFillEnvironment* pFormFillEnv,
     // 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;
+    std::vector<CPDFSDK_Annot::ObservedPtr> widgets;
     pInterForm->GetWidgets(pFormField, &widgets);
 
     // TODO(dsinclair): Determine if all widgets share the same
     // CPDFSDK_InterForm. If that's the case, we can move the code to
     // |GetFormFillEnv| out of the loop.
-    for (CPDFSDK_Widget* pWidget : widgets) {
-      pWidget->GetInterForm()
-          ->GetFormFillEnv()
-          ->UpdateAllViews(nullptr, pWidget);
+    for (auto& pObserved : widgets) {
+      if (pObserved) {
+        CPDFSDK_Widget* pWidget = static_cast<CPDFSDK_Widget*>(pObserved.Get());
+        pWidget->GetInterForm()->GetFormFillEnv()->UpdateAllViews(nullptr,
+                                                                  pWidget);
+      }
     }
   }
 
@@ -1803,8 +1809,10 @@ bool Field::numItems(IJS_Context* cc,
 }
 
 bool Field::page(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) {
-  if (!vp.IsGetting())
+  if (!vp.IsGetting()) {
+    sError = JSGetStringFromID(IDS_STRING_JSREADONLY);
     return false;
+  }
 
   std::vector<CPDF_FormField*> FieldArray = GetFormFields(m_FieldName);
   if (FieldArray.empty())
@@ -1814,9 +1822,8 @@ bool Field::page(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) {
   if (!pFormField)
     return false;
 
-  std::vector<CPDFSDK_Widget*> widgets;
+  std::vector<CPDFSDK_Annot::ObservedPtr> widgets;
   m_pFormFillEnv->GetInterForm()->GetWidgets(pFormField, &widgets);
-
   if (widgets.empty()) {
     vp << (int32_t)-1;
     return true;
@@ -1824,13 +1831,21 @@ bool Field::page(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) {
 
   CJS_Runtime* pRuntime = CJS_Runtime::FromContext(cc);
   CJS_Array PageArray;
-  for (size_t i = 0; i < widgets.size(); ++i) {
-    CPDFSDK_PageView* pPageView = widgets[i]->GetPageView();
+  int i = 0;
+  for (const auto& pObserved : widgets) {
+    if (!pObserved) {
+      sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT);
+      return false;
+    }
+
+    auto pWidget = static_cast<CPDFSDK_Widget*>(pObserved.Get());
+    CPDFSDK_PageView* pPageView = pWidget->GetPageView();
     if (!pPageView)
       return false;
 
     PageArray.SetElement(
         pRuntime, i, CJS_Value(pRuntime, (int32_t)pPageView->GetPageIndex()));
+    ++i;
   }
 
   vp << PageArray;
diff --git a/testing/resources/javascript/bug_679642.in b/testing/resources/javascript/bug_679642.in
new file mode 100644
index 0000000000..2241723dd2
--- /dev/null
+++ b/testing/resources/javascript/bug_679642.in
@@ -0,0 +1,140 @@
+{{header}}
+{{object 1 0}} <<
+  /Type /Catalog
+  /Pages 2 0 R
+  /AcroForm 4 0 R
+  /OpenAction 10 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
+  /Resources <<
+    /Font <</F1 15 0 R>>
+  >>
+  /Contents [21 0 R]
+  /MediaBox [0 0 612 792]
+  /Annots [7 0 R 8 0 R 9 0 R]
+>>
+endobj
+% Forms
+{{object 4 0}} <<
+  /XFA [
+    (xdp:xdp) 23 0 R
+    (form) 29 0 R
+    (</xdp:xdp>) 30 0 R
+  ]
+  /Fields [
+    5 0 R
+  ]
+>>
+endobj
+% Fields
+{{object 5 0}} <<
+  /T (MyField)
+  /Kids [
+    6 0 R
+  ]
+  /Rect [100 100 400 400]
+>>
+endobj
+{{object 6 0}} <<
+  /Parent 5 0 R
+  /FT /Btn
+  /Kids [
+    7 0 R
+    8 0 R
+    9 0 R
+  ]
+  /Rect [200 200 220 220]
+>>
+endobj
+{{object 7 0}} <<
+  /Parent 6 0 R
+  /Type /Annot
+  /Subtype /Widget
+  /Rect [220 220 240 240]
+>>
+endobj
+{{object 8 0}} <<
+  /Parent 6 0 R
+  /Type /Annot
+  /Subtype /Widget
+  /Rect [240 240 260 260]
+>>
+endobj
+{{object 9 0}} <<
+  /Parent 6 0 R
+  /Type /Annot
+  /Subtype /Widget
+  /Rect [240 240 260 260]
+>>
+endobj
+% OpenAction action
+{{object 10 0}} <<
+  /Type /Action
+  /S /JavaScript
+  /JS 11 0 R
+>>
+endobj
+% JS program to exexute
+{{object 11 0}} <<
+>>
+stream
+var theName = "MyField";
+function Mangles() {
+  app.alert('Starting ...');
+  try {
+    var f = this.getField(theName);
+    Object.defineProperty(Array.prototype, 1, {
+      get: () => {
+        return this[1];
+      },
+      set: (v) => {
+        app.alert('Firing ...');
+        this.removeField(theName);
+        gc();
+        return false;
+      },
+      enumerable: true
+    });
+    f.page;
+  } catch (e) {
+    app.alert("failed: " + e);
+  }
+}
+Mangles();
+endstream
+endobj
+{{object 23 0}} <<
+>>stream
+<?xml version="1.0" encoding="UTF-8"?>
+<xdp:xdp xmlns:xdp="http://ns.adobe.com/xdp/">
+endstream
+endobj
+{{object 29 0}} <<
+>>stream
+<config></config>
+<template></template>
+endstream
+endobj
+{{object 30 0}} <<
+>>stream
+</xdp:xdp>
+endstream
+endobj
+{{xref}}
+trailer <<
+  /Root 1 0 R
+>>
+{{startxref}}
+%%EOF
diff --git a/testing/resources/javascript/bug_679642_expected.txt b/testing/resources/javascript/bug_679642_expected.txt
new file mode 100644
index 0000000000..9e99cc6883
--- /dev/null
+++ b/testing/resources/javascript/bug_679642_expected.txt
@@ -0,0 +1,3 @@
+Alert: Starting ...
+Alert: Firing ...
+Alert: failed: Field.page: Object no longer exists.