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>

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);
   }