Cleanup CXFA_Node ownership
Currently a CXFA_Node can be owned by the CXFA_Document root pointer,
the CXFA_Document Purge list or as a child of another CXFA_Node. This
makes it hard to know who currently owns what.
This CL moves all ownership of nodes into a CXFA_NodeOwner class which
CXFA_Document subclasses. The node owner always owns all nodes and is
responsible for cleaning them up upon destruction. The destruction order
is not guarenteed to be in tree order as nodes can be inserted and moved
around the tree.
Change-Id: I6b202b8e844999ba835093dcdd1a808938b6d9a8
Reviewed-on: https://pdfium-review.googlesource.com/26434
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>
diff --git a/BUILD.gn b/BUILD.gn
index 6681ae8..bdadebd 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -2521,6 +2521,8 @@
"xfa/fxfa/parser/cxfa_nodeiteratortemplate.h",
"xfa/fxfa/parser/cxfa_nodelocale.cpp",
"xfa/fxfa/parser/cxfa_nodelocale.h",
+ "xfa/fxfa/parser/cxfa_nodeowner.cpp",
+ "xfa/fxfa/parser/cxfa_nodeowner.h",
"xfa/fxfa/parser/cxfa_numberofcopies.cpp",
"xfa/fxfa/parser/cxfa_numberofcopies.h",
"xfa/fxfa/parser/cxfa_numberpattern.cpp",
diff --git a/fxjs/xfa/cjx_object.cpp b/fxjs/xfa/cjx_object.cpp
index d663fcc..50643ff 100644
--- a/fxjs/xfa/cjx_object.cpp
+++ b/fxjs/xfa/cjx_object.cpp
@@ -1261,7 +1261,7 @@
pHeadChild = pSibling;
}
- std::unique_ptr<CXFA_Node> pProtoForm(pProtoNode->CloneTemplateToForm(true));
+ CXFA_Node* pProtoForm = pProtoNode->CloneTemplateToForm(true);
pHeadChild = pProtoForm->GetFirstChild();
while (pHeadChild) {
CXFA_Node* pSibling = pHeadChild->GetNextSibling();
@@ -1269,8 +1269,8 @@
ToNode(GetXFAObject())->InsertChild(pHeadChild, nullptr);
pHeadChild = pSibling;
}
-
- GetDocument()->RemovePurgeNode(pProtoForm.get());
+ GetDocument()->FreeOwnedNode(pProtoForm);
+ pProtoForm = nullptr;
}
void CJX_Object::Script_Attribute_BOOL(CFXJSE_Value* pValue,
diff --git a/xfa/fxfa/parser/cxfa_document.cpp b/xfa/fxfa/parser/cxfa_document.cpp
index 434d6cb..a21d3f9 100644
--- a/xfa/fxfa/parser/cxfa_document.cpp
+++ b/xfa/fxfa/parser/cxfa_document.cpp
@@ -6,6 +6,8 @@
#include "xfa/fxfa/parser/cxfa_document.h"
+#include <set>
+
#include "core/fxcrt/fx_extension.h"
#include "fxjs/cfxjse_engine.h"
#include "xfa/fxfa/cxfa_ffnotify.h"
@@ -87,7 +89,8 @@
} // namespace
CXFA_Document::CXFA_Document(CXFA_DocumentParser* pParser)
- : m_pParser(pParser),
+ : CXFA_NodeOwner(),
+ m_pParser(pParser),
m_pRootNode(nullptr),
m_eCurVersionMode(XFA_VERSION_DEFAULT),
m_dwDocFlags(0) {
@@ -98,12 +101,6 @@
// Remove all the bindings before freeing the node as the ownership is wonky.
if (m_pRootNode)
m_pRootNode->ReleaseBindingNodes();
-
- delete m_pRootNode;
-
- for (CXFA_Node* pNode : m_PurgeNodes)
- delete pNode;
- m_PurgeNodes.clear();
}
CXFA_LayoutProcessor* CXFA_Document::GetLayoutProcessor() {
@@ -128,14 +125,6 @@
m_pScriptSignature.reset();
}
-void CXFA_Document::SetRoot(CXFA_Node* pNewRoot) {
- if (m_pRootNode)
- AddPurgeNode(m_pRootNode);
-
- m_pRootNode = pNewRoot;
- RemovePurgeNode(pNewRoot);
-}
-
CFX_XMLDoc* CXFA_Document::GetXMLDoc() const {
return m_pParser->GetXMLDoc();
}
@@ -221,22 +210,7 @@
XFA_Element eElement) {
if (eElement == XFA_Element::Unknown)
return nullptr;
-
- std::unique_ptr<CXFA_Node> pNode = CXFA_Node::Create(this, eElement, packet);
- if (!pNode)
- return nullptr;
-
- // TODO(dsinclair): AddPrugeNode should take ownership of the pointer.
- AddPurgeNode(pNode.get());
- return pNode.release();
-}
-
-void CXFA_Document::AddPurgeNode(CXFA_Node* pNode) {
- m_PurgeNodes.insert(pNode);
-}
-
-bool CXFA_Document::RemovePurgeNode(CXFA_Node* pNode) {
- return !!m_PurgeNodes.erase(pNode);
+ return AddOwnedNode(CXFA_Node::Create(this, eElement, packet));
}
void CXFA_Document::SetFlag(uint32_t dwFlag, bool bOn) {
diff --git a/xfa/fxfa/parser/cxfa_document.h b/xfa/fxfa/parser/cxfa_document.h
index 09c2601..4d1c417 100644
--- a/xfa/fxfa/parser/cxfa_document.h
+++ b/xfa/fxfa/parser/cxfa_document.h
@@ -9,11 +9,11 @@
#include <map>
#include <memory>
-#include <set>
#include <vector>
#include "xfa/fxfa/fxfa.h"
#include "xfa/fxfa/parser/cxfa_localemgr.h"
+#include "xfa/fxfa/parser/cxfa_nodeowner.h"
enum XFA_VERSION {
XFA_VERSION_UNKNOWN = 0,
@@ -57,10 +57,10 @@
class CXFA_Node;
class CXFA_Object;
-class CXFA_Document {
+class CXFA_Document : public CXFA_NodeOwner {
public:
explicit CXFA_Document(CXFA_DocumentParser* pParser);
- virtual ~CXFA_Document();
+ ~CXFA_Document() override;
virtual CXFA_FFNotify* GetNotify() const;
@@ -77,10 +77,7 @@
CXFA_LayoutProcessor* GetDocLayout();
CFXJSE_Engine* GetScriptContext();
- void SetRoot(CXFA_Node* pNewRoot);
-
- void AddPurgeNode(CXFA_Node* pNode);
- bool RemovePurgeNode(CXFA_Node* pNode);
+ void SetRoot(CXFA_Node* pNewRoot) { m_pRootNode = pNewRoot; }
bool HasFlag(uint32_t dwFlag) { return (m_dwDocFlags & dwFlag) == dwFlag; }
void SetFlag(uint32_t dwFlag, bool bOn);
@@ -119,7 +116,6 @@
std::unique_ptr<CScript_LogPseudoModel> m_pScriptLog;
std::unique_ptr<CScript_LayoutPseudoModel> m_pScriptLayout;
std::unique_ptr<CScript_SignaturePseudoModel> m_pScriptSignature;
- std::set<CXFA_Node*> m_PurgeNodes;
XFA_VERSION m_eCurVersionMode;
uint32_t m_dwDocFlags;
};
diff --git a/xfa/fxfa/parser/cxfa_node.cpp b/xfa/fxfa/parser/cxfa_node.cpp
index 83da3e7..9159f02 100644
--- a/xfa/fxfa/parser/cxfa_node.cpp
+++ b/xfa/fxfa/parser/cxfa_node.cpp
@@ -537,16 +537,7 @@
pdfium::MakeUnique<CJX_Node>(this)) {}
CXFA_Node::~CXFA_Node() {
- ASSERT(!parent_);
-
- CXFA_Node* pNode = first_child_;
- while (pNode) {
- CXFA_Node* pNext = pNode->next_sibling_;
- pNode->parent_ = nullptr;
- delete pNode;
- pNode = pNext;
- }
- if (m_pXMLNode && IsOwnXMLNode())
+ if (m_pXMLNode && IsOwnedXMLNode())
delete m_pXMLNode;
}
@@ -1119,10 +1110,6 @@
pNode->parent_ = this;
pNode->ClearFlag(XFA_NodeFlag_HasRemovedChildren);
- bool ret = m_pDocument->RemovePurgeNode(pNode);
- ASSERT(ret);
- (void)ret; // Avoid unused variable warning.
-
if (!first_child_) {
ASSERT(!last_child_);
@@ -1221,8 +1208,6 @@
OnRemoved(bNotify);
- m_pDocument->AddPurgeNode(pNode);
-
if (!IsNeedSavingXMLNode() || !pNode->m_pXMLNode)
return;
diff --git a/xfa/fxfa/parser/cxfa_node.h b/xfa/fxfa/parser/cxfa_node.h
index e800c3c..f5eb2be 100644
--- a/xfa/fxfa/parser/cxfa_node.h
+++ b/xfa/fxfa/parser/cxfa_node.h
@@ -469,7 +469,7 @@
void SetImageEdit(const WideString& wsContentType,
const WideString& wsHref,
const WideString& wsData);
- bool IsOwnXMLNode() const { return HasFlag(XFA_NodeFlag_OwnXMLNode); }
+ bool IsOwnedXMLNode() const { return HasFlag(XFA_NodeFlag_OwnXMLNode); }
CXFA_Node* GetBindingNode() const {
if (binding_nodes_.empty())
return nullptr;
diff --git a/xfa/fxfa/parser/cxfa_nodeowner.cpp b/xfa/fxfa/parser/cxfa_nodeowner.cpp
new file mode 100644
index 0000000..ae6f589
--- /dev/null
+++ b/xfa/fxfa/parser/cxfa_nodeowner.cpp
@@ -0,0 +1,35 @@
+// 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.
+
+// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
+
+#include "xfa/fxfa/parser/cxfa_nodeowner.h"
+
+#include <utility>
+
+#include "third_party/base/stl_util.h"
+#include "xfa/fxfa/parser/cxfa_node.h"
+
+CXFA_NodeOwner::CXFA_NodeOwner() = default;
+
+CXFA_NodeOwner::~CXFA_NodeOwner() = default;
+
+CXFA_Node* CXFA_NodeOwner::AddOwnedNode(std::unique_ptr<CXFA_Node> node) {
+ if (!node)
+ return nullptr;
+
+ CXFA_Node* ret = node.get();
+ nodes_.insert(std::move(node));
+ return ret;
+}
+
+void CXFA_NodeOwner::FreeOwnedNode(CXFA_Node* node) {
+ if (!node)
+ return;
+
+ pdfium::FakeUniquePtr<CXFA_Node> search(node);
+ auto it = nodes_.find(search);
+ assert(it != nodes_.end());
+ nodes_.erase(it);
+}
diff --git a/xfa/fxfa/parser/cxfa_nodeowner.h b/xfa/fxfa/parser/cxfa_nodeowner.h
new file mode 100644
index 0000000..2aaabe4
--- /dev/null
+++ b/xfa/fxfa/parser/cxfa_nodeowner.h
@@ -0,0 +1,28 @@
+// 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.
+
+// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
+
+#ifndef XFA_FXFA_PARSER_CXFA_NODEOWNER_H_
+#define XFA_FXFA_PARSER_CXFA_NODEOWNER_H_
+
+#include <memory>
+#include <set>
+
+class CXFA_Node;
+
+class CXFA_NodeOwner {
+ public:
+ virtual ~CXFA_NodeOwner();
+
+ CXFA_Node* AddOwnedNode(std::unique_ptr<CXFA_Node> node);
+ void FreeOwnedNode(CXFA_Node* node);
+
+ protected:
+ CXFA_NodeOwner();
+
+ std::set<std::unique_ptr<CXFA_Node>> nodes_;
+};
+
+#endif // XFA_FXFA_PARSER_CXFA_NODEOWNER_H_
