From cedaa557316a3f5c436814e69d67f19795f471d7 Mon Sep 17 00:00:00 2001 From: dsinclair Date: Wed, 24 Aug 2016 11:12:19 -0700 Subject: Flip document and parser ownership This Cl switches the ownership between the parser and the document. Previously the parser owned the document and we'd jump through hoops during cleanup to delete the right object. This Cl flips the ownership so the document owns the parser and simplifies the cleanup logic where needed. BUG=pdfium:565 Review-Url: https://codereview.chromium.org/2275773003 --- core/fpdfapi/fpdf_parser/cpdf_document.cpp | 4 +- core/fpdfapi/fpdf_parser/cpdf_parser.cpp | 27 ++++++------- core/fpdfapi/fpdf_parser/include/cpdf_document.h | 6 +-- core/fpdfapi/fpdf_parser/include/cpdf_parser.h | 11 ++---- fpdfsdk/fpdf_dataavail.cpp | 13 +++--- fpdfsdk/fpdfdoc_unittest.cpp | 6 ++- fpdfsdk/fpdfview.cpp | 50 +++++++++--------------- fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp | 16 ++------ fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h | 10 +++-- fpdfsdk/pdfwindow/PWL_FontMap.cpp | 3 +- 10 files changed, 62 insertions(+), 84 deletions(-) diff --git a/core/fpdfapi/fpdf_parser/cpdf_document.cpp b/core/fpdfapi/fpdf_parser/cpdf_document.cpp index b11d0c1b79..89b41d5820 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_document.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_document.cpp @@ -483,9 +483,9 @@ int CountPages(CPDF_Dictionary* pPages, } // namespace -CPDF_Document::CPDF_Document(CPDF_Parser* pParser) +CPDF_Document::CPDF_Document(std::unique_ptr pParser) : CPDF_IndirectObjectHolder(), - m_pParser(pParser), + m_pParser(std::move(pParser)), m_pRootDict(nullptr), m_pInfoDict(nullptr), m_bLinearized(false), diff --git a/core/fpdfapi/fpdf_parser/cpdf_parser.cpp b/core/fpdfapi/fpdf_parser/cpdf_parser.cpp index ffd3f79a62..e2bab450be 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_parser.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_parser.cpp @@ -126,7 +126,7 @@ void CPDF_Parser::ShrinkObjectMap(uint32_t objnum) { void CPDF_Parser::CloseParser() { m_bVersionUpdated = false; - m_pDocument.reset(); + m_pDocument = nullptr; if (m_pTrailer) { m_pTrailer->Release(); @@ -158,9 +158,8 @@ void CPDF_Parser::CloseParser() { } } -CPDF_Parser::Error CPDF_Parser::StartParse( - IFX_FileRead* pFileAccess, - std::unique_ptr pDocument) { +CPDF_Parser::Error CPDF_Parser::StartParse(IFX_FileRead* pFileAccess, + CPDF_Document* pDocument) { CloseParser(); m_bXRefStream = FALSE; @@ -190,7 +189,7 @@ CPDF_Parser::Error CPDF_Parser::StartParse( return FORMAT_ERROR; m_pSyntax->RestorePos(m_pSyntax->m_FileLen - m_pSyntax->m_HeaderOffset - 9); - m_pDocument = std::move(pDocument); + m_pDocument = pDocument; FX_BOOL bXRefRebuilt = FALSE; if (m_pSyntax->SearchWord("startxref", TRUE, FALSE, 4096)) { @@ -765,7 +764,7 @@ FX_BOOL CPDF_Parser::RebuildCrossRef() { last_obj = start_pos; FX_FILESIZE obj_end = 0; CPDF_Object* pObject = ParseIndirectObjectAtByStrict( - m_pDocument.get(), obj_pos, objnum, &obj_end); + m_pDocument, obj_pos, objnum, &obj_end); if (CPDF_Stream* pStream = ToStream(pObject)) { if (CPDF_Dictionary* pDict = pStream->GetDict()) { if ((pDict->KeyExist("Type")) && @@ -828,8 +827,7 @@ FX_BOOL CPDF_Parser::RebuildCrossRef() { last_trailer = pos + i - 7; m_pSyntax->RestorePos(pos + i - m_pSyntax->m_HeaderOffset); - CPDF_Object* pObj = - m_pSyntax->GetObject(m_pDocument.get(), 0, 0, true); + CPDF_Object* pObj = m_pSyntax->GetObject(m_pDocument, 0, 0, true); if (pObj) { if (!pObj->IsDictionary() && !pObj->AsStream()) { pObj->Release(); @@ -851,7 +849,7 @@ FX_BOOL CPDF_Parser::RebuildCrossRef() { uint32_t dwObjNum = pElement ? pElement->GetObjNum() : 0; if (dwObjNum) { - m_pTrailer->SetAtReference(key, m_pDocument.get(), + m_pTrailer->SetAtReference(key, m_pDocument, dwObjNum); } else { m_pTrailer->SetAt(key, pElement->Clone()); @@ -975,7 +973,7 @@ FX_BOOL CPDF_Parser::RebuildCrossRef() { } FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) { - CPDF_Object* pObject = ParseIndirectObjectAt(m_pDocument.get(), *pos, 0); + CPDF_Object* pObject = ParseIndirectObjectAt(m_pDocument, *pos, 0); if (!pObject) return FALSE; @@ -1478,7 +1476,7 @@ CPDF_Dictionary* CPDF_Parser::LoadTrailerV4() { return nullptr; std::unique_ptr> pObj( - m_pSyntax->GetObject(m_pDocument.get(), 0, 0, true)); + m_pSyntax->GetObject(m_pDocument, 0, 0, true)); if (!ToDictionary(pObj.get())) return nullptr; return pObj.release()->AsDictionary(); @@ -1550,9 +1548,8 @@ FX_BOOL CPDF_Parser::IsLinearizedFile(IFX_FileRead* pFileAccess, return FALSE; } -CPDF_Parser::Error CPDF_Parser::StartLinearizedParse( - IFX_FileRead* pFileAccess, - std::unique_ptr pDocument) { +CPDF_Parser::Error CPDF_Parser::StartLinearizedParse(IFX_FileRead* pFileAccess, + CPDF_Document* pDocument) { CloseParser(); m_bXRefStream = FALSE; m_LastXRefOffset = 0; @@ -1567,7 +1564,7 @@ CPDF_Parser::Error CPDF_Parser::StartLinearizedParse( return StartParse(pFileAccess, std::move(pDocument)); } - m_pDocument = std::move(pDocument); + m_pDocument = pDocument; FX_FILESIZE dwFirstXRefOffset = m_pSyntax->SavePos(); FX_BOOL bXRefRebuilt = FALSE; diff --git a/core/fpdfapi/fpdf_parser/include/cpdf_document.h b/core/fpdfapi/fpdf_parser/include/cpdf_document.h index 31988d8c9d..f3322425ed 100644 --- a/core/fpdfapi/fpdf_parser/include/cpdf_document.h +++ b/core/fpdfapi/fpdf_parser/include/cpdf_document.h @@ -40,10 +40,10 @@ class JBig2_DocumentContext; class CPDF_Document : public CPDF_IndirectObjectHolder { public: - explicit CPDF_Document(CPDF_Parser* pParser); + explicit CPDF_Document(std::unique_ptr pParser); ~CPDF_Document() override; - CPDF_Parser* GetParser() const { return m_pParser; } + CPDF_Parser* GetParser() const { return m_pParser.get(); } CPDF_Dictionary* GetRoot() const { return m_pRootDict; } CPDF_Dictionary* GetInfo() const { return m_pInfoDict; } @@ -128,7 +128,7 @@ class CPDF_Document : public CPDF_IndirectObjectHolder { FX_BOOL CheckOCGVisible(CPDF_Dictionary* pOCG, FX_BOOL bPrinting); CPDF_Object* ParseIndirectObject(uint32_t objnum) override; - CPDF_Parser* m_pParser; + std::unique_ptr m_pParser; CPDF_Dictionary* m_pRootDict; CPDF_Dictionary* m_pInfoDict; CFX_ByteString m_ID1; diff --git a/core/fpdfapi/fpdf_parser/include/cpdf_parser.h b/core/fpdfapi/fpdf_parser/include/cpdf_parser.h index a69f0fe38e..d6a5d5703b 100644 --- a/core/fpdfapi/fpdf_parser/include/cpdf_parser.h +++ b/core/fpdfapi/fpdf_parser/include/cpdf_parser.h @@ -37,17 +37,14 @@ class CPDF_Parser { CPDF_Parser(); ~CPDF_Parser(); - Error StartParse(IFX_FileRead* pFile, - std::unique_ptr pDocument); - - Error StartLinearizedParse(IFX_FileRead* pFile, - std::unique_ptr pDocument); + Error StartParse(IFX_FileRead* pFile, CPDF_Document* pDocument); + Error StartLinearizedParse(IFX_FileRead* pFile, CPDF_Document* pDocument); void SetPassword(const FX_CHAR* password) { m_Password = password; } CFX_ByteString GetPassword() { return m_Password; } CPDF_Dictionary* GetTrailer() const { return m_pTrailer; } FX_FILESIZE GetLastXRefOffset() const { return m_LastXRefOffset; } - CPDF_Document* GetDocument() const { return m_pDocument.get(); } + CPDF_Document* GetDocument() const { return m_pDocument; } uint32_t GetPermissions() const; uint32_t GetRootObjNum(); @@ -120,7 +117,7 @@ class CPDF_Parser { // the objects. bool VerifyCrossRefV4(); - std::unique_ptr m_pDocument; + CPDF_Document* m_pDocument; // not owned std::unique_ptr m_pSyntax; bool m_bOwnFileRead; int m_FileVersion; diff --git a/fpdfsdk/fpdf_dataavail.cpp b/fpdfsdk/fpdf_dataavail.cpp index 5dd42402ed..7b9ba32fb0 100644 --- a/fpdfsdk/fpdf_dataavail.cpp +++ b/fpdfsdk/fpdf_dataavail.cpp @@ -139,16 +139,17 @@ FPDFAvail_GetDocument(FPDF_AVAIL avail, FPDF_BYTESTRING password) { std::unique_ptr pParser(new CPDF_Parser); pParser->SetPassword(password); - std::unique_ptr pDocument(new CPDF_Document(pParser.get())); - CPDF_Parser::Error error = pParser->StartLinearizedParse( - pDataAvail->m_pDataAvail->GetFileRead(), std::move(pDocument)); + std::unique_ptr pDocument( + new CPDF_Document(std::move(pParser))); + CPDF_Parser::Error error = pDocument->GetParser()->StartLinearizedParse( + pDataAvail->m_pDataAvail->GetFileRead(), pDocument.get()); if (error != CPDF_Parser::SUCCESS) { ProcessParseError(error); return nullptr; } - pDataAvail->m_pDataAvail->SetDocument(pParser->GetDocument()); - CheckUnSupportError(pParser->GetDocument(), FPDF_ERR_SUCCESS); - return FPDFDocumentFromCPDFDocument(pParser.release()->GetDocument()); + pDataAvail->m_pDataAvail->SetDocument(pDocument.get()); + CheckUnSupportError(pDocument.get(), FPDF_ERR_SUCCESS); + return FPDFDocumentFromCPDFDocument(pDocument.release()); } DLLEXPORT int STDCALL FPDFAvail_GetFirstPageNum(FPDF_DOCUMENT doc) { diff --git a/fpdfsdk/fpdfdoc_unittest.cpp b/fpdfsdk/fpdfdoc_unittest.cpp index 896c7d0c72..a555e961f3 100644 --- a/fpdfsdk/fpdfdoc_unittest.cpp +++ b/fpdfsdk/fpdfdoc_unittest.cpp @@ -10,6 +10,7 @@ #include "core/fpdfapi/fpdf_parser/include/cpdf_document.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_name.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_number.h" +#include "core/fpdfapi/fpdf_parser/include/cpdf_parser.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_reference.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_string.h" #include "core/fpdfapi/include/cpdf_modulemgr.h" @@ -23,7 +24,7 @@ class CPDF_TestDocument : public CPDF_Document { public: - CPDF_TestDocument() : CPDF_Document(nullptr) {} + CPDF_TestDocument() : CPDF_Document(std::unique_ptr()) {} void SetRoot(CPDF_Dictionary* root) { m_pRootDict = root; } CPDF_IndirectObjectHolder* GetHolder() { return this; } @@ -33,7 +34,8 @@ class CPDF_TestDocument : public CPDF_Document { class CPDF_TestXFADocument : public CPDFXFA_Document { public: CPDF_TestXFADocument() - : CPDFXFA_Document(new CPDF_TestDocument(), CPDFXFA_App::GetInstance()) {} + : CPDFXFA_Document(WrapUnique(new CPDF_TestDocument()), + CPDFXFA_App::GetInstance()) {} void SetRoot(CPDF_Dictionary* root) { reinterpret_cast(GetPDFDoc())->SetRoot(root); diff --git a/fpdfsdk/fpdfview.cpp b/fpdfsdk/fpdfview.cpp index 29fe88d943..0c3a95d094 100644 --- a/fpdfsdk/fpdfview.cpp +++ b/fpdfsdk/fpdfview.cpp @@ -66,8 +66,8 @@ CPDF_Document* CPDFDocumentFromFPDFDocument(FPDF_DOCUMENT doc) { FPDF_DOCUMENT FPDFDocumentFromCPDFDocument(CPDF_Document* doc) { #ifdef PDF_ENABLE_XFA - return doc ? FPDFDocumentFromUnderlying( - new CPDFXFA_Document(doc, CPDFXFA_App::GetInstance())) + return doc ? FPDFDocumentFromUnderlying(new CPDFXFA_Document( + WrapUnique(doc), CPDFXFA_App::GetInstance())) : nullptr; #else // PDF_ENABLE_XFA return FPDFDocumentFromUnderlying(doc); @@ -365,22 +365,17 @@ DLLEXPORT FPDF_DOCUMENT STDCALL FPDF_LoadDocument(FPDF_STRING file_path, std::unique_ptr pParser(new CPDF_Parser); pParser->SetPassword(password); - std::unique_ptr pDocument(new CPDF_Document(pParser.get())); - CPDF_Parser::Error error = - pParser->StartParse(pFileAccess, std::move(pDocument)); + std::unique_ptr pDocument( + new CPDF_Document(std::move(pParser))); + CPDF_Parser::Error error = pParser->StartParse(pFileAccess, pDocument.get()); if (error != CPDF_Parser::SUCCESS) { ProcessParseError(error); return nullptr; } #ifdef PDF_ENABLE_XFA - CPDF_Document* pPDFDoc = pParser.release()->GetDocument(); - if (!pPDFDoc) - return nullptr; - - CPDFXFA_App* pProvider = CPDFXFA_App::GetInstance(); - return new CPDFXFA_Document(pPDFDoc, pProvider); + return new CPDFXFA_Document(std::move(pDocument), CPDFXFA_App::GetInstance()); #else // PDF_ENABLE_XFA - return pParser.release()->GetDocument(); + return pDocument.release(); #endif // PDF_ENABLE_XFA } @@ -451,15 +446,16 @@ DLLEXPORT FPDF_DOCUMENT STDCALL FPDF_LoadMemDocument(const void* data_buf, std::unique_ptr pParser(new CPDF_Parser); pParser->SetPassword(password); - std::unique_ptr pDocument(new CPDF_Document(pParser.get())); + std::unique_ptr pDocument( + new CPDF_Document(std::move(pParser))); CPDF_Parser::Error error = - pParser->StartParse(pMemFile, std::move(pDocument)); + pDocument->GetParser()->StartParse(pMemFile, pDocument.get()); if (error != CPDF_Parser::SUCCESS) { ProcessParseError(error); return nullptr; } - CheckUnSupportError(pParser->GetDocument(), error); - return FPDFDocumentFromCPDFDocument(pParser.release()->GetDocument()); + CheckUnSupportError(pDocument.get(), error); + return FPDFDocumentFromCPDFDocument(pDocument.release()); } DLLEXPORT FPDF_DOCUMENT STDCALL @@ -469,14 +465,16 @@ FPDF_LoadCustomDocument(FPDF_FILEACCESS* pFileAccess, std::unique_ptr pParser(new CPDF_Parser); pParser->SetPassword(password); - std::unique_ptr pDocument(new CPDF_Document(pParser.get())); - CPDF_Parser::Error error = pParser->StartParse(pFile, std::move(pDocument)); + std::unique_ptr pDocument( + new CPDF_Document(std::move(pParser))); + CPDF_Parser::Error error = + pDocument->GetParser()->StartParse(pFile, pDocument.get()); if (error != CPDF_Parser::SUCCESS) { ProcessParseError(error); return nullptr; } - CheckUnSupportError(pParser->GetDocument(), error); - return FPDFDocumentFromCPDFDocument(pParser.release()->GetDocument()); + CheckUnSupportError(pDocument.get(), error); + return FPDFDocumentFromCPDFDocument(pDocument.release()); } DLLEXPORT FPDF_BOOL STDCALL FPDF_GetFileVersion(FPDF_DOCUMENT doc, @@ -683,19 +681,7 @@ DLLEXPORT void STDCALL FPDF_ClosePage(FPDF_PAGE page) { } DLLEXPORT void STDCALL FPDF_CloseDocument(FPDF_DOCUMENT document) { -#ifdef PDF_ENABLE_XFA delete UnderlyingFromFPDFDocument(document); -#else // PDF_ENABLE_XFA - CPDF_Document* pDoc = CPDFDocumentFromFPDFDocument(document); - if (!pDoc) - return; - CPDF_Parser* pParser = pDoc->GetParser(); - if (!pParser) { - delete pDoc; - return; - } - delete pParser; -#endif // PDF_ENABLE_XFA } DLLEXPORT unsigned long STDCALL FPDF_GetLastError() { diff --git a/fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp b/fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp index 605e53c32a..d4c43c5706 100644 --- a/fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp +++ b/fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp @@ -45,10 +45,10 @@ extern void SetLastError(int err); extern int GetLastError(); #endif -CPDFXFA_Document::CPDFXFA_Document(CPDF_Document* pPDFDoc, +CPDFXFA_Document::CPDFXFA_Document(std::unique_ptr pPDFDoc, CPDFXFA_App* pProvider) : m_iDocType(DOCTYPE_PDF), - m_pPDFDoc(pPDFDoc), + m_pPDFDoc(std::move(pPDFDoc)), m_pXFADocView(nullptr), m_pApp(pProvider), m_pJSContext(nullptr), @@ -70,16 +70,6 @@ CPDFXFA_Document::~CPDFXFA_Document() { } if (m_pJSContext && m_pSDKDoc && m_pSDKDoc->GetEnv()) m_pSDKDoc->GetEnv()->GetJSRuntime()->ReleaseContext(m_pJSContext); - // |m_pSDKDoc| has to be released before |pParser| and |m_pPDFDoc| since it - // needs to access them to kill focused annotations. - m_pSDKDoc.reset(); - if (m_pPDFDoc) { - CPDF_Parser* pParser = m_pPDFDoc->GetParser(); - if (pParser) - delete pParser; - else - delete m_pPDFDoc; - } m_nLoadStatus = FXFA_LOADSTATUS_CLOSED; } @@ -96,7 +86,7 @@ FX_BOOL CPDFXFA_Document::LoadXFADoc() { if (!pApp) return FALSE; - m_pXFADoc.reset(pApp->CreateDoc(this, m_pPDFDoc)); + m_pXFADoc.reset(pApp->CreateDoc(this, m_pPDFDoc.get())); if (!m_pXFADoc) { SetLastError(FPDF_ERR_XFALOAD); return FALSE; diff --git a/fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h b/fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h index ed4c8727fc..c83c770f97 100644 --- a/fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h +++ b/fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h @@ -27,12 +27,13 @@ class CXFA_FFDocHandler; class CPDFXFA_Document : public IXFA_DocProvider { public: - CPDFXFA_Document(CPDF_Document* pPDFDoc, CPDFXFA_App* pProvider); + CPDFXFA_Document(std::unique_ptr pPDFDoc, + CPDFXFA_App* pProvider); ~CPDFXFA_Document() override; FX_BOOL LoadXFADoc(); CPDFXFA_App* GetApp() { return m_pApp; } - CPDF_Document* GetPDFDoc() { return m_pPDFDoc; } + CPDF_Document* GetPDFDoc() { return m_pPDFDoc.get(); } CXFA_FFDoc* GetXFADoc() { return m_pXFADoc.get(); } CXFA_FFDocView* GetXFADocView() { return m_pXFADocView; } @@ -200,7 +201,10 @@ class CPDFXFA_Document : public IXFA_DocProvider { } int m_iDocType; - CPDF_Document* m_pPDFDoc; + + // |m_pSDKDoc| has to be released before |m_pPDFDoc| since it needs to access + // it to kill focused annotations. + std::unique_ptr m_pPDFDoc; std::unique_ptr m_pSDKDoc; std::unique_ptr m_pXFADoc; CXFA_FFDocView* m_pXFADocView; // not owned. diff --git a/fpdfsdk/pdfwindow/PWL_FontMap.cpp b/fpdfsdk/pdfwindow/PWL_FontMap.cpp index 96acb78ec2..131334ac22 100644 --- a/fpdfsdk/pdfwindow/PWL_FontMap.cpp +++ b/fpdfsdk/pdfwindow/PWL_FontMap.cpp @@ -9,6 +9,7 @@ #include "core/fpdfapi/fpdf_font/include/cpdf_font.h" #include "core/fpdfapi/fpdf_font/include/cpdf_fontencoding.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_document.h" +#include "core/fpdfapi/fpdf_parser/include/cpdf_parser.h" #include "core/fpdfapi/include/cpdf_modulemgr.h" #include "core/fpdfdoc/include/ipvt_fontmap.h" #include "fpdfsdk/pdfwindow/PWL_Wnd.h" @@ -50,7 +51,7 @@ void CPWL_FontMap::SetSystemHandler(CFX_SystemHandler* pSystemHandler) { CPDF_Document* CPWL_FontMap::GetDocument() { if (!m_pPDFDoc) { if (CPDF_ModuleMgr::Get()) { - m_pPDFDoc.reset(new CPDF_Document(nullptr)); + m_pPDFDoc.reset(new CPDF_Document(std::unique_ptr())); m_pPDFDoc->CreateNewDoc(); } } -- cgit v1.2.3