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