From e40678ed8a22ecd57421877af39cf7f281f618c4 Mon Sep 17 00:00:00 2001 From: Dan Sinclair Date: Thu, 15 Feb 2018 18:12:29 +0000 Subject: Make the CFX_XMLNode a MaybeOwned pointer This CL removes the HasOwnXML flag from the CXFA_Node objects and instead uses a MaybeOwned pointer to keep track of the XML nodes. Change-Id: Ie678258247ec21ecb15c639647b189e140586d25 Reviewed-on: https://pdfium-review.googlesource.com/26811 Reviewed-by: Tom Sepez Commit-Queue: dsinclair --- core/fxcrt/maybe_owned.h | 5 ++ fxjs/xfa/cjx_node.cpp | 8 +- xfa/fxfa/parser/cxfa_document_parser.cpp | 1 + xfa/fxfa/parser/cxfa_document_parser.h | 2 + xfa/fxfa/parser/cxfa_node.cpp | 97 ++++++++++++++----------- xfa/fxfa/parser/cxfa_node.h | 23 ++++-- xfa/fxfa/parser/cxfa_nodeowner.cpp | 13 ++++ xfa/fxfa/parser/cxfa_nodeowner.h | 2 + xfa/fxfa/parser/cxfa_simple_parser.cpp | 41 +++++------ xfa/fxfa/parser/xfa_document_datamerger_imp.cpp | 22 +++--- 10 files changed, 128 insertions(+), 86 deletions(-) diff --git a/core/fxcrt/maybe_owned.h b/core/fxcrt/maybe_owned.h index 130d2bdc3c..d920ea7a91 100644 --- a/core/fxcrt/maybe_owned.h +++ b/core/fxcrt/maybe_owned.h @@ -41,6 +41,11 @@ class MaybeOwned { m_pObj = ptr; m_pOwnedObj.reset(); } + // Helpful for untangling a collection of intertwined MaybeOwned<>. + void ResetIfUnowned() { + if (!IsOwned()) + Reset(); + } T* Get() const { return m_pObj.Get(); } bool IsOwned() const { return !!m_pOwnedObj; } diff --git a/fxjs/xfa/cjx_node.cpp b/fxjs/xfa/cjx_node.cpp index 9b5d4044ea..1343d258e1 100644 --- a/fxjs/xfa/cjx_node.cpp +++ b/fxjs/xfa/cjx_node.cpp @@ -7,6 +7,7 @@ #include "fxjs/xfa/cjx_node.h" #include +#include #include #include "core/fxcrt/cfx_memorystream.h" @@ -288,8 +289,8 @@ CJS_Return CJX_Node::loadXML(CFX_V8* runtime, if (GetXFANode()->GetPacketType() == XFA_PacketType::Form && GetXFANode()->GetElementType() == XFA_Element::ExData) { CFX_XMLNode* pTempXMLNode = GetXFANode()->GetXMLMappingNode(); - GetXFANode()->SetXMLMappingNode(pFakeXMLRoot.release()); - GetXFANode()->SetFlag(XFA_NodeFlag_OwnXMLNode); + GetXFANode()->SetXMLMappingNode(std::move(pFakeXMLRoot)); + if (pTempXMLNode && !pTempXMLNode->GetParent()) pFakeXMLRoot.reset(pTempXMLNode); else @@ -308,8 +309,7 @@ CJS_Return CJX_Node::loadXML(CFX_V8* runtime, } if (pFakeXMLRoot) { - pFakeRoot->SetXMLMappingNode(pFakeXMLRoot.release()); - pFakeRoot->SetFlag(XFA_NodeFlag_OwnXMLNode); + pFakeRoot->SetXMLMappingNode(std::move(pFakeXMLRoot)); } pFakeRoot->SetFlag(XFA_NodeFlag_HasRemovedChildren); diff --git a/xfa/fxfa/parser/cxfa_document_parser.cpp b/xfa/fxfa/parser/cxfa_document_parser.cpp index 652694e214..3e4aa58dd9 100644 --- a/xfa/fxfa/parser/cxfa_document_parser.cpp +++ b/xfa/fxfa/parser/cxfa_document_parser.cpp @@ -15,6 +15,7 @@ CXFA_DocumentParser::CXFA_DocumentParser(CXFA_FFNotify* pNotify) : m_pNotify(pNotify) {} CXFA_DocumentParser::~CXFA_DocumentParser() { + m_pDocument->ReleaseXMLNodesIfNeeded(); } int32_t CXFA_DocumentParser::StartParse( diff --git a/xfa/fxfa/parser/cxfa_document_parser.h b/xfa/fxfa/parser/cxfa_document_parser.h index c245843c0f..77265e4d05 100644 --- a/xfa/fxfa/parser/cxfa_document_parser.h +++ b/xfa/fxfa/parser/cxfa_document_parser.h @@ -31,6 +31,8 @@ class CXFA_DocumentParser { private: UnownedPtr const m_pNotify; + // Note, the |m_nodeParser| has an unowned pointer to the |m_pDocument| so + // the |m_nodeParser| must be cleaned up first. std::unique_ptr m_pDocument; CXFA_SimpleParser m_nodeParser; }; diff --git a/xfa/fxfa/parser/cxfa_node.cpp b/xfa/fxfa/parser/cxfa_node.cpp index a534d6ee4f..bfee8fa746 100644 --- a/xfa/fxfa/parser/cxfa_node.cpp +++ b/xfa/fxfa/parser/cxfa_node.cpp @@ -510,7 +510,6 @@ CXFA_Node::CXFA_Node(CXFA_Document* pDoc, prev_sibling_(nullptr), first_child_(nullptr), last_child_(nullptr), - m_pXMLNode(nullptr), m_ePacket(ePacket), m_uNodeFlags(XFA_NodeFlag_None), m_dwNameHash(0), @@ -536,9 +535,12 @@ CXFA_Node::CXFA_Node(CXFA_Document* pDoc, elementName, pdfium::MakeUnique(this)) {} -CXFA_Node::~CXFA_Node() { - if (m_pXMLNode && HasFlag(XFA_NodeFlag_OwnXMLNode)) - delete m_pXMLNode; +CXFA_Node::~CXFA_Node() = default; + +void CXFA_Node::ReleaseXMLNodeIfUnowned() { + // Note, this is intentionally non-recursive. The caller is responsible for + // triggering this on all the nodes which need it. + xml_node_.ResetIfUnowned(); } CXFA_Node* CXFA_Node::Clone(bool bRecursive) { @@ -563,10 +565,9 @@ CXFA_Node* CXFA_Node::Clone(bool bRecursive) { pClone->JSObject()->SetEnum(XFA_Attribute::Contains, XFA_AttributeEnum::Unknown, false); } else { - pCloneXML = m_pXMLNode->Clone(); + pCloneXML = xml_node_->Clone(); } - pClone->SetXMLMappingNode(pCloneXML.release()); - pClone->SetFlag(XFA_NodeFlag_OwnXMLNode); + pClone->SetXMLMappingNode(std::move(pCloneXML)); } if (bRecursive) { for (CXFA_Node* pChild = GetFirstChild(); pChild; @@ -1151,13 +1152,18 @@ void CXFA_Node::InsertChild(int32_t index, CXFA_Node* pNode) { if (pNotify) pNotify->OnChildAdded(this); - if (!IsNeedSavingXMLNode() || !pNode->m_pXMLNode) + if (!IsNeedSavingXMLNode() || !pNode->xml_node_) return; - ASSERT(!pNode->m_pXMLNode->GetParent()); + ASSERT(!pNode->xml_node_->GetParent()); - m_pXMLNode->InsertChildNode(pNode->m_pXMLNode, index); - pNode->ClearFlag(XFA_NodeFlag_OwnXMLNode); + 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(); + } } void CXFA_Node::InsertChild(CXFA_Node* pNode, CXFA_Node* pBeforeNode) { @@ -1208,35 +1214,41 @@ void CXFA_Node::RemoveChild(CXFA_Node* pNode, bool bNotify) { OnRemoved(bNotify); - if (!IsNeedSavingXMLNode() || !pNode->m_pXMLNode) + if (!IsNeedSavingXMLNode() || !pNode->xml_node_) return; - if (pNode->IsAttributeInXML()) { - ASSERT(pNode->m_pXMLNode == m_pXMLNode && - m_pXMLNode->GetType() == FX_XMLNODE_Element); - if (pNode->m_pXMLNode->GetType() == FX_XMLNODE_Element) { - CFX_XMLElement* pXMLElement = - static_cast(pNode->m_pXMLNode); - WideString wsAttributeName = - pNode->JSObject()->GetCData(XFA_Attribute::QualifiedName); - pXMLElement->RemoveAttribute(wsAttributeName.c_str()); - } + if (!pNode->IsAttributeInXML()) { + // This XML node _must_ be in the xml tree if we arrived here, so it is + // unowned by the XFA_Node. We turn the nodes pointer into a unique_ptr, + // remove from the XML tree and then assign the owned pointer back onto the + // XFA_Node. + auto node = pdfium::WrapUnique(pNode->xml_node_.Get()); + xml_node_->RemoveChildNode(node.get()); + pNode->xml_node_.Reset(std::move(node)); + return; + } - WideString wsName = pNode->JSObject() - ->TryAttribute(XFA_Attribute::Name, false) - .value_or(WideString()); - CFX_XMLElement* pNewXMLElement = new CFX_XMLElement(wsName); - WideString wsValue = JSObject()->GetCData(XFA_Attribute::Value); - if (!wsValue.IsEmpty()) - pNewXMLElement->SetTextData(WideString(wsValue)); - - pNode->m_pXMLNode = pNewXMLElement; - pNode->JSObject()->SetEnum(XFA_Attribute::Contains, - XFA_AttributeEnum::Unknown, false); - } else { - m_pXMLNode->RemoveChildNode(pNode->m_pXMLNode); + ASSERT(pNode->xml_node_.Get() == xml_node_.Get() && + xml_node_->GetType() == FX_XMLNODE_Element); + if (pNode->xml_node_->GetType() == FX_XMLNODE_Element) { + CFX_XMLElement* pXMLElement = + static_cast(pNode->xml_node_.Get()); + WideString wsAttributeName = + pNode->JSObject()->GetCData(XFA_Attribute::QualifiedName); + pXMLElement->RemoveAttribute(wsAttributeName.c_str()); } - pNode->SetFlag(XFA_NodeFlag_OwnXMLNode); + + WideString wsName = pNode->JSObject() + ->TryAttribute(XFA_Attribute::Name, false) + .value_or(WideString()); + auto pNewXMLElement = pdfium::MakeUnique(wsName); + WideString wsValue = JSObject()->GetCData(XFA_Attribute::Value); + if (!wsValue.IsEmpty()) + pNewXMLElement->SetTextData(WideString(wsValue)); + + pNode->xml_node_.Reset(std::move(pNewXMLElement)); + pNode->JSObject()->SetEnum(XFA_Attribute::Contains, + XFA_AttributeEnum::Unknown, false); } CXFA_Node* CXFA_Node::GetFirstChildByName(const WideStringView& wsName) const { @@ -1375,17 +1387,16 @@ void CXFA_Node::UpdateNameHash() { } CFX_XMLNode* CXFA_Node::CreateXMLMappingNode() { - if (!m_pXMLNode) { - WideString wsTag(JSObject()->GetCData(XFA_Attribute::Name)); - m_pXMLNode = new CFX_XMLElement(wsTag); - SetFlag(XFA_NodeFlag_OwnXMLNode); + if (!xml_node_) { + xml_node_ = pdfium::MakeUnique( + JSObject()->GetCData(XFA_Attribute::Name)); } - return m_pXMLNode; + return xml_node_.Get(); } bool CXFA_Node::IsNeedSavingXMLNode() { - return m_pXMLNode && (GetPacketType() == XFA_PacketType::Datasets || - GetElementType() == XFA_Element::Xfa); + return xml_node_ && (GetPacketType() == XFA_PacketType::Datasets || + GetElementType() == XFA_Element::Xfa); } CXFA_Node* CXFA_Node::GetItemIfExists(int32_t iIndex) { diff --git a/xfa/fxfa/parser/cxfa_node.h b/xfa/fxfa/parser/cxfa_node.h index 8193f59e05..85aca156d8 100644 --- a/xfa/fxfa/parser/cxfa_node.h +++ b/xfa/fxfa/parser/cxfa_node.h @@ -13,6 +13,8 @@ #include #include "core/fxcrt/fx_string.h" +#include "core/fxcrt/maybe_owned.h" +#include "core/fxcrt/xml/cfx_xmlnode.h" #include "core/fxge/fx_dib.h" #include "fxbarcode/BC_Library.h" #include "third_party/base/optional.h" @@ -21,7 +23,6 @@ class CFGAS_GEFont; class CFX_DIBitmap; -class CFX_XMLNode; class CXFA_Bind; class CXFA_Border; class CXFA_Calculate; @@ -67,9 +68,8 @@ enum XFA_NodeFlag { XFA_NodeFlag_NeedsInitApp = 1 << 2, XFA_NodeFlag_BindFormItems = 1 << 3, XFA_NodeFlag_UserInteractive = 1 << 4, - XFA_NodeFlag_OwnXMLNode = 1 << 5, - XFA_NodeFlag_UnusedNode = 1 << 6, - XFA_NodeFlag_LayoutGeneratedNode = 1 << 7 + XFA_NodeFlag_UnusedNode = 1 << 5, + XFA_NodeFlag_LayoutGeneratedNode = 1 << 6 }; class CXFA_Node : public CXFA_Object { @@ -156,10 +156,19 @@ class CXFA_Node : public CXFA_Object { bool IsFormContainer() const { return m_ePacket == XFA_PacketType::Form && IsContainerNode(); } - void SetXMLMappingNode(CFX_XMLNode* pXMLNode) { m_pXMLNode = pXMLNode; } - CFX_XMLNode* GetXMLMappingNode() const { return m_pXMLNode; } + + void ReleaseXMLNodeIfUnowned(); + void SetXMLMappingNode(MaybeOwned node) { + xml_node_ = std::move(node); + } + void SetXMLMappingNode(std::unique_ptr node) { + xml_node_.Reset(std::move(node)); + } + void SetXMLMappingNode(CFX_XMLNode* node) { xml_node_.Reset(node); } + CFX_XMLNode* GetXMLMappingNode() const { return xml_node_.Get(); } CFX_XMLNode* CreateXMLMappingNode(); bool IsNeedSavingXMLNode(); + uint32_t GetNameHash() const { return m_dwNameHash; } bool IsUnnamed() const { return m_dwNameHash == 0; } CXFA_Node* GetModelNode(); @@ -502,7 +511,7 @@ class CXFA_Node : public CXFA_Object { CXFA_Node* first_child_; CXFA_Node* last_child_; - CFX_XMLNode* m_pXMLNode; + MaybeOwned xml_node_; const XFA_PacketType m_ePacket; uint8_t m_ExecuteRecursionDepth = 0; uint16_t m_uNodeFlags; diff --git a/xfa/fxfa/parser/cxfa_nodeowner.cpp b/xfa/fxfa/parser/cxfa_nodeowner.cpp index ae6f589616..63f2f717c2 100644 --- a/xfa/fxfa/parser/cxfa_nodeowner.cpp +++ b/xfa/fxfa/parser/cxfa_nodeowner.cpp @@ -15,6 +15,19 @@ CXFA_NodeOwner::CXFA_NodeOwner() = default; CXFA_NodeOwner::~CXFA_NodeOwner() = default; +void CXFA_NodeOwner::ReleaseXMLNodesIfNeeded() { + // Because we don't know what order we'll free the nodes we may end up + // destroying the XML tree before nodes have been cleaned up that point into + // it. This will cause the ProbeForLowSeverityLifetimeIssue to fire. + // + // This doesn't happen in the destructor because of the ownership semantics + // between the CXFA_Document and CXFA_SimpleParser. It has to happen before + // the simple parser is destroyed, but the document has to live longer then + // the simple parser. + for (auto& it : nodes_) + it->ReleaseXMLNodeIfUnowned(); +} + CXFA_Node* CXFA_NodeOwner::AddOwnedNode(std::unique_ptr node) { if (!node) return nullptr; diff --git a/xfa/fxfa/parser/cxfa_nodeowner.h b/xfa/fxfa/parser/cxfa_nodeowner.h index 2aaabe4118..329b1e3284 100644 --- a/xfa/fxfa/parser/cxfa_nodeowner.h +++ b/xfa/fxfa/parser/cxfa_nodeowner.h @@ -16,6 +16,8 @@ class CXFA_NodeOwner { public: virtual ~CXFA_NodeOwner(); + void ReleaseXMLNodesIfNeeded(); + CXFA_Node* AddOwnedNode(std::unique_ptr node); void FreeOwnedNode(CXFA_Node* node); diff --git a/xfa/fxfa/parser/cxfa_simple_parser.cpp b/xfa/fxfa/parser/cxfa_simple_parser.cpp index 7424913114..f30ab6b631 100644 --- a/xfa/fxfa/parser/cxfa_simple_parser.cpp +++ b/xfa/fxfa/parser/cxfa_simple_parser.cpp @@ -724,13 +724,13 @@ CXFA_Node* CXFA_SimpleParser::ParseAsXDPPacket_Data( return pNode; } - CFX_XMLNode* pDataXMLNode = nullptr; + MaybeOwned pDataXMLNode; if (MatchNodeName(pXMLDocumentNode, L"data", packet->uri, packet->flags)) { static_cast(pXMLDocumentNode) ->RemoveAttribute(L"xmlns:xfa"); - pDataXMLNode = pXMLDocumentNode; + pDataXMLNode.Reset(pXMLDocumentNode); } else { - CFX_XMLElement* pDataElement = new CFX_XMLElement(L"xfa:data"); + auto pDataElement = pdfium::MakeUnique(L"xfa:data"); CFX_XMLNode* pParentXMLNode = pXMLDocumentNode->GetParent(); if (pParentXMLNode) pParentXMLNode->RemoveChildNode(pXMLDocumentNode); @@ -741,29 +741,24 @@ CXFA_Node* CXFA_SimpleParser::ParseAsXDPPacket_Data( ->RemoveAttribute(L"xmlns:xfa"); } pDataElement->AppendChild(pXMLDocumentNode); - pDataXMLNode = pDataElement; + pDataXMLNode.Reset(std::move(pDataElement)); } + if (!pDataXMLNode) + return nullptr; - if (pDataXMLNode) { - CXFA_Node* pNode = m_pFactory->CreateNode(XFA_PacketType::Datasets, - XFA_Element::DataGroup); - if (!pNode) { - if (pDataXMLNode != pXMLDocumentNode) - delete pDataXMLNode; - return nullptr; - } - WideString wsLocalName = - static_cast(pDataXMLNode)->GetLocalTagName(); - pNode->JSObject()->SetCData(XFA_Attribute::Name, wsLocalName, false, false); - if (!DataLoader(pNode, pDataXMLNode, true)) - return nullptr; + CXFA_Node* pNode = + m_pFactory->CreateNode(XFA_PacketType::Datasets, XFA_Element::DataGroup); + if (!pNode) + return nullptr; - pNode->SetXMLMappingNode(pDataXMLNode); - if (pDataXMLNode != pXMLDocumentNode) - pNode->SetFlag(XFA_NodeFlag_OwnXMLNode); - return pNode; - } - return nullptr; + WideString wsLocalName = + static_cast(pDataXMLNode.Get())->GetLocalTagName(); + pNode->JSObject()->SetCData(XFA_Attribute::Name, wsLocalName, false, false); + if (!DataLoader(pNode, pDataXMLNode.Get(), true)) + return nullptr; + + pNode->SetXMLMappingNode(std::move(pDataXMLNode)); + return pNode; } CXFA_Node* CXFA_SimpleParser::ParseAsXDPPacket_LocaleConnectionSourceSet( diff --git a/xfa/fxfa/parser/xfa_document_datamerger_imp.cpp b/xfa/fxfa/parser/xfa_document_datamerger_imp.cpp index 56508634c8..b7cab02b3d 100644 --- a/xfa/fxfa/parser/xfa_document_datamerger_imp.cpp +++ b/xfa/fxfa/parser/xfa_document_datamerger_imp.cpp @@ -7,6 +7,7 @@ #include "xfa/fxfa/parser/xfa_document_datamerger_imp.h" #include +#include #include #include "core/fxcrt/fx_extension.h" @@ -1368,17 +1369,21 @@ CXFA_Node* CXFA_Document::GetNotBindNode( void CXFA_Document::DoDataMerge() { CXFA_Node* pDatasetsRoot = ToNode(GetXFAObject(XFA_HASHCODE_Datasets)); if (!pDatasetsRoot) { - CFX_XMLElement* pDatasetsXMLNode = new CFX_XMLElement(L"xfa:datasets"); + // Ownership will be passed in the AppendChild below to the XML tree. + auto pDatasetsXMLNode = pdfium::MakeUnique(L"xfa:datasets"); pDatasetsXMLNode->SetString(L"xmlns:xfa", L"http://www.xfa.org/schema/xfa-data/1.0/"); pDatasetsRoot = CreateNode(XFA_PacketType::Datasets, XFA_Element::DataModel); pDatasetsRoot->JSObject()->SetCData(XFA_Attribute::Name, L"datasets", false, false); - m_pRootNode->GetXMLMappingNode()->AppendChild(pDatasetsXMLNode); + + CFX_XMLElement* ref = pDatasetsXMLNode.get(); + m_pRootNode->GetXMLMappingNode()->AppendChild(pDatasetsXMLNode.release()); m_pRootNode->InsertChild(pDatasetsRoot, nullptr); - pDatasetsRoot->SetXMLMappingNode(pDatasetsXMLNode); + pDatasetsRoot->SetXMLMappingNode(ref); } + CXFA_Node *pDataRoot = nullptr, *pDDRoot = nullptr; WideString wsDatasetsURI = pDatasetsRoot->JSObject()->TryNamespace().value_or(WideString()); @@ -1407,11 +1412,10 @@ void CXFA_Document::DoDataMerge() { } if (!pDataRoot) { - CFX_XMLElement* pDataRootXMLNode = new CFX_XMLElement(L"xfa:data"); pDataRoot = CreateNode(XFA_PacketType::Datasets, XFA_Element::DataGroup); pDataRoot->JSObject()->SetCData(XFA_Attribute::Name, L"data", false, false); - pDataRoot->SetXMLMappingNode(pDataRootXMLNode); - pDataRoot->SetFlag(XFA_NodeFlag_OwnXMLNode); + pDataRoot->SetXMLMappingNode( + pdfium::MakeUnique(L"xfa:data")); pDatasetsRoot->InsertChild(pDataRoot, nullptr); } @@ -1460,14 +1464,14 @@ void CXFA_Document::DoDataMerge() { WideString wsFormName = pSubformSetNode->JSObject()->GetCData(XFA_Attribute::Name); WideString wsDataTopLevelName(wsFormName.IsEmpty() ? L"form" : wsFormName); - CFX_XMLElement* pDataTopLevelXMLNode = - new CFX_XMLElement(wsDataTopLevelName); pDataTopLevel = static_cast( CreateNode(XFA_PacketType::Datasets, XFA_Element::DataGroup)); pDataTopLevel->JSObject()->SetCData(XFA_Attribute::Name, wsDataTopLevelName, false, false); - pDataTopLevel->SetXMLMappingNode(pDataTopLevelXMLNode); + pDataTopLevel->SetXMLMappingNode( + pdfium::MakeUnique(wsDataTopLevelName)); + CXFA_Node* pBeforeNode = pDataRoot->GetFirstChild(); pDataRoot->InsertChild(pDataTopLevel, pBeforeNode); } -- cgit v1.2.3