commit f2a636f5722bda7745b369124852efceace5513a
author Lei Zhang <thestig@chromium.org> Mon Apr 04 19:01:14 2022 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Apr 04 19:01:14 2022 +0000
tree 7b3f22c0c7cb96e7526c8e35f2bbb52feea02fa4
parent c289f1ef1bdcaee2424f636ab800982705f263f2

Change `CPDFSDK_PageView::m_SDKAnnotArray` to store std::unique_ptrs.

Instead of storing raw pointers and carefully managing them, store
std::unique_ptrs.

Change-Id: I1162eeee615af939f71f541452649f36a214b000
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/92113
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>

fpdfsdk/cpdfsdk_pageview.cpp

diff --git a/fpdfsdk/cpdfsdk_pageview.cpp b/fpdfsdk/cpdfsdk_pageview.cpp
index 00f73f0..08ce6a3 100644
--- a/fpdfsdk/cpdfsdk_pageview.cpp
+++ b/fpdfsdk/cpdfsdk_pageview.cpp
@@ -7,6 +7,7 @@
 #include "fpdfsdk/cpdfsdk_pageview.h"
 
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "core/fpdfapi/parser/cpdf_dictionary.h"
@@ -15,6 +16,7 @@
 #include "core/fpdfdoc/cpdf_annotlist.h"
 #include "core/fpdfdoc/cpdf_interactiveform.h"
 #include "core/fxcrt/autorestorer.h"
+#include "core/fxcrt/stl_util.h"
 #include "fpdfsdk/cpdfsdk_annot.h"
 #include "fpdfsdk/cpdfsdk_annotiteration.h"
 #include "fpdfsdk/cpdfsdk_annotiterator.h"
@@ -53,8 +55,9 @@
     m_page->AsPDFPage()->SetView(nullptr);
   }
 
-  for (CPDFSDK_Annot* pAnnot : m_SDKAnnotArray)
-    delete pAnnot;
+  // Manually reset elements to ensure they are deleted in order.
+  for (std::unique_ptr<CPDFSDK_Annot>& pAnnot : m_SDKAnnotArray)
+    pAnnot.reset();
 
   m_SDKAnnotArray.clear();
   m_pAnnotList.reset();
@@ -127,9 +130,7 @@
       pAnnotHandler->NewAnnotForXFA(pPDFAnnot, this);
   DCHECK(pNewAnnot);
   pSDKAnnot = pNewAnnot.get();
-  // TODO(thestig): See if |m_SDKAnnotArray|, which takes ownership of
-  // |pNewAnnot|, can hold std::unique_ptrs instead of raw pointers.
-  m_SDKAnnotArray.push_back(pNewAnnot.release());
+  m_SDKAnnotArray.push_back(std::move(pNewAnnot));
   return pSDKAnnot;
 }
 
@@ -151,11 +152,12 @@
     m_pFormFillEnv->KillFocusAnnot({});  // May invoke JS, invalidating pAnnot.
 
   if (pObserved) {
-    std::unique_ptr<CPDFSDK_Annot> to_be_deleted(pObserved.Get());
+    fxcrt::FakeUniquePtr<const CPDFSDK_Annot> fake_unique_annot(pAnnot);
+    auto it = std::find(m_SDKAnnotArray.begin(), m_SDKAnnotArray.end(),
+                        fake_unique_annot);
+    if (it != m_SDKAnnotArray.end())
+      m_SDKAnnotArray.erase(it);
   }
-  auto it = std::find(m_SDKAnnotArray.begin(), m_SDKAnnotArray.end(), pAnnot);
-  if (it != m_SDKAnnotArray.end())
-    m_SDKAnnotArray.erase(it);
 
   if (m_pCaptureWidget.Get() == pAnnot)
     m_pCaptureWidget.Reset();
@@ -179,11 +181,19 @@
   return m_pFormFillEnv->GetInteractiveForm();
 }
 
+std::vector<CPDFSDK_Annot*> CPDFSDK_PageView::GetAnnotList() const {
+  std::vector<CPDFSDK_Annot*> list;
+  list.reserve(m_SDKAnnotArray.size());
+  for (const std::unique_ptr<CPDFSDK_Annot>& elem : m_SDKAnnotArray)
+    list.push_back(elem.get());
+  return list;
+}
+
 CPDFSDK_Annot* CPDFSDK_PageView::GetAnnotByDict(CPDF_Dictionary* pDict) {
-  for (CPDFSDK_Annot* pAnnot : m_SDKAnnotArray) {
+  for (std::unique_ptr<CPDFSDK_Annot>& pAnnot : m_SDKAnnotArray) {
     CPDF_Annot* pPDFAnnot = pAnnot->GetPDFAnnot();
     if (pPDFAnnot && pPDFAnnot->GetAnnotDict() == pDict)
-      return pAnnot;
+      return pAnnot.get();
   }
   return nullptr;
 }
@@ -193,10 +203,10 @@
   if (!pWidget)
     return nullptr;
 
-  for (CPDFSDK_Annot* pAnnot : m_SDKAnnotArray) {
-    CPDFXFA_Widget* pCurrentWidget = ToXFAWidget(pAnnot);
+  for (std::unique_ptr<CPDFSDK_Annot>& pAnnot : m_SDKAnnotArray) {
+    CPDFXFA_Widget* pCurrentWidget = ToXFAWidget(pAnnot.get());
     if (pCurrentWidget && pCurrentWidget->GetXFAFFWidget() == pWidget)
-      return pAnnot;
+      return pAnnot.get();
   }
   return nullptr;
 }
@@ -596,7 +606,7 @@
           pAnnotHandlerMgr->NewAnnotForXFA(pXFAAnnot, this);
       DCHECK(pNewAnnot);
       CPDFSDK_Annot* pAnnot = pNewAnnot.get();
-      m_SDKAnnotArray.push_back(pNewAnnot.release());
+      m_SDKAnnotArray.push_back(std::move(pNewAnnot));
       pAnnotHandlerMgr->Annot_OnLoad(pAnnot);
     }
     return;
@@ -619,8 +629,8 @@
         pAnnotHandlerMgr->NewAnnot(pPDFAnnot, this);
     if (!pAnnot)
       continue;
-    m_SDKAnnotArray.push_back(pAnnot.release());
-    pAnnotHandlerMgr->Annot_OnLoad(m_SDKAnnotArray.back());
+    m_SDKAnnotArray.push_back(std::move(pAnnot));
+    pAnnotHandlerMgr->Annot_OnLoad(m_SDKAnnotArray.back().get());
   }
 }
 
@@ -660,7 +670,8 @@
 bool CPDFSDK_PageView::IsValidSDKAnnot(const CPDFSDK_Annot* p) const {
   if (!p)
     return false;
-  return pdfium::Contains(m_SDKAnnotArray, p);
+  fxcrt::FakeUniquePtr<const CPDFSDK_Annot> fake_unique_p(p);
+  return pdfium::Contains(m_SDKAnnotArray, fake_unique_p);
 }
 
 CPDFSDK_Annot* CPDFSDK_PageView::GetFocusAnnot() {

fpdfsdk/cpdfsdk_pageview.h

diff --git a/fpdfsdk/cpdfsdk_pageview.h b/fpdfsdk/cpdfsdk_pageview.h
index c10ee66..da1532c 100644
--- a/fpdfsdk/cpdfsdk_pageview.h
+++ b/fpdfsdk/cpdfsdk_pageview.h
@@ -49,9 +49,7 @@
   bool IsValidAnnot(const CPDF_Annot* p) const;
   bool IsValidSDKAnnot(const CPDFSDK_Annot* p) const;
 
-  const std::vector<CPDFSDK_Annot*>& GetAnnotList() const {
-    return m_SDKAnnotArray;
-  }
+  std::vector<CPDFSDK_Annot*> GetAnnotList() const;
   CPDFSDK_Annot* GetAnnotByDict(CPDF_Dictionary* pDict);
 
 #ifdef PDF_ENABLE_XFA
@@ -128,7 +126,7 @@
   CFX_Matrix m_curMatrix;
   UnownedPtr<IPDF_Page> const m_page;
   std::unique_ptr<CPDF_AnnotList> m_pAnnotList;
-  std::vector<CPDFSDK_Annot*> m_SDKAnnotArray;
+  std::vector<std::unique_ptr<CPDFSDK_Annot>> m_SDKAnnotArray;
   UnownedPtr<CPDFSDK_FormFillEnvironment> const m_pFormFillEnv;
   ObservedPtr<CPDFSDK_Annot> m_pCaptureWidget;
   RetainPtr<CPDF_Page> m_pOwnsPage;