From 2116105b7d0545eb353264d4b42420cf51af5195 Mon Sep 17 00:00:00 2001 From: dsinclair Date: Wed, 5 Oct 2016 15:46:15 -0700 Subject: Remove ownership of CPDFSDK_Document from CPDFXFA_Document This CL updates CPDFXFA_Document so it never owns the CPDFSDK_Document. The CPDFSDK_Document is now always owned by the CPDFXFA_Environment. This also cleans up the strange need to reverse the order of document and form destruction when using XFA. Review-Url: https://codereview.chromium.org/2397473006 --- fpdfsdk/cpdfsdk_document.cpp | 11 +++++++---- fpdfsdk/cpdfsdk_document.h | 1 + fpdfsdk/cpdfsdk_environment.h | 8 ++------ fpdfsdk/fpdfformfill.cpp | 11 ++++++----- fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp | 6 +++++- fpdfsdk/fpdfxfa/cpdfxfa_document.cpp | 10 ++++++---- fpdfsdk/fpdfxfa/cpdfxfa_document.h | 8 +++----- samples/pdfium_test.cc | 19 ++++--------------- 8 files changed, 34 insertions(+), 40 deletions(-) diff --git a/fpdfsdk/cpdfsdk_document.cpp b/fpdfsdk/cpdfsdk_document.cpp index 544c1e6e19..a0febafd48 100644 --- a/fpdfsdk/cpdfsdk_document.cpp +++ b/fpdfsdk/cpdfsdk_document.cpp @@ -39,14 +39,17 @@ CPDFSDK_Document::CPDFSDK_Document(UnderlyingDocumentType* pDoc, CPDFSDK_Document::~CPDFSDK_Document() { m_bBeingDestroyed = TRUE; + ClearAllFocusedAnnots(); + for (auto& it : m_pageMap) + delete it.second; + m_pageMap.clear(); +} + +void CPDFSDK_Document::ClearAllFocusedAnnots() { for (auto& it : m_pageMap) { if (it.second->IsValidSDKAnnot(GetFocusAnnot())) KillFocusAnnot(0); } - - for (auto& it : m_pageMap) - delete it.second; - m_pageMap.clear(); } CPDFSDK_PageView* CPDFSDK_Document::GetPageView( diff --git a/fpdfsdk/cpdfsdk_document.h b/fpdfsdk/cpdfsdk_document.h index 697ee48f9d..561bfc198d 100644 --- a/fpdfsdk/cpdfsdk_document.h +++ b/fpdfsdk/cpdfsdk_document.h @@ -69,6 +69,7 @@ class CPDFSDK_Document : public CFX_Observable { CPDFSDK_Annot* GetFocusAnnot() { return m_pFocusAnnot.Get(); } FX_BOOL SetFocusAnnot(CPDFSDK_Annot::ObservedPtr* pAnnot); FX_BOOL KillFocusAnnot(uint32_t nFlag); + void ClearAllFocusedAnnots(); FX_BOOL ExtractPages(const std::vector& arrExtraPages, CPDF_Document* pDstDoc); diff --git a/fpdfsdk/cpdfsdk_environment.h b/fpdfsdk/cpdfsdk_environment.h index 9943d38c67..bed757c2ba 100644 --- a/fpdfsdk/cpdfsdk_environment.h +++ b/fpdfsdk/cpdfsdk_environment.h @@ -146,8 +146,7 @@ class CPDFSDK_Environment final { void JS_docgotoPage(int nPageNum); FX_BOOL IsJSInitiated() const { return m_pInfo && m_pInfo->m_pJsPlatform; } - void SetSDKDocument(CPDFSDK_Document* pFXDoc) { m_pSDKDoc = pFXDoc; } - CPDFSDK_Document* GetSDKDocument() const { return m_pSDKDoc; } + CPDFSDK_Document* GetSDKDocument() const { return m_pSDKDoc.get(); } UnderlyingDocumentType* GetUnderlyingDocument() const { return m_pUnderlyingDoc; } @@ -166,10 +165,7 @@ class CPDFSDK_Environment final { std::unique_ptr m_pActionHandler; std::unique_ptr m_pJSRuntime; FPDF_FORMFILLINFO* const m_pInfo; - // Ownership of |m_pSDKDoc| depends on if this is XFA. If we're in XFA then - // the object is owned by the CPDFXFA_Document. In non-xfa then we own - // the pointer. - CPDFSDK_Document* m_pSDKDoc; + std::unique_ptr m_pSDKDoc; UnderlyingDocumentType* const m_pUnderlyingDoc; std::unique_ptr m_pFormFiller; std::unique_ptr m_pSysHandler; diff --git a/fpdfsdk/fpdfformfill.cpp b/fpdfsdk/fpdfformfill.cpp index 6fea1af42b..904a3c867e 100644 --- a/fpdfsdk/fpdfformfill.cpp +++ b/fpdfsdk/fpdfformfill.cpp @@ -254,8 +254,7 @@ FPDFDOC_InitFormFillEnvironment(FPDF_DOCUMENT document, CPDFSDK_Environment* pEnv = new CPDFSDK_Environment(pDocument, formInfo); #ifdef PDF_ENABLE_XFA - // Ownership of the SDKDocument is passed to the CPDFXFA_Document. - pDocument->SetSDKDoc(pdfium::WrapUnique(pEnv->GetSDKDocument())); + pDocument->SetSDKDoc(pEnv->GetSDKDocument()); CPDFXFA_App::GetInstance()->AddFormFillEnv(pEnv); #endif // PDF_ENABLE_XFA @@ -271,10 +270,12 @@ FPDFDOC_ExitFormFillEnvironment(FPDF_FORMHANDLE hHandle) { #ifdef PDF_ENABLE_XFA CPDFXFA_App::GetInstance()->RemoveFormFillEnv(pEnv); -#else // PDF_ENABLE_XFA + + // Reset the focused annotations and remove the SDK document from the + // XFA document. if (CPDFSDK_Document* pSDKDoc = pEnv->GetSDKDocument()) { - pEnv->SetSDKDocument(nullptr); - delete pSDKDoc; + pSDKDoc->ClearAllFocusedAnnots(); + pSDKDoc->GetXFADocument()->SetSDKDoc(nullptr); } #endif // PDF_ENABLE_XFA diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp b/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp index 1b27e16551..fed5409624 100644 --- a/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp +++ b/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp @@ -258,7 +258,11 @@ FX_BOOL CPDFXFA_DocEnvironment::PopupMenu(CXFA_FFWidget* hWidget, void CPDFXFA_DocEnvironment::PageViewEvent(CXFA_FFPageView* pPageView, uint32_t dwFlags) { - CPDFSDK_Environment* pEnv = m_pDocument->GetSDKDoc()->GetEnv(); + CPDFSDK_Document* pSDKDoc = m_pDocument->GetSDKDoc(); + if (!pSDKDoc) + return; + + CPDFSDK_Environment* pEnv = pSDKDoc->GetEnv(); if (!pEnv) return; diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp b/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp index e49cfde8c4..121d060453 100644 --- a/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp +++ b/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp @@ -33,6 +33,7 @@ CPDFXFA_Document::CPDFXFA_Document(std::unique_ptr pPDFDoc, CPDFXFA_App* pProvider) : m_iDocType(DOCTYPE_PDF), m_pPDFDoc(std::move(pPDFDoc)), + m_pSDKDoc(nullptr), m_pXFADocView(nullptr), m_pApp(pProvider), m_nLoadStatus(FXFA_LOADSTATUS_PRELOAD), @@ -42,6 +43,11 @@ CPDFXFA_Document::CPDFXFA_Document(std::unique_ptr pPDFDoc, CPDFXFA_Document::~CPDFXFA_Document() { m_nLoadStatus = FXFA_LOADSTATUS_CLOSING; + if (m_pSDKDoc) { + m_pSDKDoc->ClearAllFocusedAnnots(); + m_pSDKDoc = nullptr; + } + if (m_pXFADoc) { CXFA_FFApp* pApp = m_pApp->GetXFAApp(); if (pApp) { @@ -191,10 +197,6 @@ void CPDFXFA_Document::RemovePage(CPDFXFA_Page* page) { m_XFAPageList.SetAt(page->GetPageIndex(), nullptr); } -void CPDFXFA_Document::SetSDKDoc(std::unique_ptr pSDKDoc) { - m_pSDKDoc.reset(pSDKDoc.release()); -} - void CPDFXFA_Document::ClearChangeMark() { if (m_pSDKDoc) m_pSDKDoc->ClearChangeMark(); diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_document.h b/fpdfsdk/fpdfxfa/cpdfxfa_document.h index f67a880a36..fa415f917c 100644 --- a/fpdfsdk/fpdfxfa/cpdfxfa_document.h +++ b/fpdfsdk/fpdfxfa/cpdfxfa_document.h @@ -40,8 +40,8 @@ class CPDFXFA_Document { CXFA_FFDocView* GetXFADocView() { return m_pXFADocView; } int GetDocType() const { return m_iDocType; } - CPDFSDK_Document* GetSDKDoc() const { return m_pSDKDoc.get(); } - void SetSDKDoc(std::unique_ptr pSDKDoc); + CPDFSDK_Document* GetSDKDoc() const { return m_pSDKDoc; } + void SetSDKDoc(CPDFSDK_Document* pSDKDoc) { m_pSDKDoc = pSDKDoc; } void DeletePage(int page_index); int GetPageCount() const; @@ -78,10 +78,8 @@ class CPDFXFA_Document { int m_iDocType; std::unique_ptr m_pPDFDoc; - // |m_pSDKDoc| must be destroyed before |m_pPDFDoc| since it needs to access - // it to kill focused annotations. - std::unique_ptr m_pSDKDoc; std::unique_ptr m_pXFADoc; + CPDFSDK_Document* m_pSDKDoc; // not owned. CXFA_FFDocView* m_pXFADocView; // not owned. CPDFXFA_App* const m_pApp; CFX_ArrayTemplate m_XFAPageList; diff --git a/samples/pdfium_test.cc b/samples/pdfium_test.cc index 35e0d50678..5c0f81b1b0 100644 --- a/samples/pdfium_test.cc +++ b/samples/pdfium_test.cc @@ -86,19 +86,6 @@ static FPDF_FORMFILLINFO_PDFiumTest* ToPDFiumTestFormFillInfo( return static_cast(formFillInfo); } -static void CloseDocAndForm(FPDF_DOCUMENT doc, FPDF_FORMHANDLE form) { -#ifdef PDF_ENABLE_XFA - // Note: The shut down order here is the reverse of the non-XFA branch order. - // Need to work out if this is required, and if it is, the lifetimes of - // objects owned by |doc| that |form| reference. - FPDF_CloseDocument(doc); - FPDFDOC_ExitFormFillEnvironment(form); -#else // PDF_ENABLE_XFA - FPDFDOC_ExitFormFillEnvironment(form); - FPDF_CloseDocument(doc); -#endif // PDF_ENABLE_XFA -} - static bool CheckDimensions(int stride, int width, int height) { if (stride < 0 || width < 0 || height < 0) return false; @@ -791,7 +778,8 @@ void RenderPdf(const std::string& name, if (nRet == PDF_DATA_ERROR) { fprintf(stderr, "Unknown error in checking if page %d is available.\n", i); - CloseDocAndForm(doc, form); + FPDFDOC_ExitFormFillEnvironment(form); + FPDF_CloseDocument(doc); return; } } @@ -803,7 +791,8 @@ void RenderPdf(const std::string& name, FORM_DoDocumentAAction(form, FPDFDOC_AACTION_WC); - CloseDocAndForm(doc, form); + FPDFDOC_ExitFormFillEnvironment(form); + FPDF_CloseDocument(doc); fprintf(stderr, "Rendered %d pages.\n", rendered_pages); if (bad_pages) -- cgit v1.2.3