Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP

On Acrobat, if "/AP" is present on a text markup definition,
the coordinates used to draw the annotation come from "/Rect
values, whereas if "/AP" is not defined, the array defined
in /QuadPoints is used to grab the annotation coordinates from.
PDFium, on the other hand, uses "/Rect" regardless of
presence or absence of "/AP".

CL fixes PDFium to work similarly to Acrobat, in this case.

TEST=testing/resources/pixel/bug_585_*.in
BUG=pdfium:585

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

6 files changed, 267 insertions, 26 deletions

diff --git a/DEPS b/DEPS
index e3ef8e8372..e57e5af206 100644
--- a/DEPS
+++ b/DEPS
@@ -14,7 +14,7 @@ vars = {
   'gmock_revision': '29763965ab52f24565299976b936d1265cb6a271',
   'gtest_revision': '8245545b6dc9c4703e6496d1efd19e975ad2b038',
   'icu_revision': '2341038bf72869a5683a893a2b319a48ffec7f62',
-  'pdfium_tests_revision': '8485b3093524ddb7319e0381ab10c576e59d5091',
+  'pdfium_tests_revision': 'e86a1bac556194ee572b0cd37d04ea646c7b5320',
   'skia_revision': '39f7a10a04a914384944d8bf62621144ac4eeaa3',
   'tools_memory_revision': '427f10475e1a8d72424c29d00bf689122b738e5d',
   'trace_event_revision': '54b8455be9505c2cb0cf5c26bb86739c236471aa',
diff --git a/core/fpdfdoc/cpdf_annot.cpp b/core/fpdfdoc/cpdf_annot.cpp
index bee96a62ff..2cc767a066 100644
--- a/core/fpdfdoc/cpdf_annot.cpp
+++ b/core/fpdfdoc/cpdf_annot.cpp
@@ -20,6 +20,15 @@
 
 namespace {
 
+char kPDFiumKey_HasGeneratedAP[] = "PDFIUM_HasGeneratedAP";
+
+bool IsTextMarkupAnnotation(CPDF_Annot::Subtype type) {
+  return type == CPDF_Annot::Subtype::HIGHLIGHT ||
+         type == CPDF_Annot::Subtype::SQUIGGLY ||
+         type == CPDF_Annot::Subtype::STRIKEOUT ||
+         type == CPDF_Annot::Subtype::UNDERLINE;
+}
+
 bool ShouldGenerateAPForAnnotation(CPDF_Dictionary* pAnnotDict) {
   // If AP dictionary exists, we use the appearance defined in the
   // existing AP dictionary.
@@ -40,6 +49,8 @@ CPDF_Annot::CPDF_Annot(CPDF_Dictionary* pDict,
       m_bOpenState(false),
       m_pPopupAnnot(nullptr) {
   m_nSubtype = StringToAnnotSubtype(m_pAnnotDict->GetStringFor("Subtype"));
+  m_bIsTextMarkupAnnotation = IsTextMarkupAnnotation(m_nSubtype);
+  m_bHasGeneratedAP = m_pAnnotDict->GetBooleanFor(kPDFiumKey_HasGeneratedAP);
   GenerateAPIfNeeded();
 }
 
@@ -53,24 +64,30 @@ void CPDF_Annot::GenerateAPIfNeeded() {
   if (!ShouldGenerateAPForAnnotation(m_pAnnotDict))
     return;
 
+  bool result = false;
   if (m_nSubtype == CPDF_Annot::Subtype::CIRCLE)
-    CPVT_GenerateAP::GenerateCircleAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateCircleAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::HIGHLIGHT)
-    CPVT_GenerateAP::GenerateHighlightAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateHighlightAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::INK)
-    CPVT_GenerateAP::GenerateInkAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateInkAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::POPUP)
-    CPVT_GenerateAP::GeneratePopupAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GeneratePopupAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::SQUARE)
-    CPVT_GenerateAP::GenerateSquareAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateSquareAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::SQUIGGLY)
-    CPVT_GenerateAP::GenerateSquigglyAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateSquigglyAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::STRIKEOUT)
-    CPVT_GenerateAP::GenerateStrikeOutAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateStrikeOutAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::TEXT)
-    CPVT_GenerateAP::GenerateTextAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateTextAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::UNDERLINE)
-    CPVT_GenerateAP::GenerateUnderlineAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateUnderlineAP(m_pDocument, m_pAnnotDict);
+
+  if (result) {
+    m_pAnnotDict->SetBooleanFor(kPDFiumKey_HasGeneratedAP, result);
+    m_bHasGeneratedAP = result;
+  }
 }
 
 bool CPDF_Annot::ShouldDrawAnnotation() {
@@ -91,11 +108,23 @@ CPDF_Annot::Subtype CPDF_Annot::GetSubtype() const {
   return m_nSubtype;
 }
 
+CFX_FloatRect CPDF_Annot::RectForDrawing() const {
+  if (!m_pAnnotDict)
+    return CFX_FloatRect();
+
+  bool bShouldUseQuadPointsCoords =
+      m_bIsTextMarkupAnnotation && m_bHasGeneratedAP;
+  if (bShouldUseQuadPointsCoords)
+    return RectFromQuadPoints(m_pAnnotDict);
+
+  return m_pAnnotDict->GetRectFor("Rect");
+}
+
 CFX_FloatRect CPDF_Annot::GetRect() const {
   if (!m_pAnnotDict)
     return CFX_FloatRect();
 
-  CFX_FloatRect rect = m_pAnnotDict->GetRectFor("Rect");
+  CFX_FloatRect rect = RectForDrawing();
   rect.Normalize();
   return rect;
 }
@@ -176,6 +205,26 @@ static CPDF_Form* FPDFDOC_Annot_GetMatrix(const CPDF_Page* pPage,
 }
 
 // Static.
+CFX_FloatRect CPDF_Annot::RectFromQuadPoints(CPDF_Dictionary* pAnnotDict) {
+  CPDF_Array* pArray = pAnnotDict->GetArrayFor("QuadPoints");
+  if (!pArray)
+    return CFX_FloatRect();
+
+  // QuadPoints are defined with 4 pairs of numbers
+  // ([ pair0, pair1, pair2, pair3 ]), where
+  // pair0 = top_left
+  // pair1 = top_right
+  // pair2 = bottom_left
+  // pair3 = bottom_right
+  //
+  // On the other hand, /Rect is define as 2 pairs [pair0, pair1] where:
+  // pair0 = bottom_left
+  // pair1 = top_right.
+  return CFX_FloatRect(pArray->GetNumberAt(4), pArray->GetNumberAt(5),
+                       pArray->GetNumberAt(2), pArray->GetNumberAt(3));
+}
+
+// Static.
 bool CPDF_Annot::IsAnnotationHidden(CPDF_Dictionary* pAnnotDict) {
   return !!(pAnnotDict->GetIntegerFor("F") & ANNOTFLAG_HIDDEN);
 }
diff --git a/core/fpdfdoc/cpvt_generateap.cpp b/core/fpdfdoc/cpvt_generateap.cpp
index decc408c33..d37aaf3acb 100644
--- a/core/fpdfdoc/cpvt_generateap.cpp
+++ b/core/fpdfdoc/cpvt_generateap.cpp
@@ -586,7 +586,8 @@ CPDF_Dictionary* GenerateResourceDict(CPDF_Dictionary* pExtGStateDict,
 void GenerateAndSetAPDict(CPDF_Document* pDoc,
                           CPDF_Dictionary* pAnnotDict,
                           const CFX_ByteTextBuf& sAppStream,
-                          CPDF_Dictionary* pResourceDict) {
+                          CPDF_Dictionary* pResourceDict,
+                          bool bIsTextMarkupAnnotation) {
   CPDF_Dictionary* pAPDict = new CPDF_Dictionary;
   pAnnotDict->SetFor("AP", pAPDict);
 
@@ -602,7 +603,9 @@ void GenerateAndSetAPDict(CPDF_Document* pDoc,
   pStreamDict->SetStringFor("Subtype", "Form");
   pStreamDict->SetMatrixFor("Matrix", CFX_Matrix());
 
-  CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect");
+  CFX_FloatRect rect = bIsTextMarkupAnnotation
+                           ? CPDF_Annot::RectFromQuadPoints(pAnnotDict)
+                           : pAnnotDict->GetRectFor("Rect");
   pStreamDict->SetRectFor("BBox", rect);
 
   pStreamDict->SetFor("Resources", pResourceDict);
@@ -783,7 +786,8 @@ bool CPVT_GenerateAP::GenerateCircleAP(CPDF_Document* pDoc,
       GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal");
   CPDF_Dictionary* pResourceDict =
       GenerateResourceDict(pExtGStateDict, nullptr);
-  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict);
+  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict,
+                       false /*IsTextMarkupAnnotation*/);
   return true;
 }
 
@@ -797,7 +801,7 @@ bool CPVT_GenerateAP::GenerateHighlightAP(CPDF_Document* pDoc,
                                           CPVT_Color(CPVT_Color::kRGB, 1, 1, 0),
                                           PaintOperation::FILL);
 
-  CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect");
+  CFX_FloatRect rect = CPDF_Annot::RectFromQuadPoints(pAnnotDict);
   rect.Normalize();
 
   sAppStream << rect.left << " " << rect.top << " m " << rect.right << " "
@@ -809,7 +813,8 @@ bool CPVT_GenerateAP::GenerateHighlightAP(CPDF_Document* pDoc,
       GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Multiply");
   CPDF_Dictionary* pResourceDict =
       GenerateResourceDict(pExtGStateDict, nullptr);
-  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict);
+  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict,
+                       true /*IsTextMarkupAnnotation*/);
 
   return true;
 }
@@ -863,7 +868,8 @@ bool CPVT_GenerateAP::GenerateInkAP(CPDF_Document* pDoc,
       GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal");
   CPDF_Dictionary* pResourceDict =
       GenerateResourceDict(pExtGStateDict, nullptr);
-  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict);
+  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict,
+                       false /*IsTextMarkupAnnotation*/);
   return true;
 }
 
@@ -885,7 +891,8 @@ bool CPVT_GenerateAP::GenerateTextAP(CPDF_Document* pDoc,
       GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal");
   CPDF_Dictionary* pResourceDict =
       GenerateResourceDict(pExtGStateDict, nullptr);
-  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict);
+  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict,
+                       false /*IsTextMarkupAnnotation*/);
   return true;
 }
 
@@ -899,7 +906,7 @@ bool CPVT_GenerateAP::GenerateUnderlineAP(CPDF_Document* pDoc,
                                           CPVT_Color(CPVT_Color::kRGB, 0, 0, 0),
                                           PaintOperation::STROKE);
 
-  CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect");
+  CFX_FloatRect rect = CPDF_Annot::RectFromQuadPoints(pAnnotDict);
   rect.Normalize();
 
   FX_FLOAT fLineWidth = 1.0;
@@ -911,7 +918,8 @@ bool CPVT_GenerateAP::GenerateUnderlineAP(CPDF_Document* pDoc,
       GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal");
   CPDF_Dictionary* pResourceDict =
       GenerateResourceDict(pExtGStateDict, nullptr);
-  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict);
+  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict,
+                       true /*IsTextMarkupAnnotation*/);
   return true;
 }
 
@@ -949,7 +957,8 @@ bool CPVT_GenerateAP::GeneratePopupAP(CPDF_Document* pDoc,
     return false;
 
   sAppStream << GetPopupContentsString(pDoc, *pAnnotDict, pDefFont, sFontName);
-  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict);
+  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict,
+                       false /*IsTextMarkupAnnotation*/);
   return true;
 }
 
@@ -996,7 +1005,8 @@ bool CPVT_GenerateAP::GenerateSquareAP(CPDF_Document* pDoc,
       GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal");
   CPDF_Dictionary* pResourceDict =
       GenerateResourceDict(pExtGStateDict, nullptr);
-  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict);
+  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict,
+                       false /*IsTextMarkupAnnotation*/);
   return true;
 }
 
@@ -1010,7 +1020,7 @@ bool CPVT_GenerateAP::GenerateSquigglyAP(CPDF_Document* pDoc,
                                           CPVT_Color(CPVT_Color::kRGB, 0, 0, 0),
                                           PaintOperation::STROKE);
 
-  CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect");
+  CFX_FloatRect rect = CPDF_Annot::RectFromQuadPoints(pAnnotDict);
   rect.Normalize();
 
   FX_FLOAT fLineWidth = 1.0;
@@ -1044,7 +1054,8 @@ bool CPVT_GenerateAP::GenerateSquigglyAP(CPDF_Document* pDoc,
       GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal");
   CPDF_Dictionary* pResourceDict =
       GenerateResourceDict(pExtGStateDict, nullptr);
-  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict);
+  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict,
+                       true /*IsTextMarkupAnnotation*/);
   return true;
 }
 
@@ -1058,7 +1069,7 @@ bool CPVT_GenerateAP::GenerateStrikeOutAP(CPDF_Document* pDoc,
                                           CPVT_Color(CPVT_Color::kRGB, 0, 0, 0),
                                           PaintOperation::STROKE);
 
-  CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect");
+  CFX_FloatRect rect = CPDF_Annot::RectFromQuadPoints(pAnnotDict);
   rect.Normalize();
 
   FX_FLOAT fLineWidth = 1.0;
@@ -1070,7 +1081,8 @@ bool CPVT_GenerateAP::GenerateStrikeOutAP(CPDF_Document* pDoc,
       GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal");
   CPDF_Dictionary* pResourceDict =
       GenerateResourceDict(pExtGStateDict, nullptr);
-  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict);
+  GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict,
+                       true /*IsTextMarkupAnnotation*/);
   return true;
 }
 
diff --git a/core/fpdfdoc/include/cpdf_annot.h b/core/fpdfdoc/include/cpdf_annot.h
index c16decc7d6..43f58931d3 100644
--- a/core/fpdfdoc/include/cpdf_annot.h
+++ b/core/fpdfdoc/include/cpdf_annot.h
@@ -71,6 +71,7 @@ class CPDF_Annot {
   static CPDF_Annot::Subtype StringToAnnotSubtype(
       const CFX_ByteString& sSubtype);
   static CFX_ByteString AnnotSubtypeToString(CPDF_Annot::Subtype nSubtype);
+  static CFX_FloatRect RectFromQuadPoints(CPDF_Dictionary* pAnnotDict);
 
   CPDF_Annot(CPDF_Dictionary* pDict, CPDF_Document* pDocument, bool bToOwnDict);
   ~CPDF_Annot();
@@ -102,6 +103,8 @@ class CPDF_Annot {
   void GenerateAPIfNeeded();
   bool ShouldDrawAnnotation();
 
+  CFX_FloatRect RectForDrawing() const;
+
   // For regular annotations, |m_pAnnotDict| is not owned. For
   // our artificially created popup annotations, |m_pAnnotDict|
   // is owned by this class.
@@ -112,6 +115,8 @@ class CPDF_Annot {
   std::map<CPDF_Stream*, std::unique_ptr<CPDF_Form>> m_APMap;
   // |m_bOpenState| is only set for popup annotations.
   bool m_bOpenState;
+  bool m_bHasGeneratedAP;
+  bool m_bIsTextMarkupAnnotation;
   // Not owned. If there is a valid pointer in |m_pPopupAnnot|,
   // then this annot is never a popup.
   CPDF_Annot* m_pPopupAnnot;
diff --git a/testing/resources/pixel/bug_585.in b/testing/resources/pixel/bug_585.in
new file mode 100644
index 0000000000..b8b56ad427
--- /dev/null
+++ b/testing/resources/pixel/bug_585.in
@@ -0,0 +1,175 @@
+{{header}}
+{{object 1 0}} <<
+  /Type /Catalog
+  /Pages 2 0 R
+>>
+endobj
+{{object 2 0}} <<
+  /Type /Pages
+  /Count 1
+  /Kids [
+    10 0 R
+  ]
+>>
+endobj
+% Page number 0.
+{{object 10 0}} <<
+  /Type /Page
+  /Parent 2 0 R
+  /Resources <<
+    /Font <</F1 15 0 R>>
+  >>
+  /MediaBox [0 0 612 792]
+  /Annots [
+    22 0 R
+    23 0 R
+    24 0 R
+    25 0 R
+  ]
+  /Tabs /R
+>>
+endobj
+
+{{object 22 0}} <<
+  /Type /Annot
+  /Subtype /Highlight
+  /Rect [ -1 -1 -1 -1 ]
+  /NM (Annot-1)
+  /F 4
+  /QuadPoints [ 475 688 512 688 475 679 512 679 ]
+  /C [ 0.0001108646 0.001760244 0.9982184 ]
+  /Contents ()
+>>
+endobj
+
+{{object 23 0}} <<
+  /Border [
+    0
+    0
+    1
+  ]
+  /C [
+    0.294118
+    0.6
+    1
+  ]
+  /CA 1
+  /CreationDate (D:20150312175256+08'00')
+  /F 4
+  /M (D:20150312175256+08'00')
+  /NM (7f264ba2-e270-42a1-a390-eb41278072ff)
+  /QuadPoints [
+    227.567
+    688.016
+    298.115
+    688.016
+    227.567
+    679.292
+    298.115
+    679.292
+  ]
+  /RC (<?xml version="1.0"?><body xmlns="http://www.w3.org/1999/xhtml" xmlns:xfa="http://www.xfa.org/schema/xfa-data/1.0/" xfa:APIVersion="Acroform:2.7.0.0" xfa:spec="2.1"><p style="text-align:left" dir="ltr"><span style="line-height:0pt,font-size:0pt;font-style:normal;font-weight:normal;color:#000000;font-family:Helvetica"></span>\r\n</p>\r\n</body>\r\n)
+  /Rect [
+    -1
+    -1
+    -1
+    -1
+  ]
+  /Subj (Squiggly)
+  /Subtype /Squiggly
+  /T (Administrator)
+  /Type /Annot
+>>
+endobj
+
+{{object 24 0}} <<
+  /Border [
+    0
+    0
+    1
+  ]
+  /C [
+    0.278431
+    0.603922
+    0.6
+  ]
+  /CA 1
+  /CreationDate (D:20150312175350+08'00')
+  /F 4
+  /IRT 6 0 R
+  /IT /StrikeOutTextEdit
+  /M (D:20150312175350+08'00')
+  /NM (2912a3cf-3b30-48a7-8531-431ce468e6a8)
+  /P 4 0 R
+  /QuadPoints [
+    186.875
+    714.836
+    293.483
+    714.836
+    186.875
+    706.064
+    293.483
+    706.064
+  ]
+  /RT /Group
+  /Rect [
+    -1
+    -1
+    -1
+    -1
+  ]
+  /Subj (Replace)
+  /Subtype /StrikeOut
+  /T (Administrator)
+  /Type /Annot
+>>
+endobj
+
+{{object 25 0}} <<
+  /Border [
+    0
+    0
+    1
+  ]
+  /C [
+    0.2
+    0.619608
+    0
+  ]
+  /CA 1
+  /CreationDate (D:20150312175318+08'00')
+  /F 4
+  /M (D:20150312175318+08'00')
+  /NM (c99bdf16-3040-4bf6-9b8a-a80cc563a6a7)
+  /P 4 0 R
+  /Popup 6 0 R
+  /QuadPoints [
+    204.395
+    647.984
+    403.895
+    647.984
+    204.395
+    636.872
+    403.895
+    636.872
+  ]
+  /RC (<?xml version="1.0"?><body xmlns="http://www.w3.org/1999/xhtml" xmlns:xfa="http://www.xfa.org/schema/xfa-data/1.0/" xfa:APIVersion="Acroform:2.7.0.0" xfa:spec="2.1"><p style="text-align:left" dir="ltr"><span style="line-height:0pt,font-size:0pt;font-style:normal;font-weight:normal;color:#000000;font-family:Helvetica"></span>\r\n</p>\r\n</body>\r\n)
+  /Rect [
+    -1
+    -1
+    -1
+    -1
+  ]
+  /Subj (Underline)
+  /Subtype /Underline
+  /T (Administrator)
+  /Type /Annot
+>>
+endobj
+
+{{xref}}
+trailer <<
+  /Root 1 0 R
+>>
+{{startxref}}
+%%EOF
diff --git a/testing/resources/pixel/bug_585_expected.pdf.0.png b/testing/resources/pixel/bug_585_expected.pdf.0.png
new file mode 100644
index 0000000000..34bc892231
--- /dev/null
+++ b/testing/resources/pixel/bug_585_expected.pdf.0.png