Custom toString() methods may delete annots.

In this case, we observe the destruction of the object, but have
unfortunately saved a pointer to it in a local variable.

BUG=679643

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

4 files changed, 171 insertions, 21 deletions

diff --git a/fpdfsdk/javascript/Annot.cpp b/fpdfsdk/javascript/Annot.cpp
index 0c16b3b05a..1aef4634f2 100644
--- a/fpdfsdk/javascript/Annot.cpp
+++ b/fpdfsdk/javascript/Annot.cpp
@@ -38,20 +38,24 @@ Annot::Annot(CJS_Object* pJSObject) : CJS_EmbedObj(pJSObject) {}
 Annot::~Annot() {}
 
 bool Annot::hidden(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) {
-  CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot.Get());
-  if (!baAnnot)
-    return false;
-
   if (vp.IsGetting()) {
-    CPDF_Annot* pPDFAnnot = baAnnot->GetPDFAnnot();
+    if (!m_pAnnot) {
+      sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT);
+      return false;
+    }
+    CPDF_Annot* pPDFAnnot = ToBAAnnot(m_pAnnot.Get())->GetPDFAnnot();
     vp << CPDF_Annot::IsAnnotationHidden(pPDFAnnot->GetAnnotDict());
     return true;
   }
 
   bool bHidden;
-  vp >> bHidden;
+  vp >> bHidden;  // May invalidate m_pAnnot.
+  if (!m_pAnnot) {
+    sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT);
+    return false;
+  }
 
-  uint32_t flags = baAnnot->GetFlags();
+  uint32_t flags = ToBAAnnot(m_pAnnot.Get())->GetFlags();
   if (bHidden) {
     flags |= ANNOTFLAG_HIDDEN;
     flags |= ANNOTFLAG_INVISIBLE;
@@ -63,23 +67,28 @@ bool Annot::hidden(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) {
     flags &= ~ANNOTFLAG_NOVIEW;
     flags |= ANNOTFLAG_PRINT;
   }
-  baAnnot->SetFlags(flags);
+  ToBAAnnot(m_pAnnot.Get())->SetFlags(flags);
   return true;
 }
 
 bool Annot::name(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) {
-  CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot.Get());
-  if (!baAnnot)
-    return false;
-
   if (vp.IsGetting()) {
-    vp << baAnnot->GetAnnotName();
+    if (!m_pAnnot) {
+      sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT);
+      return false;
+    }
+    vp << ToBAAnnot(m_pAnnot.Get())->GetAnnotName();
     return true;
   }
 
   CFX_WideString annotName;
-  vp >> annotName;
-  baAnnot->SetAnnotName(annotName);
+  vp >> annotName;  // May invalidate m_pAnnot.
+  if (!m_pAnnot) {
+    sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT);
+    return false;
+  }
+
+  ToBAAnnot(m_pAnnot.Get())->SetAnnotName(annotName);
   return true;
 }
 
@@ -88,12 +97,12 @@ bool Annot::type(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) {
     sError = JSGetStringFromID(IDS_STRING_JSREADONLY);
     return false;
   }
-
-  CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot.Get());
-  if (!baAnnot)
+  if (!m_pAnnot) {
+    sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT);
     return false;
-
-  vp << CPDF_Annot::AnnotSubtypeToString(baAnnot->GetAnnotSubtype());
+  }
+  vp << CPDF_Annot::AnnotSubtypeToString(
+      ToBAAnnot(m_pAnnot.Get())->GetAnnotSubtype());
   return true;
 }
 
diff --git a/fpdfsdk/javascript/JS_Value.h b/fpdfsdk/javascript/JS_Value.h
index ff2c620980..313f0c3e7b 100644
--- a/fpdfsdk/javascript/JS_Value.h
+++ b/fpdfsdk/javascript/JS_Value.h
@@ -92,6 +92,7 @@ class CJS_PropValue {
   CJS_Runtime* GetJSRuntime() const { return m_pJSRuntime; }
   CJS_Value* GetJSValue() { return &m_Value; }
 
+  // These calls may re-enter JS (and hence invalidate objects).
   void operator<<(int val);
   void operator>>(int&) const;
   void operator<<(bool val);
@@ -127,13 +128,15 @@ class CJS_Array {
   virtual ~CJS_Array();
 
   void Attach(v8::Local<v8::Array> pArray);
+  int GetLength(CJS_Runtime* pRuntime) const;
+
+  // These two calls may re-enter JS (and hence invalidate objects).
   void GetElement(CJS_Runtime* pRuntime,
                   unsigned index,
                   CJS_Value& value) const;
   void SetElement(CJS_Runtime* pRuntime,
                   unsigned index,
                   const CJS_Value& value);
-  int GetLength(CJS_Runtime* pRuntime) const;
 
   v8::Local<v8::Array> ToV8Array(CJS_Runtime* pRuntime) const;
 
diff --git a/testing/resources/javascript/bug_679643.in b/testing/resources/javascript/bug_679643.in
new file mode 100644
index 0000000000..e9643860f7
--- /dev/null
+++ b/testing/resources/javascript/bug_679643.in
@@ -0,0 +1,135 @@
+{{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 annots = this.getAnnots();
+    annots[0].name = {
+      toString: () => {
+        app.alert('Firing ...');
+        this.removeField(theName);
+        gc();
+        return false;
+      }
+    };
+  } 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_679643_expected.txt b/testing/resources/javascript/bug_679643_expected.txt
new file mode 100644
index 0000000000..36d4a31344
--- /dev/null
+++ b/testing/resources/javascript/bug_679643_expected.txt
@@ -0,0 +1,3 @@
+Alert: Starting ...
+Alert: Firing ...
+Alert: failed: Annot.name: Object no longer exists.