From 13aa65a71294cac6e4bdaab73ddd6f4b9fcd8676 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Tue, 17 Apr 2018 21:34:18 +0000 Subject: Add ownership to CFX_XMLNode children This CL sets the CFX_XML tree ownership. The pointers set into the tree must be unique_ptrs and the CFX_XMLNode children are set to be either unique_ptrs or UnownedPtrs. Change-Id: Ib0db495c81471e40f5b4533503f7bbe5a784fd77 Reviewed-on: https://pdfium-review.googlesource.com/30711 Reviewed-by: Henrique Nakashima Commit-Queue: dsinclair --- core/fxcrt/xml/cfx_xmlelement.cpp | 2 +- core/fxcrt/xml/cfx_xmlnode.cpp | 77 ++++++++++++++++++-------------- core/fxcrt/xml/cfx_xmlnode.h | 22 ++++----- core/fxcrt/xml/cfx_xmlparser.cpp | 30 ++++++++----- fxjs/xfa/cjx_node.cpp | 4 +- xfa/fxfa/parser/cxfa_document.cpp | 3 +- xfa/fxfa/parser/cxfa_document_parser.cpp | 7 ++- xfa/fxfa/parser/cxfa_node.cpp | 10 +---- 8 files changed, 86 insertions(+), 69 deletions(-) diff --git a/core/fxcrt/xml/cfx_xmlelement.cpp b/core/fxcrt/xml/cfx_xmlelement.cpp index c54d8488a3..d37c55c443 100644 --- a/core/fxcrt/xml/cfx_xmlelement.cpp +++ b/core/fxcrt/xml/cfx_xmlelement.cpp @@ -90,7 +90,7 @@ WideString CFX_XMLElement::GetTextData() const { void CFX_XMLElement::SetTextData(const WideString& wsText) { if (wsText.GetLength() < 1) return; - AppendChild(new CFX_XMLText(wsText)); + AppendChild(pdfium::MakeUnique(wsText)); } void CFX_XMLElement::Save(const RetainPtr& pXMLStream) { diff --git a/core/fxcrt/xml/cfx_xmlnode.cpp b/core/fxcrt/xml/cfx_xmlnode.cpp index 6303615d01..a8be133171 100644 --- a/core/fxcrt/xml/cfx_xmlnode.cpp +++ b/core/fxcrt/xml/cfx_xmlnode.cpp @@ -6,6 +6,7 @@ #include "core/fxcrt/xml/cfx_xmlnode.h" +#include #include #include "core/fxcrt/xml/cfx_xmlchardata.h" @@ -25,81 +26,89 @@ FX_XMLNODETYPE CFX_XMLNode::GetType() const { } void CFX_XMLNode::DeleteChildren() { - CFX_XMLNode* pChild = first_child_; - first_child_ = nullptr; + CFX_XMLNode* child = last_child_.Get(); + // Clear last child early as it will have been deleted already. last_child_ = nullptr; - - while (pChild) { - CFX_XMLNode* pNext = pChild->next_sibling_; - delete pChild; - pChild = pNext; + while (child) { + child = child->prev_sibling_.Get(); + if (child) + child->next_sibling_ = nullptr; } + first_child_ = nullptr; } -void CFX_XMLNode::AppendChild(CFX_XMLNode* pNode) { - InsertChildNode(pNode, -1); +void CFX_XMLNode::AppendChild(std::unique_ptr pNode) { + InsertChildNode(std::move(pNode), -1); } -void CFX_XMLNode::InsertChildNode(CFX_XMLNode* pNode, int32_t index) { +void CFX_XMLNode::InsertChildNode(std::unique_ptr pNode, + int32_t index) { ASSERT(!pNode->parent_); pNode->parent_ = this; + // No existing children, add node as first child. if (!first_child_) { ASSERT(!last_child_); - first_child_ = pNode; - last_child_ = pNode; - pNode->prev_sibling_ = nullptr; - pNode->next_sibling_ = nullptr; + first_child_ = std::move(pNode); + last_child_ = first_child_.get(); + first_child_->prev_sibling_ = nullptr; + first_child_->next_sibling_ = nullptr; return; } if (index == 0) { - pNode->next_sibling_ = first_child_; + first_child_->prev_sibling_ = pNode.get(); + pNode->next_sibling_ = std::move(first_child_); pNode->prev_sibling_ = nullptr; - first_child_->prev_sibling_ = pNode; - first_child_ = pNode; + first_child_ = std::move(pNode); return; } int32_t iCount = 0; - CFX_XMLNode* pFind = first_child_; + CFX_XMLNode* pFind = first_child_.get(); + // Note, negative indexes, and indexes after the end of the list will result + // in appending to the list. while (++iCount != index && pFind->next_sibling_) - pFind = pFind->next_sibling_; + pFind = pFind->next_sibling_.get(); pNode->prev_sibling_ = pFind; - pNode->next_sibling_ = pFind->next_sibling_; if (pFind->next_sibling_) - pFind->next_sibling_->prev_sibling_ = pNode; + pFind->next_sibling_->prev_sibling_ = pNode.get(); + pNode->next_sibling_ = std::move(pFind->next_sibling_); - pFind->next_sibling_ = pNode; - if (pFind == last_child_) - last_child_ = pNode; + pFind->next_sibling_ = std::move(pNode); + if (pFind == last_child_.Get()) + last_child_ = pFind->next_sibling_.get(); } void CFX_XMLNode::RemoveChildNode(CFX_XMLNode* pNode) { - ASSERT(first_child_ && pNode); - - if (first_child_ == pNode) - first_child_ = pNode->next_sibling_; - else - pNode->prev_sibling_->next_sibling_ = pNode->next_sibling_; + ASSERT(first_child_); + ASSERT(pNode); + + if (first_child_.get() == pNode) { + first_child_.release(); + first_child_ = std::move(pNode->next_sibling_); + } else { + CFX_XMLNode* prev = pNode->prev_sibling_.Get(); + prev->next_sibling_.release(); // Release pNode + prev->next_sibling_ = std::move(pNode->next_sibling_); + } - if (last_child_ == pNode) - last_child_ = pNode->prev_sibling_; + if (last_child_.Get() == pNode) + last_child_ = pNode->prev_sibling_.Get(); if (pNode->next_sibling_) pNode->next_sibling_->prev_sibling_ = pNode->prev_sibling_; pNode->parent_ = nullptr; - pNode->next_sibling_ = nullptr; pNode->prev_sibling_ = nullptr; } CFX_XMLNode* CFX_XMLNode::GetRoot() { CFX_XMLNode* pParent = this; while (pParent->parent_) - pParent = pParent->parent_; + pParent = pParent->parent_.Get(); return pParent; } diff --git a/core/fxcrt/xml/cfx_xmlnode.h b/core/fxcrt/xml/cfx_xmlnode.h index 86b5e998e6..d46ab9b29d 100644 --- a/core/fxcrt/xml/cfx_xmlnode.h +++ b/core/fxcrt/xml/cfx_xmlnode.h @@ -35,12 +35,12 @@ class CFX_XMLNode { virtual void Save(const RetainPtr& pXMLStream); CFX_XMLNode* GetRoot(); - CFX_XMLNode* GetParent() const { return parent_; } - CFX_XMLNode* GetFirstChild() const { return first_child_; } - CFX_XMLNode* GetNextSibling() const { return next_sibling_; } + CFX_XMLNode* GetParent() const { return parent_.Get(); } + CFX_XMLNode* GetFirstChild() const { return first_child_.get(); } + CFX_XMLNode* GetNextSibling() const { return next_sibling_.get(); } - void AppendChild(CFX_XMLNode* pNode); - void InsertChildNode(CFX_XMLNode* pNode, int32_t index); + void AppendChild(std::unique_ptr pNode); + void InsertChildNode(std::unique_ptr pNode, int32_t index); void RemoveChildNode(CFX_XMLNode* pNode); void DeleteChildren(); @@ -48,11 +48,13 @@ class CFX_XMLNode { WideString EncodeEntities(const WideString& value); private: - CFX_XMLNode* parent_ = nullptr; - CFX_XMLNode* next_sibling_ = nullptr; - CFX_XMLNode* prev_sibling_ = nullptr; - CFX_XMLNode* first_child_ = nullptr; - CFX_XMLNode* last_child_ = nullptr; + // A node owns it's first child and it owns it's next sibling. The rest + // are unowned pointers. + UnownedPtr parent_; + UnownedPtr last_child_; + UnownedPtr prev_sibling_; + std::unique_ptr first_child_; + std::unique_ptr next_sibling_; }; #endif // CORE_FXCRT_XML_CFX_XMLNODE_H_ diff --git a/core/fxcrt/xml/cfx_xmlparser.cpp b/core/fxcrt/xml/cfx_xmlparser.cpp index 9336c11f87..9be8da76b4 100644 --- a/core/fxcrt/xml/cfx_xmlparser.cpp +++ b/core/fxcrt/xml/cfx_xmlparser.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include "core/fxcrt/fx_codepage.h" #include "core/fxcrt/fx_extension.h" @@ -177,20 +178,23 @@ bool CFX_XMLParser::Parse() { case FX_XmlSyntaxResult::TargetName: m_ws1 = GetTargetName(); if (m_ws1 == L"originalXFAVersion" || m_ws1 == L"acrobat") { - m_pChild = new CFX_XMLInstruction(m_ws1); - m_pParent->AppendChild(m_pChild); + auto child = pdfium::MakeUnique(m_ws1); + m_pChild = child.get(); + m_pParent->AppendChild(std::move(child)); } else { m_pChild = nullptr; } m_ws1.clear(); break; - case FX_XmlSyntaxResult::TagName: + case FX_XmlSyntaxResult::TagName: { m_ws1 = GetTagName(); - m_pChild = new CFX_XMLElement(m_ws1); - m_pParent->AppendChild(m_pChild); + auto child = pdfium::MakeUnique(m_ws1); + m_pChild = child.get(); + m_pParent->AppendChild(std::move(child)); m_NodeStack.push(m_pChild); m_pParent = m_pChild; break; + } case FX_XmlSyntaxResult::AttriName: m_ws1 = GetAttributeName(); break; @@ -201,18 +205,22 @@ bool CFX_XMLParser::Parse() { } m_ws1.clear(); break; - case FX_XmlSyntaxResult::Text: + case FX_XmlSyntaxResult::Text: { m_ws1 = GetTextData(); - m_pChild = new CFX_XMLText(m_ws1); - m_pParent->AppendChild(m_pChild); + auto child = pdfium::MakeUnique(m_ws1); + m_pChild = child.get(); + m_pParent->AppendChild(std::move(child)); m_pChild = m_pParent; break; - case FX_XmlSyntaxResult::CData: + } + case FX_XmlSyntaxResult::CData: { m_ws1 = GetTextData(); - m_pChild = new CFX_XMLCharData(m_ws1); - m_pParent->AppendChild(m_pChild); + auto child = pdfium::MakeUnique(m_ws1); + m_pChild = child.get(); + m_pParent->AppendChild(std::move(child)); m_pChild = m_pParent; break; + } case FX_XmlSyntaxResult::TargetData: if (m_pChild) { if (m_pChild->GetType() != FX_XMLNODE_Instruction) diff --git a/fxjs/xfa/cjx_node.cpp b/fxjs/xfa/cjx_node.cpp index 44a5a1a42c..a8a88c9627 100644 --- a/fxjs/xfa/cjx_node.cpp +++ b/fxjs/xfa/cjx_node.cpp @@ -251,7 +251,7 @@ CJS_Return CJX_Node::loadXML(CFX_V8* runtime, while (pXMLChild) { CFX_XMLNode* pXMLSibling = pXMLChild->GetNextSibling(); pXMLNode->RemoveChildNode(pXMLChild); - pFakeXMLRoot->AppendChild(pXMLChild); + pFakeXMLRoot->AppendChild(pdfium::WrapUnique(pXMLChild)); pXMLChild = pXMLSibling; } } else { @@ -259,7 +259,7 @@ CJS_Return CJX_Node::loadXML(CFX_V8* runtime, if (pXMLParent) pXMLParent->RemoveChildNode(pXMLNode); - pFakeXMLRoot->AppendChild(pXMLNode); + pFakeXMLRoot->AppendChild(pdfium::WrapUnique(pXMLNode)); } pParser->ConstructXFANode(pFakeRoot, pFakeXMLRoot.get()); diff --git a/xfa/fxfa/parser/cxfa_document.cpp b/xfa/fxfa/parser/cxfa_document.cpp index 713e15c4fa..c3e73e2f19 100644 --- a/xfa/fxfa/parser/cxfa_document.cpp +++ b/xfa/fxfa/parser/cxfa_document.cpp @@ -7,6 +7,7 @@ #include "xfa/fxfa/parser/cxfa_document.h" #include +#include #include "core/fxcrt/fx_extension.h" #include "core/fxcrt/fx_fallthrough.h" @@ -1633,7 +1634,7 @@ void CXFA_Document::DoDataMerge() { false); CFX_XMLElement* ref = pDatasetsXMLNode.get(); - m_pRootNode->GetXMLMappingNode()->AppendChild(pDatasetsXMLNode.release()); + m_pRootNode->GetXMLMappingNode()->AppendChild(std::move(pDatasetsXMLNode)); m_pRootNode->InsertChild(pDatasetsRoot, nullptr); pDatasetsRoot->SetXMLMappingNode(ref); } diff --git a/xfa/fxfa/parser/cxfa_document_parser.cpp b/xfa/fxfa/parser/cxfa_document_parser.cpp index 62828d71a7..4770d849b1 100644 --- a/xfa/fxfa/parser/cxfa_document_parser.cpp +++ b/xfa/fxfa/parser/cxfa_document_parser.cpp @@ -349,7 +349,7 @@ std::unique_ptr CXFA_DocumentParser::LoadXML( ASSERT(pStream); auto root = pdfium::MakeUnique(); - root->AppendChild(new CFX_XMLInstruction(L"xml")); + root->AppendChild(pdfium::MakeUnique(L"xml")); CFX_XMLParser parser(root.get(), pStream); return parser.Parse() ? std::move(root) : nullptr; @@ -674,7 +674,10 @@ CXFA_Node* CXFA_DocumentParser::ParseAsXDPPacket_Data( static_cast(pXMLDocumentNode) ->RemoveAttribute(L"xmlns:xfa"); } - pDataElement->AppendChild(pXMLDocumentNode); + // The node was either removed from the parent above, or already has no + // parent so we can take ownership. + pDataElement->AppendChild( + pdfium::WrapUnique(pXMLDocumentNode)); pDataXMLNode.Reset(std::move(pDataElement)); } if (!pDataXMLNode) diff --git a/xfa/fxfa/parser/cxfa_node.cpp b/xfa/fxfa/parser/cxfa_node.cpp index 7da4ed551d..3399023037 100644 --- a/xfa/fxfa/parser/cxfa_node.cpp +++ b/xfa/fxfa/parser/cxfa_node.cpp @@ -1158,14 +1158,8 @@ void CXFA_Node::InsertChild(int32_t index, CXFA_Node* pNode) { return; ASSERT(!pNode->xml_node_->GetParent()); - - xml_node_->InsertChildNode(pNode->xml_node_.Get(), index); - if (pNode->xml_node_.IsOwned()) { - // We remove ownership of the XML node from pNode and transfer the ownership - // into the XML tree, the pNode still has an unowned pointer to the XML - // node. - pNode->xml_node_.Release().release(); - } + ASSERT(pNode->xml_node_.IsOwned()); + xml_node_->InsertChildNode(pNode->xml_node_.Release(), index); } void CXFA_Node::InsertChild(CXFA_Node* pNode, CXFA_Node* pBeforeNode) { -- cgit v1.2.3