summaryrefslogtreecommitdiff
path: root/xfa
diff options
context:
space:
mode:
authorDan Sinclair <dsinclair@chromium.org>2018-02-15 18:12:29 +0000
committerChromium commit bot <commit-bot@chromium.org>2018-02-15 18:12:29 +0000
commite40678ed8a22ecd57421877af39cf7f281f618c4 (patch)
tree50bbf114829ad3b4852b3739f65c6385091c4ba0 /xfa
parent625e6fec9ddd1d023116f7bd22d2192ef9bc49c8 (diff)
downloadpdfium-e40678ed8a22ecd57421877af39cf7f281f618c4.tar.xz
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 <tsepez@chromium.org> Commit-Queue: dsinclair <dsinclair@chromium.org>
Diffstat (limited to 'xfa')
-rw-r--r--xfa/fxfa/parser/cxfa_document_parser.cpp1
-rw-r--r--xfa/fxfa/parser/cxfa_document_parser.h2
-rw-r--r--xfa/fxfa/parser/cxfa_node.cpp97
-rw-r--r--xfa/fxfa/parser/cxfa_node.h23
-rw-r--r--xfa/fxfa/parser/cxfa_nodeowner.cpp13
-rw-r--r--xfa/fxfa/parser/cxfa_nodeowner.h2
-rw-r--r--xfa/fxfa/parser/cxfa_simple_parser.cpp41
-rw-r--r--xfa/fxfa/parser/xfa_document_datamerger_imp.cpp22
8 files changed, 119 insertions, 82 deletions
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<CXFA_FFNotify> 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<CXFA_Document> 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<CJX_Node>(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<CFX_XMLElement*>(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<CFX_XMLNode>(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<CFX_XMLElement*>(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<CFX_XMLElement>(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<CFX_XMLElement>(
+ 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 <vector>
#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<CFX_XMLNode> node) {
+ xml_node_ = std::move(node);
+ }
+ void SetXMLMappingNode(std::unique_ptr<CFX_XMLNode> 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<CFX_XMLNode> 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<CXFA_Node> 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<CXFA_Node> 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<CFX_XMLNode> pDataXMLNode;
if (MatchNodeName(pXMLDocumentNode, L"data", packet->uri, packet->flags)) {
static_cast<CFX_XMLElement*>(pXMLDocumentNode)
->RemoveAttribute(L"xmlns:xfa");
- pDataXMLNode = pXMLDocumentNode;
+ pDataXMLNode.Reset(pXMLDocumentNode);
} else {
- CFX_XMLElement* pDataElement = new CFX_XMLElement(L"xfa:data");
+ auto pDataElement = pdfium::MakeUnique<CFX_XMLElement>(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<CFX_XMLElement*>(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<CFX_XMLElement*>(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 <map>
+#include <utility>
#include <vector>
#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<CFX_XMLElement>(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<CFX_XMLElement>(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<CXFA_DataGroup*>(
CreateNode(XFA_PacketType::Datasets, XFA_Element::DataGroup));
pDataTopLevel->JSObject()->SetCData(XFA_Attribute::Name, wsDataTopLevelName,
false, false);
- pDataTopLevel->SetXMLMappingNode(pDataTopLevelXMLNode);
+ pDataTopLevel->SetXMLMappingNode(
+ pdfium::MakeUnique<CFX_XMLElement>(wsDataTopLevelName));
+
CXFA_Node* pBeforeNode = pDataRoot->GetFirstChild();
pDataRoot->InsertChild(pDataTopLevel, pBeforeNode);
}