Make ownership of XML document slightly saner.

Remove it from the document builder.

Change-Id: I20d15423546d573b3e9a304756548daf85d571dd
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/71910
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/fxjs/xfa/cjx_node.cpp b/fxjs/xfa/cjx_node.cpp
index 98954f1..642eda8 100644
--- a/fxjs/xfa/cjx_node.cpp
+++ b/fxjs/xfa/cjx_node.cpp
@@ -266,17 +266,19 @@
   if (params.size() >= 3)
     bOverwrite = runtime->ToBoolean(params[2]);
 
-  CFX_XMLParser parser(
-      pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(expression.raw_span()));
+  auto stream =
+      pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(expression.raw_span());
 
+  CFX_XMLParser parser(stream);
+  std::unique_ptr<CFX_XMLDocument> xml_doc = parser.Parse();
   auto builder = std::make_unique<CXFA_DocumentBuilder>(GetDocument());
-  CFX_XMLNode* pXMLNode = builder->Build(parser.Parse());
+  CFX_XMLNode* pXMLNode = builder->Build(xml_doc.get());
   if (!pXMLNode)
     return CJS_Result::Success();
 
   CFX_XMLDocument* top_xml_doc =
       GetXFANode()->GetDocument()->GetNotify()->GetFFDoc()->GetXMLDocument();
-  top_xml_doc->AppendNodesFrom(builder->GetXMLDoc().get());
+  top_xml_doc->AppendNodesFrom(xml_doc.get());
 
   if (bIgnoreRoot &&
       (pXMLNode->GetType() != CFX_XMLNode::Type::kElement ||
diff --git a/xfa/fxfa/cxfa_ffdoc.cpp b/xfa/fxfa/cxfa_ffdoc.cpp
index 0053ba4..6bb8536 100644
--- a/xfa/fxfa/cxfa_ffdoc.cpp
+++ b/xfa/fxfa/cxfa_ffdoc.cpp
@@ -102,19 +102,12 @@
 
 bool CXFA_FFDoc::ParseDoc(const RetainPtr<IFX_SeekableStream>& stream) {
   CFX_XMLParser xml_parser(stream);
-  std::unique_ptr<CFX_XMLDocument> xml_doc = xml_parser.Parse();
-  if (!xml_doc)
+  m_pXMLDoc = xml_parser.Parse();
+  if (!m_pXMLDoc)
     return false;
 
   CXFA_DocumentBuilder builder(m_pDocument.get());
-  bool parsed = builder.BuildDocument(std::move(xml_doc), XFA_PacketType::Xdp);
-
-  // We have to set the XML document before we return so that we can clean
-  // up in the OpenDoc method. If we don't, the XMLDocument will get free'd
-  // when this method returns and UnownedPtrs get unhappy.
-  m_pXMLDoc = builder.GetXMLDoc();
-
-  if (!parsed)
+  if (!builder.BuildDocument(m_pXMLDoc.get(), XFA_PacketType::Xdp))
     return false;
 
   m_pDocument->SetRoot(builder.GetRootNode());
diff --git a/xfa/fxfa/parser/cxfa_document_builder.cpp b/xfa/fxfa/parser/cxfa_document_builder.cpp
index b3cd01f..d800bcc 100644
--- a/xfa/fxfa/parser/cxfa_document_builder.cpp
+++ b/xfa/fxfa/parser/cxfa_document_builder.cpp
@@ -229,9 +229,9 @@
 
 CXFA_DocumentBuilder::~CXFA_DocumentBuilder() = default;
 
-bool CXFA_DocumentBuilder::BuildDocument(std::unique_ptr<CFX_XMLDocument> pXML,
+bool CXFA_DocumentBuilder::BuildDocument(CFX_XMLDocument* pXML,
                                          XFA_PacketType ePacketID) {
-  CFX_XMLNode* root = Build(std::move(pXML));
+  CFX_XMLNode* root = Build(pXML);
   if (!root)
     return false;
 
@@ -239,12 +239,11 @@
   return !!m_pRootNode;
 }
 
-CFX_XMLNode* CXFA_DocumentBuilder::Build(
-    std::unique_ptr<CFX_XMLDocument> pXML) {
+CFX_XMLNode* CXFA_DocumentBuilder::Build(CFX_XMLDocument* pXML) {
   if (!pXML)
     return nullptr;
 
-  xml_doc_ = std::move(pXML);
+  xml_doc_ = pXML;
   xml_doc_->GetRoot()->InsertChildNode(
       xml_doc_->CreateNode<CFX_XMLInstruction>(L"xml"), 0);
 
diff --git a/xfa/fxfa/parser/cxfa_document_builder.h b/xfa/fxfa/parser/cxfa_document_builder.h
index 9486dac..73f4d56 100644
--- a/xfa/fxfa/parser/cxfa_document_builder.h
+++ b/xfa/fxfa/parser/cxfa_document_builder.h
@@ -25,11 +25,8 @@
   explicit CXFA_DocumentBuilder(CXFA_Document* pFactory);
   ~CXFA_DocumentBuilder();
 
-  CFX_XMLNode* Build(std::unique_ptr<CFX_XMLDocument> pXML);
-  bool BuildDocument(std::unique_ptr<CFX_XMLDocument> pXML,
-                     XFA_PacketType ePacketID);
-  std::unique_ptr<CFX_XMLDocument> GetXMLDoc() { return std::move(xml_doc_); }
-
+  CFX_XMLNode* Build(CFX_XMLDocument* pXML);
+  bool BuildDocument(CFX_XMLDocument* pXML, XFA_PacketType ePacketID);
   void ConstructXFANode(CXFA_Node* pXFANode, CFX_XMLNode* pXMLNode);
   CXFA_Node* GetRootNode() const;
 
@@ -68,9 +65,8 @@
                         XFA_PacketType ePacketID);
 
   UnownedPtr<CXFA_Document> m_pFactory;
-  std::unique_ptr<CFX_XMLDocument> xml_doc_;
-  // TODO(dsinclair): Figure out who owns this.
-  CXFA_Node* m_pRootNode = nullptr;
+  UnownedPtr<CFX_XMLDocument> xml_doc_;
+  UnownedPtr<CXFA_Node> m_pRootNode;  // All nodes owned by CXFA_NodeOwner.
   size_t m_ExecuteRecursionDepth = 0;
 };
 
diff --git a/xfa/fxfa/parser/cxfa_document_builder_unittest.cpp b/xfa/fxfa/parser/cxfa_document_builder_unittest.cpp
index 81f39ee..e105ca1 100644
--- a/xfa/fxfa/parser/cxfa_document_builder_unittest.cpp
+++ b/xfa/fxfa/parser/cxfa_document_builder_unittest.cpp
@@ -17,17 +17,19 @@
     builder_ = std::make_unique<CXFA_DocumentBuilder>(doc_.get());
   }
 
-  void TearDown() override {
-    // Hold the XML tree until we cleanup the document.
-    std::unique_ptr<CFX_XMLDocument> doc = builder_->GetXMLDoc();
-    builder_ = nullptr;
-    doc_ = nullptr;
+  CXFA_Document* GetDoc() const { return doc_.get(); }
+
+  CXFA_Node* ParseAndBuild(const RetainPtr<CFX_ReadOnlyMemoryStream>& stream) {
+    xml_ = CFX_XMLParser(stream).Parse();
+    if (!xml_)
+      return nullptr;
+    if (!builder_->BuildDocument(xml_.get(), XFA_PacketType::Config))
+      return nullptr;
+    return builder_->GetRootNode();
   }
 
-  CXFA_Document* GetDoc() const { return doc_.get(); }
-  CXFA_DocumentBuilder* GetBuilder() const { return builder_.get(); }
-
  private:
+  std::unique_ptr<CFX_XMLDocument> xml_;
   std::unique_ptr<CXFA_Document> doc_;
   std::unique_ptr<CXFA_DocumentBuilder> builder_;
 };
@@ -36,20 +38,14 @@
   static const char kInput[] = "";
   auto stream = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(
       pdfium::as_bytes(pdfium::make_span(kInput)));
-  CFX_XMLParser xml_parser(stream);
-  EXPECT_FALSE(
-      GetBuilder()->BuildDocument(xml_parser.Parse(), XFA_PacketType::Config));
-  EXPECT_FALSE(GetBuilder()->GetRootNode());
+  EXPECT_EQ(nullptr, ParseAndBuild(stream));
 }
 
 TEST_F(CXFA_DocumentBuilderTest, BadInput) {
   static const char kInput[] = "<<<>bar?>>>>>>>";
   auto stream = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(
       pdfium::as_bytes(pdfium::make_span(kInput)));
-  CFX_XMLParser xml_parser(stream);
-  EXPECT_FALSE(
-      GetBuilder()->BuildDocument(xml_parser.Parse(), XFA_PacketType::Config));
-  EXPECT_FALSE(GetBuilder()->GetRootNode());
+  EXPECT_EQ(nullptr, ParseAndBuild(stream));
 }
 
 TEST_F(CXFA_DocumentBuilderTest, XMLInstructionsScriptOff) {
@@ -62,11 +58,8 @@
 
   auto stream = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(
       pdfium::as_bytes(pdfium::make_span(kInput)));
-  CFX_XMLParser xml_parser(stream);
-  ASSERT_TRUE(
-      GetBuilder()->BuildDocument(xml_parser.Parse(), XFA_PacketType::Config));
 
-  CXFA_Node* root = GetBuilder()->GetRootNode();
+  CXFA_Node* root = ParseAndBuild(stream);
   ASSERT_TRUE(root);
   EXPECT_FALSE(GetDoc()->is_scripting());
 }
@@ -82,11 +75,8 @@
 
   auto stream = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(
       pdfium::as_bytes(pdfium::make_span(kInput)));
-  CFX_XMLParser xml_parser(stream);
-  ASSERT_TRUE(
-      GetBuilder()->BuildDocument(xml_parser.Parse(), XFA_PacketType::Config));
 
-  CXFA_Node* root = GetBuilder()->GetRootNode();
+  CXFA_Node* root = ParseAndBuild(stream);
   ASSERT_TRUE(root);
   EXPECT_TRUE(GetDoc()->is_scripting());
 }
@@ -101,11 +91,8 @@
 
   auto stream = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(
       pdfium::as_bytes(pdfium::make_span(kInput)));
-  CFX_XMLParser xml_parser(stream);
-  ASSERT_TRUE(
-      GetBuilder()->BuildDocument(xml_parser.Parse(), XFA_PacketType::Config));
 
-  CXFA_Node* root = GetBuilder()->GetRootNode();
+  CXFA_Node* root = ParseAndBuild(stream);
   ASSERT_TRUE(root);
   EXPECT_TRUE(GetDoc()->is_strict_scoping());
 }
@@ -120,11 +107,8 @@
 
   auto stream = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(
       pdfium::as_bytes(pdfium::make_span(kInput)));
-  CFX_XMLParser xml_parser(stream);
-  ASSERT_TRUE(
-      GetBuilder()->BuildDocument(xml_parser.Parse(), XFA_PacketType::Config));
 
-  CXFA_Node* root = GetBuilder()->GetRootNode();
+  CXFA_Node* root = ParseAndBuild(stream);
   ASSERT_TRUE(root);
   EXPECT_FALSE(GetDoc()->is_strict_scoping());
 }
@@ -142,13 +126,9 @@
 
   auto stream = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(
       pdfium::as_bytes(pdfium::make_span(kInput)));
-  CFX_XMLParser xml_parser(stream);
-  ASSERT_TRUE(
-      GetBuilder()->BuildDocument(xml_parser.Parse(), XFA_PacketType::Config));
 
-  CXFA_Node* root = GetBuilder()->GetRootNode();
+  CXFA_Node* root = ParseAndBuild(stream);
   ASSERT_TRUE(root);
-
   EXPECT_TRUE(GetDoc()->is_scripting());
   EXPECT_TRUE(GetDoc()->is_strict_scoping());
 }