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 <hnakashima@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

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<CFX_XMLText>(wsText));
 }
 
 void CFX_XMLElement::Save(const RetainPtr<IFX_SeekableStream>& 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 <utility>
 #include <vector>
 
 #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<CFX_XMLNode> pNode) {
+  InsertChildNode(std::move(pNode), -1);
 }
 
-void CFX_XMLNode::InsertChildNode(CFX_XMLNode* pNode, int32_t index) {
+void CFX_XMLNode::InsertChildNode(std::unique_ptr<CFX_XMLNode> 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<IFX_SeekableStream>& 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<CFX_XMLNode> pNode);
+  void InsertChildNode(std::unique_ptr<CFX_XMLNode> 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<CFX_XMLNode> parent_;
+  UnownedPtr<CFX_XMLNode> last_child_;
+  UnownedPtr<CFX_XMLNode> prev_sibling_;
+  std::unique_ptr<CFX_XMLNode> first_child_;
+  std::unique_ptr<CFX_XMLNode> 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 <algorithm>
 #include <cwctype>
 #include <iterator>
+#include <utility>
 
 #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<CFX_XMLInstruction>(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<CFX_XMLElement>(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<CFX_XMLText>(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<CFX_XMLCharData>(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<CFX_XMLNode>(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<CFX_XMLNode>(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 <set>
+#include <utility>
 
 #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<CFX_XMLNode> CXFA_DocumentParser::LoadXML(
   ASSERT(pStream);
 
   auto root = pdfium::MakeUnique<CFX_XMLNode>();
-  root->AppendChild(new CFX_XMLInstruction(L"xml"));
+  root->AppendChild(pdfium::MakeUnique<CFX_XMLInstruction>(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<CFX_XMLElement*>(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<CFX_XMLNode>(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) {