From 80bf582f00adeefae67ae432b74c3b0609e7845e Mon Sep 17 00:00:00 2001 From: Dan Sinclair Date: Tue, 13 Feb 2018 20:55:03 +0000 Subject: Cleanup CXFA_Node ownership Currently a CXFA_Node can be owned by the CXFA_Document root pointer, the CXFA_Document Purge list or as a child of another CXFA_Node. This makes it hard to know who currently owns what. This CL moves all ownership of nodes into a CXFA_NodeOwner class which CXFA_Document subclasses. The node owner always owns all nodes and is responsible for cleaning them up upon destruction. The destruction order is not guarenteed to be in tree order as nodes can be inserted and moved around the tree. Change-Id: I6b202b8e844999ba835093dcdd1a808938b6d9a8 Reviewed-on: https://pdfium-review.googlesource.com/26434 Reviewed-by: Tom Sepez Commit-Queue: dsinclair --- BUILD.gn | 2 ++ fxjs/xfa/cjx_object.cpp | 6 +++--- xfa/fxfa/parser/cxfa_document.cpp | 36 +++++------------------------------- xfa/fxfa/parser/cxfa_document.h | 12 ++++-------- xfa/fxfa/parser/cxfa_node.cpp | 17 +---------------- xfa/fxfa/parser/cxfa_node.h | 2 +- xfa/fxfa/parser/cxfa_nodeowner.cpp | 35 +++++++++++++++++++++++++++++++++++ xfa/fxfa/parser/cxfa_nodeowner.h | 28 ++++++++++++++++++++++++++++ 8 files changed, 79 insertions(+), 59 deletions(-) create mode 100644 xfa/fxfa/parser/cxfa_nodeowner.cpp create mode 100644 xfa/fxfa/parser/cxfa_nodeowner.h diff --git a/BUILD.gn b/BUILD.gn index 6681ae82cc..bdadebd400 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -2521,6 +2521,8 @@ if (pdf_enable_xfa) { "xfa/fxfa/parser/cxfa_nodeiteratortemplate.h", "xfa/fxfa/parser/cxfa_nodelocale.cpp", "xfa/fxfa/parser/cxfa_nodelocale.h", + "xfa/fxfa/parser/cxfa_nodeowner.cpp", + "xfa/fxfa/parser/cxfa_nodeowner.h", "xfa/fxfa/parser/cxfa_numberofcopies.cpp", "xfa/fxfa/parser/cxfa_numberofcopies.h", "xfa/fxfa/parser/cxfa_numberpattern.cpp", diff --git a/fxjs/xfa/cjx_object.cpp b/fxjs/xfa/cjx_object.cpp index d663fcce70..50643ffc5b 100644 --- a/fxjs/xfa/cjx_object.cpp +++ b/fxjs/xfa/cjx_object.cpp @@ -1261,7 +1261,7 @@ void CJX_Object::Script_Attribute_String(CFXJSE_Value* pValue, pHeadChild = pSibling; } - std::unique_ptr pProtoForm(pProtoNode->CloneTemplateToForm(true)); + CXFA_Node* pProtoForm = pProtoNode->CloneTemplateToForm(true); pHeadChild = pProtoForm->GetFirstChild(); while (pHeadChild) { CXFA_Node* pSibling = pHeadChild->GetNextSibling(); @@ -1269,8 +1269,8 @@ void CJX_Object::Script_Attribute_String(CFXJSE_Value* pValue, ToNode(GetXFAObject())->InsertChild(pHeadChild, nullptr); pHeadChild = pSibling; } - - GetDocument()->RemovePurgeNode(pProtoForm.get()); + GetDocument()->FreeOwnedNode(pProtoForm); + pProtoForm = nullptr; } void CJX_Object::Script_Attribute_BOOL(CFXJSE_Value* pValue, diff --git a/xfa/fxfa/parser/cxfa_document.cpp b/xfa/fxfa/parser/cxfa_document.cpp index 434d6cbfde..a21d3f974c 100644 --- a/xfa/fxfa/parser/cxfa_document.cpp +++ b/xfa/fxfa/parser/cxfa_document.cpp @@ -6,6 +6,8 @@ #include "xfa/fxfa/parser/cxfa_document.h" +#include + #include "core/fxcrt/fx_extension.h" #include "fxjs/cfxjse_engine.h" #include "xfa/fxfa/cxfa_ffnotify.h" @@ -87,7 +89,8 @@ void MergeNode(CXFA_Document* pDocument, } // namespace CXFA_Document::CXFA_Document(CXFA_DocumentParser* pParser) - : m_pParser(pParser), + : CXFA_NodeOwner(), + m_pParser(pParser), m_pRootNode(nullptr), m_eCurVersionMode(XFA_VERSION_DEFAULT), m_dwDocFlags(0) { @@ -98,12 +101,6 @@ CXFA_Document::~CXFA_Document() { // Remove all the bindings before freeing the node as the ownership is wonky. if (m_pRootNode) m_pRootNode->ReleaseBindingNodes(); - - delete m_pRootNode; - - for (CXFA_Node* pNode : m_PurgeNodes) - delete pNode; - m_PurgeNodes.clear(); } CXFA_LayoutProcessor* CXFA_Document::GetLayoutProcessor() { @@ -128,14 +125,6 @@ void CXFA_Document::ClearLayoutData() { m_pScriptSignature.reset(); } -void CXFA_Document::SetRoot(CXFA_Node* pNewRoot) { - if (m_pRootNode) - AddPurgeNode(m_pRootNode); - - m_pRootNode = pNewRoot; - RemovePurgeNode(pNewRoot); -} - CFX_XMLDoc* CXFA_Document::GetXMLDoc() const { return m_pParser->GetXMLDoc(); } @@ -221,22 +210,7 @@ CXFA_Node* CXFA_Document::CreateNode(XFA_PacketType packet, XFA_Element eElement) { if (eElement == XFA_Element::Unknown) return nullptr; - - std::unique_ptr pNode = CXFA_Node::Create(this, eElement, packet); - if (!pNode) - return nullptr; - - // TODO(dsinclair): AddPrugeNode should take ownership of the pointer. - AddPurgeNode(pNode.get()); - return pNode.release(); -} - -void CXFA_Document::AddPurgeNode(CXFA_Node* pNode) { - m_PurgeNodes.insert(pNode); -} - -bool CXFA_Document::RemovePurgeNode(CXFA_Node* pNode) { - return !!m_PurgeNodes.erase(pNode); + return AddOwnedNode(CXFA_Node::Create(this, eElement, packet)); } void CXFA_Document::SetFlag(uint32_t dwFlag, bool bOn) { diff --git a/xfa/fxfa/parser/cxfa_document.h b/xfa/fxfa/parser/cxfa_document.h index 09c2601fa9..4d1c417c5b 100644 --- a/xfa/fxfa/parser/cxfa_document.h +++ b/xfa/fxfa/parser/cxfa_document.h @@ -9,11 +9,11 @@ #include #include -#include #include #include "xfa/fxfa/fxfa.h" #include "xfa/fxfa/parser/cxfa_localemgr.h" +#include "xfa/fxfa/parser/cxfa_nodeowner.h" enum XFA_VERSION { XFA_VERSION_UNKNOWN = 0, @@ -57,10 +57,10 @@ class CXFA_LayoutProcessor; class CXFA_Node; class CXFA_Object; -class CXFA_Document { +class CXFA_Document : public CXFA_NodeOwner { public: explicit CXFA_Document(CXFA_DocumentParser* pParser); - virtual ~CXFA_Document(); + ~CXFA_Document() override; virtual CXFA_FFNotify* GetNotify() const; @@ -77,10 +77,7 @@ class CXFA_Document { CXFA_LayoutProcessor* GetDocLayout(); CFXJSE_Engine* GetScriptContext(); - void SetRoot(CXFA_Node* pNewRoot); - - void AddPurgeNode(CXFA_Node* pNode); - bool RemovePurgeNode(CXFA_Node* pNode); + void SetRoot(CXFA_Node* pNewRoot) { m_pRootNode = pNewRoot; } bool HasFlag(uint32_t dwFlag) { return (m_dwDocFlags & dwFlag) == dwFlag; } void SetFlag(uint32_t dwFlag, bool bOn); @@ -119,7 +116,6 @@ class CXFA_Document { std::unique_ptr m_pScriptLog; std::unique_ptr m_pScriptLayout; std::unique_ptr m_pScriptSignature; - std::set m_PurgeNodes; XFA_VERSION m_eCurVersionMode; uint32_t m_dwDocFlags; }; diff --git a/xfa/fxfa/parser/cxfa_node.cpp b/xfa/fxfa/parser/cxfa_node.cpp index 83da3e76df..9159f020da 100644 --- a/xfa/fxfa/parser/cxfa_node.cpp +++ b/xfa/fxfa/parser/cxfa_node.cpp @@ -537,16 +537,7 @@ CXFA_Node::CXFA_Node(CXFA_Document* pDoc, pdfium::MakeUnique(this)) {} CXFA_Node::~CXFA_Node() { - ASSERT(!parent_); - - CXFA_Node* pNode = first_child_; - while (pNode) { - CXFA_Node* pNext = pNode->next_sibling_; - pNode->parent_ = nullptr; - delete pNode; - pNode = pNext; - } - if (m_pXMLNode && IsOwnXMLNode()) + if (m_pXMLNode && IsOwnedXMLNode()) delete m_pXMLNode; } @@ -1119,10 +1110,6 @@ void CXFA_Node::InsertChild(int32_t index, CXFA_Node* pNode) { pNode->parent_ = this; pNode->ClearFlag(XFA_NodeFlag_HasRemovedChildren); - bool ret = m_pDocument->RemovePurgeNode(pNode); - ASSERT(ret); - (void)ret; // Avoid unused variable warning. - if (!first_child_) { ASSERT(!last_child_); @@ -1221,8 +1208,6 @@ void CXFA_Node::RemoveChild(CXFA_Node* pNode, bool bNotify) { OnRemoved(bNotify); - m_pDocument->AddPurgeNode(pNode); - if (!IsNeedSavingXMLNode() || !pNode->m_pXMLNode) return; diff --git a/xfa/fxfa/parser/cxfa_node.h b/xfa/fxfa/parser/cxfa_node.h index e800c3c0c3..f5eb2be77e 100644 --- a/xfa/fxfa/parser/cxfa_node.h +++ b/xfa/fxfa/parser/cxfa_node.h @@ -469,7 +469,7 @@ class CXFA_Node : public CXFA_Object { void SetImageEdit(const WideString& wsContentType, const WideString& wsHref, const WideString& wsData); - bool IsOwnXMLNode() const { return HasFlag(XFA_NodeFlag_OwnXMLNode); } + bool IsOwnedXMLNode() const { return HasFlag(XFA_NodeFlag_OwnXMLNode); } CXFA_Node* GetBindingNode() const { if (binding_nodes_.empty()) return nullptr; diff --git a/xfa/fxfa/parser/cxfa_nodeowner.cpp b/xfa/fxfa/parser/cxfa_nodeowner.cpp new file mode 100644 index 0000000000..ae6f589616 --- /dev/null +++ b/xfa/fxfa/parser/cxfa_nodeowner.cpp @@ -0,0 +1,35 @@ +// Copyright 2018 PDFium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com + +#include "xfa/fxfa/parser/cxfa_nodeowner.h" + +#include + +#include "third_party/base/stl_util.h" +#include "xfa/fxfa/parser/cxfa_node.h" + +CXFA_NodeOwner::CXFA_NodeOwner() = default; + +CXFA_NodeOwner::~CXFA_NodeOwner() = default; + +CXFA_Node* CXFA_NodeOwner::AddOwnedNode(std::unique_ptr node) { + if (!node) + return nullptr; + + CXFA_Node* ret = node.get(); + nodes_.insert(std::move(node)); + return ret; +} + +void CXFA_NodeOwner::FreeOwnedNode(CXFA_Node* node) { + if (!node) + return; + + pdfium::FakeUniquePtr search(node); + auto it = nodes_.find(search); + assert(it != nodes_.end()); + nodes_.erase(it); +} diff --git a/xfa/fxfa/parser/cxfa_nodeowner.h b/xfa/fxfa/parser/cxfa_nodeowner.h new file mode 100644 index 0000000000..2aaabe4118 --- /dev/null +++ b/xfa/fxfa/parser/cxfa_nodeowner.h @@ -0,0 +1,28 @@ +// Copyright 2018 PDFium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com + +#ifndef XFA_FXFA_PARSER_CXFA_NODEOWNER_H_ +#define XFA_FXFA_PARSER_CXFA_NODEOWNER_H_ + +#include +#include + +class CXFA_Node; + +class CXFA_NodeOwner { + public: + virtual ~CXFA_NodeOwner(); + + CXFA_Node* AddOwnedNode(std::unique_ptr node); + void FreeOwnedNode(CXFA_Node* node); + + protected: + CXFA_NodeOwner(); + + std::set> nodes_; +}; + +#endif // XFA_FXFA_PARSER_CXFA_NODEOWNER_H_ -- cgit v1.2.3