Fix CFX_XML and add unit tests

This CL fixes several issues in the CFX_XML class and
adds unit tests.

Change-Id: I05270690de8f3c45dceb866e17ef899ae6d23389
Reviewed-on: https://pdfium-review.googlesource.com/31753
Commit-Queue: dsinclair <dsinclair@chromium.org>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>

9 files changed, 709 insertions, 9 deletions

diff --git a/core/fxcrt/xml/cfx_xmlchardata_unittest.cpp b/core/fxcrt/xml/cfx_xmlchardata_unittest.cpp
new file mode 100644
index 0000000000..0fa48e520f
--- /dev/null
+++ b/core/fxcrt/xml/cfx_xmlchardata_unittest.cpp
@@ -0,0 +1,34 @@
+// Copyright 2018 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/fxcrt/xml/cfx_xmlchardata.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/string_write_stream.h"
+#include "testing/test_support.h"
+
+TEST(CFX_XMLCharDataTest, GetType) {
+  CFX_XMLCharData data(L"My Data");
+  EXPECT_EQ(FX_XMLNODE_CharData, data.GetType());
+}
+
+TEST(CFX_XMLCharDataTest, GetText) {
+  CFX_XMLCharData data(L"My Data");
+  EXPECT_EQ(L"My Data", data.GetText());
+}
+
+TEST(CFX_XMLCharDataTest, Clone) {
+  CFX_XMLCharData data(L"My Data");
+  auto clone = data.Clone();
+  EXPECT_TRUE(clone != nullptr);
+  EXPECT_NE(&data, clone.get());
+  ASSERT_EQ(FX_XMLNODE_CharData, clone->GetType());
+  EXPECT_EQ(L"My Data", static_cast<CFX_XMLCharData*>(clone.get())->GetText());
+}
+
+TEST(CFX_XMLCharDataTest, Save) {
+  auto stream = pdfium::MakeRetain<StringWriteStream>();
+  CFX_XMLCharData data(L"My Data");
+  data.Save(stream);
+  EXPECT_EQ("<![CDATA[My Data]]>", stream->ToString());
+}
diff --git a/core/fxcrt/xml/cfx_xmlelement_unittest.cpp b/core/fxcrt/xml/cfx_xmlelement_unittest.cpp
new file mode 100644
index 0000000000..79e067f511
--- /dev/null
+++ b/core/fxcrt/xml/cfx_xmlelement_unittest.cpp
@@ -0,0 +1,217 @@
+// Copyright 2018 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <utility>
+
+#include "core/fxcrt/xml/cfx_xmlchardata.h"
+#include "core/fxcrt/xml/cfx_xmlelement.h"
+#include "core/fxcrt/xml/cfx_xmltext.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/string_write_stream.h"
+#include "testing/test_support.h"
+#include "third_party/base/ptr_util.h"
+
+TEST(CFX_XMLElementTest, GetType) {
+  CFX_XMLElement node(L"node");
+  EXPECT_EQ(FX_XMLNODE_Element, node.GetType());
+}
+
+TEST(CFX_XMLElementTest, GetName) {
+  CFX_XMLElement node(L"node");
+  EXPECT_EQ(L"node", node.GetName());
+}
+
+TEST(CFX_XMLElementTest, GetLocalTagName) {
+  CFX_XMLElement node1(L"node1");
+  EXPECT_EQ(L"node1", node1.GetLocalTagName());
+
+  CFX_XMLElement node2(L"test:node2");
+  EXPECT_EQ(L"node2", node2.GetLocalTagName());
+}
+
+TEST(CFX_XMLElementTest, GetNamespacePrefix) {
+  CFX_XMLElement node1(L"node1");
+  EXPECT_EQ(L"", node1.GetNamespacePrefix());
+
+  CFX_XMLElement node2(L"test:node2");
+  EXPECT_EQ(L"test", node2.GetNamespacePrefix());
+}
+
+TEST(CFX_XMLElementTest, GetNamespaceURI) {
+  CFX_XMLElement node1(L"node1");
+  EXPECT_EQ(L"", node1.GetNamespaceURI());
+
+  node1.SetAttribute(L"xmlns", L"https://example.org/ns1");
+  EXPECT_EQ(L"https://example.org/ns1", node1.GetNamespaceURI());
+
+  CFX_XMLElement node2(L"test:node2");
+  EXPECT_EQ(L"", node2.GetNamespaceURI());
+
+  node2.SetAttribute(L"xmlns", L"https://example.org/ns2");
+  EXPECT_EQ(L"", node2.GetNamespaceURI());
+
+  node2.SetAttribute(L"xmlns:test", L"https://example.org/ns2");
+  EXPECT_EQ(L"https://example.org/ns2", node2.GetNamespaceURI());
+}
+
+TEST(CFX_XMLElementTest, Attributes) {
+  CFX_XMLElement node(L"test:node");
+  node.SetAttribute(L"first", L"one");
+  node.SetAttribute(L"second", L"two");
+
+  ASSERT_TRUE(node.HasAttribute(L"first"));
+  EXPECT_EQ(L"one", node.GetAttribute(L"first"));
+  ASSERT_TRUE(node.HasAttribute(L"second"));
+  EXPECT_EQ(L"two", node.GetAttribute(L"second"));
+
+  ASSERT_EQ(2U, node.GetAttributes().size());
+
+  node.RemoveAttribute(L"first");
+  EXPECT_FALSE(node.HasAttribute(L"first"));
+
+  ASSERT_EQ(1U, node.GetAttributes().size());
+}
+
+TEST(CFX_XMLElementTest, Clone) {
+  CFX_XMLElement node(L"test:node");
+  node.SetAttribute(L"first", L"one");
+  node.SetAttribute(L"second", L"two");
+  node.SetAttribute(L"xmlns:test", L"https://example.org/test");
+
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLText>(L"Text Child"));
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLElement>(L"Node child"));
+
+  auto clone = node.Clone();
+  EXPECT_TRUE(clone != nullptr);
+
+  ASSERT_EQ(FX_XMLNODE_Element, clone->GetType());
+  CFX_XMLElement* inst = static_cast<CFX_XMLElement*>(clone.get());
+
+  EXPECT_EQ(L"test:node", inst->GetName());
+  EXPECT_EQ(L"node", inst->GetLocalTagName());
+  EXPECT_EQ(L"test", inst->GetNamespacePrefix());
+  EXPECT_EQ(L"https://example.org/test", inst->GetNamespaceURI());
+
+  ASSERT_TRUE(inst->HasAttribute(L"first"));
+  EXPECT_EQ(L"one", inst->GetAttribute(L"first"));
+  ASSERT_TRUE(inst->HasAttribute(L"second"));
+  EXPECT_EQ(L"two", inst->GetAttribute(L"second"));
+
+  // Only clone the Text node, so expect only one child.
+  ASSERT_TRUE(inst->GetFirstChild() != nullptr);
+  EXPECT_TRUE(inst->GetFirstChild()->GetNextSibling() == nullptr);
+
+  ASSERT_EQ(FX_XMLNODE_Text, inst->GetFirstChild()->GetType());
+  auto* text = static_cast<CFX_XMLText*>(inst->GetFirstChild());
+  EXPECT_EQ(L"Text Child", text->GetText());
+}
+
+TEST(CFX_XMLElementTest, Save) {
+  auto stream = pdfium::MakeRetain<StringWriteStream>();
+  CFX_XMLElement node(L"root");
+
+  node.Save(stream);
+  EXPECT_EQ("<root />\n", stream->ToString());
+}
+
+TEST(CFX_XMLElementTest, SaveWithAttributes) {
+  auto stream = pdfium::MakeRetain<StringWriteStream>();
+  CFX_XMLElement node(L"root");
+  node.SetAttribute(L"first", L"one");
+  node.SetAttribute(L"second", L"two");
+
+  node.Save(stream);
+  EXPECT_EQ("<root first=\"one\" second=\"two\" />\n", stream->ToString());
+}
+
+TEST(CFX_XMLElementTest, SaveWithChildren) {
+  auto stream = pdfium::MakeRetain<StringWriteStream>();
+  CFX_XMLElement node(L"node");
+
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLText>(L"Text Child 1"));
+
+  auto child = pdfium::MakeUnique<CFX_XMLElement>(L"node-child");
+  CFX_XMLElement* node_child1 = child.get();
+  node.AppendChild(std::move(child));
+
+  node_child1->AppendChild(pdfium::MakeUnique<CFX_XMLText>(L"Text Child 2"));
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLCharData>(L"Char Data"));
+
+  node.Save(stream);
+  EXPECT_EQ(
+      "<node>\n"
+      "Text Child 1"
+      "<node-child>\nText Child 2</node-child>\n"
+      "<![CDATA[Char Data]]>"
+      "</node>\n",
+      stream->ToString());
+}
+
+TEST(CFX_XMLElementTest, SaveWithNamespace) {
+  auto stream = pdfium::MakeRetain<StringWriteStream>();
+  CFX_XMLElement node(L"test:root");
+  node.SetAttribute(L"xmlns:test", L"https://example.org/ns1");
+
+  node.Save(stream);
+  EXPECT_EQ("<test:root xmlns:test=\"https://example.org/ns1\" />\n",
+            stream->ToString());
+}
+
+TEST(CFX_XMLElementTest, GetFirstChildNamed) {
+  CFX_XMLElement node(L"node");
+  auto child = pdfium::MakeUnique<CFX_XMLElement>(L"node-child");
+  CFX_XMLElement* node_child1 = child.get();
+  node.AppendChild(std::move(child));
+
+  auto* found = node.GetFirstChildNamed(L"node-child");
+  EXPECT_TRUE(found != nullptr);
+  EXPECT_EQ(node_child1, found);
+}
+
+TEST(CFX_XMLElementTest, GetFirstChildNamedMissing) {
+  CFX_XMLElement node(L"node");
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLElement>(L"node-child"));
+
+  auto* found = node.GetFirstChildNamed(L"node-sibling");
+  EXPECT_TRUE(found == nullptr);
+}
+
+TEST(CFX_XMLElementTest, GetNthChildNamed) {
+  CFX_XMLElement node(L"node");
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLElement>(L"node-child"));
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLElement>(L"node-child"));
+
+  auto child = pdfium::MakeUnique<CFX_XMLElement>(L"node-child");
+  CFX_XMLElement* node_child3 = child.get();
+  node.AppendChild(std::move(child));
+
+  auto* found = node.GetNthChildNamed(L"node-child", 2);
+  EXPECT_TRUE(found != nullptr);
+  EXPECT_EQ(node_child3, found);
+}
+
+TEST(CFX_XMLElementTest, GetNthChildNamedMissingChild) {
+  CFX_XMLElement node(L"node");
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLElement>(L"node-child"));
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLElement>(L"node-child"));
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLElement>(L"node-child"));
+
+  auto* found = node.GetNthChildNamed(L"node-child", 5);
+  EXPECT_TRUE(found == nullptr);
+}
+
+TEST(CFX_XMLElementTest, GetTextData) {
+  CFX_XMLElement node(L"node");
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLText>(L"Text Child 1"));
+
+  auto child = pdfium::MakeUnique<CFX_XMLElement>(L"Node child");
+  CFX_XMLElement* node_child1 = child.get();
+  node.AppendChild(std::move(child));
+
+  node_child1->AppendChild(pdfium::MakeUnique<CFX_XMLText>(L"Text Child 2"));
+
+  node.AppendChild(pdfium::MakeUnique<CFX_XMLCharData>(L"Char Data"));
+
+  EXPECT_EQ(L"Text Child 1Char Data", node.GetTextData());
+}
diff --git a/core/fxcrt/xml/cfx_xmlinstruction.cpp b/core/fxcrt/xml/cfx_xmlinstruction.cpp
index d0f5cbb68b..b7a87b588a 100644
--- a/core/fxcrt/xml/cfx_xmlinstruction.cpp
+++ b/core/fxcrt/xml/cfx_xmlinstruction.cpp
@@ -49,12 +49,12 @@ void CFX_XMLInstruction::Save(
 
   pXMLStream->WriteString("<?");
   pXMLStream->WriteString(name_.UTF8Encode().AsStringView());
+  pXMLStream->WriteString(" ");
 
   for (const WideString& target : m_TargetData) {
-    pXMLStream->WriteString("\"");
     pXMLStream->WriteString(target.UTF8Encode().AsStringView());
-    pXMLStream->WriteString("\"");
+    pXMLStream->WriteString(" ");
   }
 
-  pXMLStream->WriteString("?>");
+  pXMLStream->WriteString("?>\n");
 }
diff --git a/core/fxcrt/xml/cfx_xmlinstruction_unittest.cpp b/core/fxcrt/xml/cfx_xmlinstruction_unittest.cpp
new file mode 100644
index 0000000000..c60d401303
--- /dev/null
+++ b/core/fxcrt/xml/cfx_xmlinstruction_unittest.cpp
@@ -0,0 +1,111 @@
+// Copyright 2018 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/fxcrt/xml/cfx_xmlinstruction.h"
+#include "core/fxcrt/cfx_memorystream.h"
+#include "core/fxcrt/xml/cfx_xmlelement.h"
+#include "core/fxcrt/xml/cfx_xmlparser.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/string_write_stream.h"
+#include "testing/test_support.h"
+
+TEST(CFX_XMLInstructionTest, GetType) {
+  CFX_XMLInstruction node(L"acrobat");
+  EXPECT_EQ(FX_XMLNODE_Instruction, node.GetType());
+}
+
+TEST(CFX_XMLInstructionTest, AcrobatInstruction) {
+  CFX_XMLInstruction node(L"acrobat");
+  EXPECT_TRUE(node.IsAcrobat());
+  EXPECT_FALSE(node.IsOriginalXFAVersion());
+}
+
+TEST(CFX_XMLInstructionTest, OriginalXFAInstruction) {
+  CFX_XMLInstruction node(L"originalXFAVersion");
+  EXPECT_TRUE(node.IsOriginalXFAVersion());
+  EXPECT_FALSE(node.IsAcrobat());
+}
+
+TEST(CFX_XMLInstructionTest, TargetData) {
+  CFX_XMLInstruction node(L"acrobat");
+  EXPECT_EQ(0U, node.GetTargetData().size());
+
+  node.AppendData(L"firstString");
+  node.AppendData(L"secondString");
+
+  auto& data = node.GetTargetData();
+  ASSERT_EQ(2U, data.size());
+  EXPECT_EQ(L"firstString", data[0]);
+  EXPECT_EQ(L"secondString", data[1]);
+}
+
+TEST(CFX_XMLInstructionTest, Clone) {
+  CFX_XMLInstruction node(L"acrobat");
+  node.AppendData(L"firstString");
+  node.AppendData(L"secondString");
+
+  auto clone = node.Clone();
+  EXPECT_TRUE(clone != nullptr);
+
+  ASSERT_EQ(FX_XMLNODE_Instruction, clone->GetType());
+  CFX_XMLInstruction* inst = static_cast<CFX_XMLInstruction*>(clone.get());
+
+  EXPECT_TRUE(inst->IsAcrobat());
+
+  auto& data = inst->GetTargetData();
+  ASSERT_EQ(2U, data.size());
+  EXPECT_EQ(L"firstString", data[0]);
+  EXPECT_EQ(L"secondString", data[1]);
+}
+
+TEST(CFX_XMLInstructionTest, SaveXML) {
+  auto stream = pdfium::MakeRetain<StringWriteStream>();
+  CFX_XMLInstruction node(L"xml");
+  node.Save(stream);
+  EXPECT_EQ("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n", stream->ToString());
+}
+
+TEST(CFX_XMLInstructionTest, SaveAcrobat) {
+  auto stream = pdfium::MakeRetain<StringWriteStream>();
+  CFX_XMLInstruction node(L"acrobat");
+  node.AppendData(L"http://www.xfa.org/schema/xfa-template/3.3/");
+  node.AppendData(L"Display:1");
+
+  node.Save(stream);
+  EXPECT_EQ(
+      "<?acrobat http://www.xfa.org/schema/xfa-template/3.3/ Display:1 ?>\n",
+      stream->ToString());
+}
+
+TEST(CFX_XMLInstructionTest, ParseAndReSave) {
+  const char* input =
+      "<?acrobat http://www.xfa.org/schema/xfa-template/3.3/ Display:1 ?>\n"
+      "<node></node>";
+
+  auto in_stream = pdfium::MakeRetain<CFX_MemoryStream>(
+      reinterpret_cast<uint8_t*>(const_cast<char*>(input)), strlen(input),
+      false);
+
+  CFX_XMLElement root(L"root");
+  CFX_XMLParser parser(&root, in_stream);
+  ASSERT_TRUE(parser.Parse());
+  ASSERT_TRUE(root.GetFirstChild() != nullptr);
+  ASSERT_EQ(FX_XMLNODE_Instruction, root.GetFirstChild()->GetType());
+
+  CFX_XMLInstruction* node =
+      static_cast<CFX_XMLInstruction*>(root.GetFirstChild());
+  ASSERT_TRUE(node != nullptr);
+  EXPECT_TRUE(node->IsAcrobat());
+
+  auto& data = node->GetTargetData();
+  ASSERT_EQ(2U, data.size());
+  EXPECT_EQ(L"http://www.xfa.org/schema/xfa-template/3.3/", data[0]);
+  EXPECT_EQ(L"Display:1", data[1]);
+
+  auto out_stream = pdfium::MakeRetain<StringWriteStream>();
+  node->Save(out_stream);
+  EXPECT_EQ(
+      "<?acrobat http://www.xfa.org/schema/xfa-template/3.3/ Display:1 ?>\n",
+      out_stream->ToString());
+}
diff --git a/core/fxcrt/xml/cfx_xmlnode.cpp b/core/fxcrt/xml/cfx_xmlnode.cpp
index 4f811e608e..088cbf367c 100644
--- a/core/fxcrt/xml/cfx_xmlnode.cpp
+++ b/core/fxcrt/xml/cfx_xmlnode.cpp
@@ -27,8 +27,18 @@ void CFX_XMLNode::DeleteChildren() {
   last_child_ = nullptr;
   while (child) {
     child = child->prev_sibling_.Get();
-    if (child)
+    if (child) {
+      if (child->next_sibling_) {
+        child->next_sibling_->prev_sibling_ = nullptr;
+        child->next_sibling_->parent_ = nullptr;
+      }
+
       child->next_sibling_ = nullptr;
+    }
+  }
+  if (first_child_) {
+    first_child_->next_sibling_ = nullptr;
+    first_child_->parent_ = nullptr;
   }
   first_child_ = nullptr;
 }
@@ -82,21 +92,30 @@ void CFX_XMLNode::RemoveChildNode(CFX_XMLNode* pNode) {
   ASSERT(first_child_);
   ASSERT(pNode);
 
+  if (pNode->GetParent() != this)
+    return;
+
   if (first_child_.get() == pNode) {
     first_child_.release();
     first_child_ = std::move(pNode->next_sibling_);
+    if (first_child_) {
+      first_child_->prev_sibling_ = nullptr;
+
+      if (first_child_->next_sibling_)
+        first_child_->next_sibling_->prev_sibling_ = first_child_.get();
+    }
   } else {
     CFX_XMLNode* prev = pNode->prev_sibling_.Get();
     prev->next_sibling_.release();  // Release pNode
     prev->next_sibling_ = std::move(pNode->next_sibling_);
+
+    if (prev->next_sibling_)
+      prev->next_sibling_->prev_sibling_ = prev;
   }
 
   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->prev_sibling_ = nullptr;
 }
diff --git a/core/fxcrt/xml/cfx_xmlnode.h b/core/fxcrt/xml/cfx_xmlnode.h
index ec731eb62e..4c98a3b51a 100644
--- a/core/fxcrt/xml/cfx_xmlnode.h
+++ b/core/fxcrt/xml/cfx_xmlnode.h
@@ -38,6 +38,9 @@ class CFX_XMLNode {
   void RemoveChildNode(CFX_XMLNode* pNode);
   void DeleteChildren();
 
+  CFX_XMLNode* GetLastChildForTesting() const { return last_child_.Get(); }
+  CFX_XMLNode* GetPrevSiblingForTesting() const { return prev_sibling_.Get(); }
+
  protected:
   WideString EncodeEntities(const WideString& value);
 
diff --git a/core/fxcrt/xml/cfx_xmlnode_unittest.cpp b/core/fxcrt/xml/cfx_xmlnode_unittest.cpp
new file mode 100644
index 0000000000..1c69069942
--- /dev/null
+++ b/core/fxcrt/xml/cfx_xmlnode_unittest.cpp
@@ -0,0 +1,275 @@
+// Copyright 2018 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <utility>
+
+#include "core/fxcrt/xml/cfx_xmlelement.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/test_support.h"
+#include "third_party/base/ptr_util.h"
+
+TEST(CFX_XMLNodeTest, GetParent) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+  auto child3 = pdfium::MakeUnique<CFX_XMLElement>(L"node3");
+
+  CFX_XMLElement* node2 = child2.get();
+  CFX_XMLElement* node3 = child3.get();
+
+  node1->AppendChild(std::move(child2));
+  node2->AppendChild(std::move(child3));
+
+  EXPECT_EQ(nullptr, node1->GetParent());
+  EXPECT_EQ(node1.get(), node2->GetParent());
+  EXPECT_EQ(node2, node3->GetParent());
+}
+
+TEST(CFX_XMLNodeTest, GetRoot) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+  auto child3 = pdfium::MakeUnique<CFX_XMLElement>(L"node3");
+
+  CFX_XMLElement* node2 = child2.get();
+  CFX_XMLElement* node3 = child3.get();
+
+  node1->AppendChild(std::move(child2));
+  node2->AppendChild(std::move(child3));
+
+  EXPECT_EQ(node1.get(), node1->GetRoot());
+  EXPECT_EQ(node1.get(), node2->GetRoot());
+  EXPECT_EQ(node1.get(), node3->GetRoot());
+}
+
+TEST(CFX_XMLNodeTest, GetChildren) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+  auto child3 = pdfium::MakeUnique<CFX_XMLElement>(L"node3");
+  auto child4 = pdfium::MakeUnique<CFX_XMLElement>(L"node4");
+
+  CFX_XMLElement* node2 = child2.get();
+  CFX_XMLElement* node3 = child3.get();
+  CFX_XMLElement* node4 = child4.get();
+
+  node1->AppendChild(std::move(child2));
+  node1->AppendChild(std::move(child4));
+  node2->AppendChild(std::move(child3));
+
+  EXPECT_EQ(node2, node1->GetFirstChild());
+
+  EXPECT_EQ(node4, node2->GetNextSibling());
+  EXPECT_EQ(node3, node2->GetFirstChild());
+
+  EXPECT_TRUE(node3->GetNextSibling() == nullptr);
+  EXPECT_TRUE(node3->GetFirstChild() == nullptr);
+
+  EXPECT_TRUE(node4->GetNextSibling() == nullptr);
+  EXPECT_TRUE(node4->GetFirstChild() == nullptr);
+}
+
+TEST(CFX_XMLNodeTest, DeleteChildren) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+  auto child3 = pdfium::MakeUnique<CFX_XMLElement>(L"node3");
+  auto child4 = pdfium::MakeUnique<CFX_XMLElement>(L"node4");
+
+  CFX_XMLElement* node2 = child2.get();
+  // CFX_XMLElement* node3 = child3.get();
+  // CFX_XMLElement* node4 = child4.get();
+
+  node1->AppendChild(std::move(child2));
+  node1->AppendChild(std::move(child4));
+  node2->AppendChild(std::move(child3));
+
+  node1->DeleteChildren();
+  EXPECT_TRUE(node1->GetFirstChild() == nullptr);
+  EXPECT_TRUE(node1->GetLastChildForTesting() == nullptr);
+
+  // TODO(dsinclair): This isn't true currently but will be true when
+  // we own the nodes in an XML document. (Currently nodes are unique_ptrs
+  // so the objects have been deleted by this point.)
+
+  // EXPECT_TRUE(node2->GetParent() == nullptr);
+  // EXPECT_TRUE(node4->GetParent() == nullptr);
+
+  // // node2 and node4 should no longer be siblings.
+  // EXPECT_TRUE(node2->GetNextSibling() == nullptr);
+  // EXPECT_TRUE(node4->GetPrevSiblingForTesting() == nullptr);
+
+  // Deleting children doesn't change deleted substructure
+  // EXPECT_EQ(node3, node2->GetFirstChild());
+  // EXPECT_TRUE(node3->GetParent() == node2);
+}
+
+TEST(CFX_XMLNodeTest, AddingChildren) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+  auto child3 = pdfium::MakeUnique<CFX_XMLElement>(L"node3");
+  auto child4 = pdfium::MakeUnique<CFX_XMLElement>(L"node4");
+  auto child5 = pdfium::MakeUnique<CFX_XMLElement>(L"node5");
+
+  CFX_XMLElement* node2 = child2.get();
+  CFX_XMLElement* node3 = child3.get();
+  CFX_XMLElement* node4 = child4.get();
+  CFX_XMLElement* node5 = child5.get();
+
+  node1->AppendChild(std::move(child2));
+  node1->AppendChild(std::move(child3));
+
+  EXPECT_EQ(node1.get(), node2->GetParent());
+  EXPECT_EQ(node1.get(), node3->GetParent());
+
+  EXPECT_EQ(node2, node1->GetFirstChild());
+  EXPECT_EQ(node3, node2->GetNextSibling());
+  EXPECT_TRUE(node3->GetNextSibling() == nullptr);
+
+  // Insert to negative appends.
+  node1->InsertChildNode(std::move(child4), -1);
+  EXPECT_EQ(node1.get(), node4->GetParent());
+  EXPECT_EQ(node4, node3->GetNextSibling());
+  EXPECT_TRUE(node4->GetNextSibling() == nullptr);
+
+  node1->InsertChildNode(std::move(child5), 1);
+  EXPECT_EQ(node1.get(), node5->GetParent());
+  EXPECT_EQ(node2, node1->GetFirstChild());
+  EXPECT_EQ(node5, node2->GetNextSibling());
+  EXPECT_EQ(node3, node5->GetNextSibling());
+  EXPECT_EQ(node4, node3->GetNextSibling());
+  EXPECT_TRUE(node4->GetNextSibling() == nullptr);
+}
+
+TEST(CFX_XMLNodeTest, RemovingMiddleChild) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+  auto child3 = pdfium::MakeUnique<CFX_XMLElement>(L"node3");
+  auto child4 = pdfium::MakeUnique<CFX_XMLElement>(L"node4");
+
+  CFX_XMLElement* node2 = child2.get();
+  CFX_XMLElement* node3 = child3.get();
+  CFX_XMLElement* node4 = child4.get();
+
+  node1->AppendChild(std::move(child2));
+  node1->AppendChild(std::move(child3));
+  node1->AppendChild(std::move(child4));
+
+  EXPECT_EQ(node2, node1->GetFirstChild());
+  EXPECT_EQ(node3, node2->GetNextSibling());
+  EXPECT_EQ(node4, node3->GetNextSibling());
+  EXPECT_TRUE(node4->GetNextSibling() == nullptr);
+
+  node1->RemoveChildNode(node3);
+  // Node is released by parent, so need to take ownership
+  child3 = pdfium::WrapUnique(node3);
+
+  EXPECT_TRUE(node3->GetParent() == nullptr);
+  EXPECT_TRUE(node3->GetNextSibling() == nullptr);
+  EXPECT_TRUE(node3->GetPrevSiblingForTesting() == nullptr);
+
+  EXPECT_EQ(node2, node1->GetFirstChild());
+  EXPECT_EQ(node4, node2->GetNextSibling());
+  EXPECT_EQ(node2, node4->GetPrevSiblingForTesting());
+  EXPECT_TRUE(node4->GetNextSibling() == nullptr);
+}
+
+TEST(CFX_XMLNodeTest, RemovingFirstChild) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+  auto child3 = pdfium::MakeUnique<CFX_XMLElement>(L"node3");
+  auto child4 = pdfium::MakeUnique<CFX_XMLElement>(L"node4");
+
+  CFX_XMLElement* node2 = child2.get();
+  CFX_XMLElement* node3 = child3.get();
+  CFX_XMLElement* node4 = child4.get();
+
+  node1->AppendChild(std::move(child2));
+  node1->AppendChild(std::move(child3));
+  node1->AppendChild(std::move(child4));
+
+  EXPECT_EQ(node2, node1->GetFirstChild());
+  EXPECT_EQ(node3, node2->GetNextSibling());
+  EXPECT_EQ(node4, node3->GetNextSibling());
+  EXPECT_TRUE(node4->GetNextSibling() == nullptr);
+
+  node1->RemoveChildNode(node2);
+  // Node is released by parent, so need to take ownership
+  child2 = pdfium::WrapUnique(node2);
+
+  EXPECT_TRUE(node2->GetParent() == nullptr);
+  EXPECT_TRUE(node2->GetNextSibling() == nullptr);
+  EXPECT_TRUE(node2->GetPrevSiblingForTesting() == nullptr);
+
+  EXPECT_EQ(node3, node1->GetFirstChild());
+  EXPECT_TRUE(node3->GetPrevSiblingForTesting() == nullptr);
+  EXPECT_EQ(node4, node3->GetNextSibling());
+  EXPECT_TRUE(node4->GetNextSibling() == nullptr);
+}
+
+TEST(CFX_XMLNodeTest, RemovingLastChild) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+  auto child3 = pdfium::MakeUnique<CFX_XMLElement>(L"node3");
+  auto child4 = pdfium::MakeUnique<CFX_XMLElement>(L"node4");
+
+  CFX_XMLElement* node2 = child2.get();
+  CFX_XMLElement* node3 = child3.get();
+  CFX_XMLElement* node4 = child4.get();
+
+  node1->AppendChild(std::move(child2));
+  node1->AppendChild(std::move(child3));
+  node1->AppendChild(std::move(child4));
+
+  EXPECT_EQ(node2, node1->GetFirstChild());
+  EXPECT_EQ(node3, node2->GetNextSibling());
+  EXPECT_EQ(node4, node3->GetNextSibling());
+  EXPECT_TRUE(node4->GetNextSibling() == nullptr);
+
+  node1->RemoveChildNode(node4);
+  // Node is released by parent, so need to take ownership
+  child4 = pdfium::WrapUnique(node4);
+
+  EXPECT_TRUE(node4->GetParent() == nullptr);
+  EXPECT_TRUE(node4->GetNextSibling() == nullptr);
+  EXPECT_TRUE(node4->GetPrevSiblingForTesting() == nullptr);
+
+  EXPECT_EQ(node2, node1->GetFirstChild());
+  EXPECT_EQ(node3, node2->GetNextSibling());
+  EXPECT_TRUE(node3->GetNextSibling() == nullptr);
+}
+
+TEST(CFX_XMLNodeTest, RemovingOnlyChild) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+
+  CFX_XMLElement* node2 = child2.get();
+
+  node1->AppendChild(std::move(child2));
+
+  EXPECT_EQ(node2, node1->GetFirstChild());
+  EXPECT_TRUE(node2->GetNextSibling() == nullptr);
+
+  node1->RemoveChildNode(node2);
+  // Node is released by parent, so need to take ownership
+  child2 = pdfium::WrapUnique(node2);
+
+  EXPECT_TRUE(node2->GetParent() == nullptr);
+
+  EXPECT_TRUE(node1->GetFirstChild() == nullptr);
+  EXPECT_TRUE(node2->GetNextSibling() == nullptr);
+  EXPECT_TRUE(node2->GetPrevSiblingForTesting() == nullptr);
+}
+
+TEST(CFX_XMLNodeTest, RemoveMissingChild) {
+  auto node1 = pdfium::MakeUnique<CFX_XMLElement>(L"node");
+  auto child2 = pdfium::MakeUnique<CFX_XMLElement>(L"node2");
+  auto child3 = pdfium::MakeUnique<CFX_XMLElement>(L"node3");
+
+  CFX_XMLElement* node2 = child2.get();
+  CFX_XMLElement* node3 = child3.get();
+
+  node1->AppendChild(std::move(child2));
+  node1->RemoveChildNode(node3);
+
+  EXPECT_TRUE(node3->GetParent() == nullptr);
+  EXPECT_EQ(node2, node1->GetFirstChild());
+  EXPECT_TRUE(node2->GetNextSibling() == nullptr);
+}
diff --git a/core/fxcrt/xml/cfx_xmlparser.cpp b/core/fxcrt/xml/cfx_xmlparser.cpp
index eb79637095..cbfb949705 100644
--- a/core/fxcrt/xml/cfx_xmlparser.cpp
+++ b/core/fxcrt/xml/cfx_xmlparser.cpp
@@ -175,8 +175,8 @@ bool CFX_XMLParser::Parse() {
           auto* instruction = static_cast<CFX_XMLInstruction*>(m_pChild);
           if (!target_data.IsEmpty())
             instruction->AppendData(target_data);
-
-          instruction->AppendData(GetTextData());
+          if (!GetTextData().IsEmpty())
+            instruction->AppendData(GetTextData());
         }
         break;
       }
diff --git a/core/fxcrt/xml/cfx_xmltext_unittest.cpp b/core/fxcrt/xml/cfx_xmltext_unittest.cpp
new file mode 100644
index 0000000000..ab977e9546
--- /dev/null
+++ b/core/fxcrt/xml/cfx_xmltext_unittest.cpp
@@ -0,0 +1,41 @@
+// Copyright 2018 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/fxcrt/xml/cfx_xmltext.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/string_write_stream.h"
+#include "testing/test_support.h"
+
+TEST(CFX_XMLTextTest, GetType) {
+  CFX_XMLText text(L"My Text");
+  EXPECT_EQ(FX_XMLNODE_Text, text.GetType());
+}
+
+TEST(CFX_XMLTextTest, GetText) {
+  CFX_XMLText data(L"My Data");
+  EXPECT_EQ(L"My Data", data.GetText());
+}
+
+TEST(CFX_XMLTextTest, Clone) {
+  CFX_XMLText data(L"My Data");
+  auto clone = data.Clone();
+  EXPECT_TRUE(clone != nullptr);
+  ASSERT_EQ(FX_XMLNODE_Text, clone->GetType());
+  EXPECT_EQ(L"My Data", static_cast<CFX_XMLText*>(clone.get())->GetText());
+}
+
+TEST(CFX_XMLTextTest, Save) {
+  auto stream = pdfium::MakeRetain<StringWriteStream>();
+  CFX_XMLText data(L"My Data & this is < and > and ' and \" stuff.");
+  data.Save(stream);
+  EXPECT_EQ("My Data &amp; this is &lt; and &gt; and &apos; and &quot; stuff.",
+            stream->ToString());
+}
+
+TEST(CFX_XMLTextTest, SetText) {
+  CFX_XMLText data(L"My Data");
+  EXPECT_EQ(L"My Data", data.GetText());
+  data.SetText(L"New Text");
+  EXPECT_EQ(L"New Text", data.GetText());
+}