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;