XFA: Do not use UnownedPtr between CXFA_Nodes.
They all have document lifetime and may be removed in any order.
Bug: chromium:907427
Change-Id: Ie37addd435e2607fe083ceb76039bced25b764aa
Reviewed-on: https://pdfium-review.googlesource.com/c/45830
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/fxjs/xfa/cjx_object.cpp b/fxjs/xfa/cjx_object.cpp
index c1c7cbc..44ebcc9 100644
--- a/fxjs/xfa/cjx_object.cpp
+++ b/fxjs/xfa/cjx_object.cpp
@@ -623,8 +623,8 @@
i++;
}
}
- for (const auto& pArrayNode : *(pBind->GetBindItems())) {
- if (pArrayNode.Get() != ToNode(GetXFAObject())) {
+ for (auto* pArrayNode : *(pBind->GetBindItems())) {
+ if (pArrayNode != ToNode(GetXFAObject())) {
pArrayNode->JSObject()->SetContent(wsContent, wsContent, bNotify,
bScriptModify, false);
}
@@ -649,8 +649,8 @@
if (pBindNode && bSyncData) {
pBindNode->JSObject()->SetContent(wsContent, wsXMLValue, bNotify,
bScriptModify, false);
- for (const auto& pArrayNode : *(pBindNode->GetBindItems())) {
- if (pArrayNode.Get() != ToNode(GetXFAObject())) {
+ for (auto* pArrayNode : *(pBindNode->GetBindItems())) {
+ if (pArrayNode != ToNode(GetXFAObject())) {
pArrayNode->JSObject()->SetContent(wsContent, wsContent, bNotify,
true, false);
}
@@ -722,7 +722,7 @@
SetAttributeValue(wsContent, wsXMLValue, bNotify, bScriptModify);
if (pBindNode && bSyncData) {
- for (const auto& pArrayNode : *(pBindNode->GetBindItems())) {
+ for (auto* pArrayNode : *(pBindNode->GetBindItems())) {
pArrayNode->JSObject()->SetContent(wsContent, wsContent, bNotify,
bScriptModify, false);
}
@@ -1453,7 +1453,7 @@
CXFA_Node* pContainerNode = nullptr;
if (ToNode(GetXFAObject())->GetPacketType() == XFA_PacketType::Datasets) {
WideString wsPicture;
- for (const auto& pFormNode : *(ToNode(GetXFAObject())->GetBindItems())) {
+ for (auto* pFormNode : *(ToNode(GetXFAObject())->GetBindItems())) {
if (!pFormNode || pFormNode->HasRemovedChildren())
continue;
diff --git a/xfa/fxfa/parser/cxfa_document.cpp b/xfa/fxfa/parser/cxfa_document.cpp
index f870497..24a03ed 100644
--- a/xfa/fxfa/parser/cxfa_document.cpp
+++ b/xfa/fxfa/parser/cxfa_document.cpp
@@ -1280,13 +1280,7 @@
m_eCurVersionMode(XFA_VERSION_DEFAULT),
m_dwDocFlags(0) {}
-CXFA_Document::~CXFA_Document() {
- // The destruction order of the nodes is not known because they're stored in a
- // list in the document. Therefore. the binding nodes must be released before
- // freeing the nodes to avoid dangling UnownedPtrs.
- if (m_pRootNode)
- m_pRootNode->ReleaseBindingNodes();
-}
+CXFA_Document::~CXFA_Document() = default;
CXFA_LayoutProcessor* CXFA_Document::GetLayoutProcessor() {
if (!m_pLayoutProcessor)
diff --git a/xfa/fxfa/parser/cxfa_node.cpp b/xfa/fxfa/parser/cxfa_node.cpp
index e4899bd..0f2ee99 100644
--- a/xfa/fxfa/parser/cxfa_node.cpp
+++ b/xfa/fxfa/parser/cxfa_node.cpp
@@ -865,17 +865,13 @@
return GetBindingNode();
}
-std::vector<UnownedPtr<CXFA_Node>>* CXFA_Node::GetBindItems() {
- return &binding_nodes_;
-}
-
int32_t CXFA_Node::AddBindItem(CXFA_Node* pFormNode) {
ASSERT(pFormNode);
if (BindsFormItems()) {
bool found = false;
- for (auto& v : binding_nodes_) {
- if (v.Get() == pFormNode) {
+ for (auto* v : binding_nodes_) {
+ if (v == pFormNode) {
found = true;
break;
}
@@ -893,9 +889,9 @@
if (pOldFormItem == pFormNode)
return 1;
- std::vector<UnownedPtr<CXFA_Node>> items;
- items.emplace_back(pOldFormItem);
- items.emplace_back(pFormNode);
+ std::vector<CXFA_Node*> items;
+ items.push_back(pOldFormItem);
+ items.push_back(pFormNode);
binding_nodes_ = std::move(items);
m_uNodeFlags |= XFA_NodeFlag_BindFormItems;
@@ -904,10 +900,8 @@
int32_t CXFA_Node::RemoveBindItem(CXFA_Node* pFormNode) {
if (BindsFormItems()) {
- auto it = std::find_if(binding_nodes_.begin(), binding_nodes_.end(),
- [&pFormNode](const UnownedPtr<CXFA_Node>& node) {
- return node.Get() == pFormNode;
- });
+ auto it =
+ std::find(binding_nodes_.begin(), binding_nodes_.end(), pFormNode);
if (it != binding_nodes_.end())
binding_nodes_.erase(it);
@@ -953,10 +947,10 @@
return nullptr;
CXFA_Node* pFieldNode = nullptr;
- for (const auto& pFormNode : *(pDataNode->GetBindItems())) {
+ for (auto* pFormNode : *(pDataNode->GetBindItems())) {
if (!pFormNode || pFormNode->HasRemovedChildren())
continue;
- pFieldNode = pFormNode->IsWidgetReady() ? pFormNode.Get() : nullptr;
+ pFieldNode = pFormNode->IsWidgetReady() ? pFormNode : nullptr;
if (pFieldNode)
wsPicture = pFieldNode->GetPictureContent(XFA_VALUEPICTURE_DataBind);
if (!wsPicture.IsEmpty())
@@ -1423,16 +1417,6 @@
m_uNodeFlags &= ~dwFlag;
}
-void CXFA_Node::ReleaseBindingNodes() {
- // Clear any binding nodes as we don't necessarily destruct in an order that
- // makes sense.
- for (auto& node : binding_nodes_)
- node.Release();
-
- for (CXFA_Node* pNode = first_child_; pNode; pNode = pNode->next_sibling_)
- pNode->ReleaseBindingNodes();
-}
-
bool CXFA_Node::IsAttributeInXML() {
return JSObject()->GetEnum(XFA_Attribute::Contains) ==
XFA_AttributeEnum::MetaData;
diff --git a/xfa/fxfa/parser/cxfa_node.h b/xfa/fxfa/parser/cxfa_node.h
index e8311f4..6731a3b 100644
--- a/xfa/fxfa/parser/cxfa_node.h
+++ b/xfa/fxfa/parser/cxfa_node.h
@@ -141,10 +141,6 @@
binding_nodes_.emplace_back(node);
}
- // TODO(dsinclair): This should not be needed. Nodes should get un-bound when
- // they're deleted instead of us pointing to bad objects.
- void ReleaseBindingNodes();
-
bool HasRemovedChildren() const {
return HasFlag(XFA_NodeFlag_HasRemovedChildren);
}
@@ -199,7 +195,7 @@
CXFA_Node* GetDataDescriptionNode();
void SetDataDescriptionNode(CXFA_Node* pDataDescriptionNode);
CXFA_Node* GetBindData();
- std::vector<UnownedPtr<CXFA_Node>>* GetBindItems();
+ std::vector<CXFA_Node*>* GetBindItems() { return &binding_nodes_; }
int32_t AddBindItem(CXFA_Node* pFormNode);
int32_t RemoveBindItem(CXFA_Node* pFormNode);
bool HasBindItem();
@@ -469,7 +465,7 @@
CXFA_Node* GetBindingNode() const {
if (binding_nodes_.empty())
return nullptr;
- return binding_nodes_[0].Get();
+ return binding_nodes_[0];
}
bool BindsFormItems() const { return HasFlag(XFA_NodeFlag_BindFormItems); }
bool NeedsInitApp() const { return HasFlag(XFA_NodeFlag_NeedsInitApp); }
@@ -505,8 +501,8 @@
uint8_t m_ExecuteRecursionDepth = 0;
uint16_t m_uNodeFlags = XFA_NodeFlag_None;
uint32_t m_dwNameHash = 0;
- CXFA_Node* m_pAuxNode = nullptr; // Raw, node tree cleanup order.
- std::vector<UnownedPtr<CXFA_Node>> binding_nodes_;
+ CXFA_Node* m_pAuxNode = nullptr; // Raw, node tree cleanup order.
+ std::vector<CXFA_Node*> binding_nodes_; // Raw, node tree cleanup order.
bool m_bIsNull = true;
bool m_bPreNull = true;
bool is_widget_ready_ = false;
