Deprecate FPDFDest_GetPageIndex and create a fixed version.

FPDFDest_GetDestPageIndex() has a well defined return value for
errors (-1). Keeping FPDFDest_GetPageIndex() to avoid changing
behavior of the old API for whoever relies on it.

Bug: pdfium:938
Change-Id: Iad528923cb156e957a419540c262a65f45cb777d
Reviewed-on: https://pdfium-review.googlesource.com/24811
Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>

8 files changed, 72 insertions, 9 deletions

diff --git a/core/fpdfdoc/cpdf_dest.cpp b/core/fpdfdoc/cpdf_dest.cpp
index 0098f73f66..015cdcbc09 100644
--- a/core/fpdfdoc/cpdf_dest.cpp
+++ b/core/fpdfdoc/cpdf_dest.cpp
@@ -38,7 +38,7 @@ CPDF_Dest::CPDF_Dest(CPDF_Object* pObj) : m_pObj(pObj) {}
 
 CPDF_Dest::~CPDF_Dest() {}
 
-int CPDF_Dest::GetPageIndex(CPDF_Document* pDoc) const {
+int CPDF_Dest::GetPageIndexDeprecated(CPDF_Document* pDoc) const {
   CPDF_Array* pArray = ToArray(m_pObj.Get());
   if (!pArray)
     return 0;
@@ -46,10 +46,31 @@ int CPDF_Dest::GetPageIndex(CPDF_Document* pDoc) const {
   CPDF_Object* pPage = pArray->GetDirectObjectAt(0);
   if (!pPage)
     return 0;
+
   if (pPage->IsNumber())
     return pPage->GetInteger();
+
   if (!pPage->IsDictionary())
     return 0;
+
+  return pDoc->GetPageIndex(pPage->GetObjNum());
+}
+
+int CPDF_Dest::GetDestPageIndex(CPDF_Document* pDoc) const {
+  CPDF_Array* pArray = ToArray(m_pObj.Get());
+  if (!pArray)
+    return -1;
+
+  CPDF_Object* pPage = pArray->GetDirectObjectAt(0);
+  if (!pPage)
+    return -1;
+
+  if (pPage->IsNumber())
+    return pPage->GetInteger();
+
+  if (!pPage->IsDictionary())
+    return -1;
+
   return pDoc->GetPageIndex(pPage->GetObjNum());
 }
 
diff --git a/core/fpdfdoc/cpdf_dest.h b/core/fpdfdoc/cpdf_dest.h
index 584669a82d..709a2470c5 100644
--- a/core/fpdfdoc/cpdf_dest.h
+++ b/core/fpdfdoc/cpdf_dest.h
@@ -23,7 +23,13 @@ class CPDF_Dest {
 
   CPDF_Object* GetObject() const { return m_pObj.Get(); }
   ByteString GetRemoteName() const;
-  int GetPageIndex(CPDF_Document* pDoc) const;
+
+  // Deprecated. Use GetDestPageIndex instead.
+  // This method is wrong. It returns 0 for errors, when it could mean the first
+  // page as well. Keeping it avoids changing the behavior of
+  // FPDFDest_GetPageIndex().
+  int GetPageIndexDeprecated(CPDF_Document* pDoc) const;
+  int GetDestPageIndex(CPDF_Document* pDoc) const;
   uint32_t GetPageObjNum() const;
 
   // Returns the zoom mode, as one of the PDFDEST_VIEW_* values in fpdf_doc.h.
diff --git a/fpdfsdk/fpdfdoc.cpp b/fpdfsdk/fpdfdoc.cpp
index 47ecf4223b..4b51d6391d 100644
--- a/fpdfsdk/fpdfdoc.cpp
+++ b/fpdfsdk/fpdfdoc.cpp
@@ -200,14 +200,29 @@ FPDFAction_GetURIPath(FPDF_DOCUMENT document,
 }
 
 FPDF_EXPORT unsigned long FPDF_CALLCONV
-FPDFDest_GetPageIndex(FPDF_DOCUMENT document, FPDF_DEST pDict) {
-  if (!pDict)
+FPDFDest_GetPageIndex(FPDF_DOCUMENT document, FPDF_DEST pDest) {
+  if (!pDest)
     return 0;
+
   CPDF_Document* pDoc = CPDFDocumentFromFPDFDocument(document);
   if (!pDoc)
     return 0;
-  CPDF_Dest dest(static_cast<CPDF_Array*>(pDict));
-  return dest.GetPageIndex(pDoc);
+
+  CPDF_Dest dest(static_cast<CPDF_Array*>(pDest));
+  return dest.GetPageIndexDeprecated(pDoc);
+}
+
+FPDF_EXPORT long FPDF_CALLCONV FPDFDest_GetDestPageIndex(FPDF_DOCUMENT document,
+                                                         FPDF_DEST pDest) {
+  if (!pDest)
+    return -1;
+
+  CPDF_Document* pDoc = CPDFDocumentFromFPDFDocument(document);
+  if (!pDoc)
+    return -1;
+
+  CPDF_Dest dest(static_cast<CPDF_Array*>(pDest));
+  return dest.GetDestPageIndex(pDoc);
 }
 
 FPDF_EXPORT unsigned long FPDF_CALLCONV
diff --git a/fpdfsdk/fpdfdoc_embeddertest.cpp b/fpdfsdk/fpdfdoc_embeddertest.cpp
index d346330486..abe3d8c646 100644
--- a/fpdfsdk/fpdfdoc_embeddertest.cpp
+++ b/fpdfsdk/fpdfdoc_embeddertest.cpp
@@ -20,26 +20,31 @@ TEST_F(FPDFDocEmbeddertest, DestGetPageIndex) {
 
   // NULL FPDF_DEST case.
   EXPECT_EQ(0U, FPDFDest_GetPageIndex(document(), nullptr));
+  EXPECT_EQ(-1, FPDFDest_GetDestPageIndex(document(), nullptr));
 
   // Page number directly in item from Dests NameTree.
   FPDF_DEST dest = FPDF_GetNamedDestByName(document(), "First");
   EXPECT_TRUE(dest);
   EXPECT_EQ(1U, FPDFDest_GetPageIndex(document(), dest));
+  EXPECT_EQ(1, FPDFDest_GetDestPageIndex(document(), dest));
 
   // Page number via object reference in item from Dests NameTree.
   dest = FPDF_GetNamedDestByName(document(), "Next");
   EXPECT_TRUE(dest);
   EXPECT_EQ(1U, FPDFDest_GetPageIndex(document(), dest));
+  EXPECT_EQ(1, FPDFDest_GetDestPageIndex(document(), dest));
 
   // Page number directly in item from Dests dictionary.
   dest = FPDF_GetNamedDestByName(document(), "FirstAlternate");
   EXPECT_TRUE(dest);
   EXPECT_EQ(11U, FPDFDest_GetPageIndex(document(), dest));
+  EXPECT_EQ(11, FPDFDest_GetDestPageIndex(document(), dest));
 
   // Invalid object reference in item from Dests NameTree.
   dest = FPDF_GetNamedDestByName(document(), "LastAlternate");
   EXPECT_TRUE(dest);
   EXPECT_EQ(0U, FPDFDest_GetPageIndex(document(), dest));
+  EXPECT_EQ(-1, FPDFDest_GetDestPageIndex(document(), dest));
 }
 
 TEST_F(FPDFDocEmbeddertest, DestGetView) {
@@ -106,6 +111,7 @@ TEST_F(FPDFDocEmbeddertest, DestGetLocationInPage) {
 
   // NULL FPDF_DEST case.
   EXPECT_EQ(0U, FPDFDest_GetPageIndex(document(), nullptr));
+  EXPECT_EQ(-1, FPDFDest_GetDestPageIndex(document(), nullptr));
 
   FPDF_DEST dest = FPDF_GetNamedDestByName(document(), "First");
   EXPECT_TRUE(dest);
@@ -134,6 +140,7 @@ TEST_F(FPDFDocEmbeddertest, BUG_680376) {
   EXPECT_TRUE(dest);
   EXPECT_EQ(static_cast<unsigned long>(-1),
             FPDFDest_GetPageIndex(document(), dest));
+  EXPECT_EQ(-1, FPDFDest_GetDestPageIndex(document(), dest));
 }
 
 TEST_F(FPDFDocEmbeddertest, ActionGetFilePath) {
diff --git a/fpdfsdk/fpdfview_c_api_test.c b/fpdfsdk/fpdfview_c_api_test.c
index abffb7676f..193050e84a 100644
--- a/fpdfsdk/fpdfview_c_api_test.c
+++ b/fpdfsdk/fpdfview_c_api_test.c
@@ -107,6 +107,7 @@ int CheckPDFiumCApi() {
     CHK(FPDFAction_GetDest);
     CHK(FPDFAction_GetFilePath);
     CHK(FPDFAction_GetURIPath);
+    CHK(FPDFDest_GetDestPageIndex);
     CHK(FPDFDest_GetPageIndex);
     CHK(FPDFDest_GetLocationInPage);
     CHK(FPDFDest_GetView);
diff --git a/fpdfsdk/fsdk_actionhandler.cpp b/fpdfsdk/fsdk_actionhandler.cpp
index 0059bb919c..49e00411b8 100644
--- a/fpdfsdk/fsdk_actionhandler.cpp
+++ b/fpdfsdk/fsdk_actionhandler.cpp
@@ -362,7 +362,7 @@ void CPDFSDK_ActionHandler::DoAction_GoTo(
   ASSERT(pPDFDocument);
 
   CPDF_Dest MyDest = action.GetDest(pPDFDocument);
-  int nPageIndex = MyDest.GetPageIndex(pPDFDocument);
+  int nPageIndex = MyDest.GetPageIndexDeprecated(pPDFDocument);
   int nFitType = MyDest.GetZoomMode();
   const CPDF_Array* pMyArray = ToArray(MyDest.GetObject());
   std::vector<float> posArray;
diff --git a/fxjs/cjs_document.cpp b/fxjs/cjs_document.cpp
index 76d3f18b40..9af0f07f75 100644
--- a/fxjs/cjs_document.cpp
+++ b/fxjs/cjs_document.cpp
@@ -1467,8 +1467,8 @@ CJS_Return Document::gotoNamedDest(
       scrollPositionArray.push_back(arrayObject->GetFloatAt(i));
   }
   pRuntime->BeginBlock();
-  m_pFormFillEnv->DoGoToAction(dest.GetPageIndex(pDocument), dest.GetZoomMode(),
-                               scrollPositionArray.data(),
+  m_pFormFillEnv->DoGoToAction(dest.GetPageIndexDeprecated(pDocument),
+                               dest.GetZoomMode(), scrollPositionArray.data(),
                                scrollPositionArray.size());
   pRuntime->EndBlock();
   return CJS_Return(true);
diff --git a/public/fpdf_doc.h b/public/fpdf_doc.h
index b523575967..7ec232c45e 100644
--- a/public/fpdf_doc.h
+++ b/public/fpdf_doc.h
@@ -177,15 +177,28 @@ FPDFAction_GetURIPath(FPDF_DOCUMENT document,
                       void* buffer,
                       unsigned long buflen);
 
+// Deprecated. Use FPDFDest_GetDestPageIndex() instead.
+//
 // Get the page index of |dest|.
 //
 //   document - handle to the document.
 //   dest     - handle to the destination.
 //
 // Returns the page index containing |dest|. Page indices start from 0.
+// On an error, returns 0 or -1. Note that 0 can mean the first page, hence
+// do not use this API.
 FPDF_EXPORT unsigned long FPDF_CALLCONV
 FPDFDest_GetPageIndex(FPDF_DOCUMENT document, FPDF_DEST dest);
 
+// Get the page index of |dest|.
+//
+//   document - handle to the document.
+//   dest     - handle to the destination.
+//
+// Returns the -based page index containing |dest|. Returns -1 on error.
+FPDF_EXPORT long FPDF_CALLCONV FPDFDest_GetDestPageIndex(FPDF_DOCUMENT document,
+                                                         FPDF_DEST dest);
+
 // Get the view (fit type) specified by |dest|.
 // Experimental API. Subject to change.
 //