CJX_Object::SetContent() should only update value nodes.
This method operates in two passes. In the first part, a set
of value nodes children is collected, but in the second pass,
all children are traversed regardless of whether they are
value nodes. There's no reason to believe these are the same
when the node has non-value node children.
- Fix possibly overly-strong assert in CXFA_Node. In turn, this
causes one test to leave a node unmodified rather than CHECK().
Bug: chromium:985781
Change-Id: Idd8ae5d8fb5f07ae01487db589036bdab3a99ada
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/57932
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 dd30480..29d6d17 100644
--- a/fxjs/xfa/cjx_object.cpp
+++ b/fxjs/xfa/cjx_object.cpp
@@ -550,15 +550,14 @@
break;
CXFA_Node* pChildValue = pValue->GetFirstChild();
- ASSERT(pChildValue);
pChildValue->JSObject()->SetCData(XFA_Attribute::ContentType,
L"text/xml", false, false);
pChildValue->JSObject()->SetContent(wsContent, wsContent, bNotify,
bScriptModify, false);
+
CXFA_Node* pBind = ToNode(GetXFAObject())->GetBindData();
if (bSyncData && pBind) {
std::vector<WideString> wsSaveTextArray;
- size_t iSize = 0;
if (!wsContent.IsEmpty()) {
size_t iStart = 0;
size_t iLength = wsContent.GetLength();
@@ -577,41 +576,39 @@
wsContent.Mid(iStart, iLength - iStart));
}
}
- iSize = wsSaveTextArray.size();
}
- if (iSize == 0) {
- while (CXFA_Node* pChildNode = pBind->GetFirstChild()) {
- pBind->RemoveChildAndNotify(pChildNode, true);
- }
- } else {
- std::vector<CXFA_Node*> valueNodes =
- pBind->GetNodeListForType(XFA_Element::DataValue);
- size_t iDatas = valueNodes.size();
- if (iDatas < iSize) {
- size_t iAddNodes = iSize - iDatas;
- CXFA_Node* pValueNodes = nullptr;
+ std::vector<CXFA_Node*> valueNodes =
+ pBind->GetNodeListForType(XFA_Element::DataValue);
+
+ // Adusting node count might have side effects, do not trust that
+ // we'll ever actually get there.
+ size_t tries = 0;
+ while (valueNodes.size() != wsSaveTextArray.size()) {
+ if (++tries > 4)
+ return;
+ if (valueNodes.size() < wsSaveTextArray.size()) {
+ size_t iAddNodes = wsSaveTextArray.size() - valueNodes.size();
while (iAddNodes-- > 0) {
- pValueNodes =
+ CXFA_Node* pValueNodes =
pBind->CreateSamePacketNode(XFA_Element::DataValue);
pValueNodes->JSObject()->SetCData(XFA_Attribute::Name, L"value",
false, false);
pValueNodes->CreateXMLMappingNode();
pBind->InsertChildAndNotify(pValueNodes, nullptr);
}
- pValueNodes = nullptr;
- } else if (iDatas > iSize) {
- size_t iDelNodes = iDatas - iSize;
- while (iDelNodes-- > 0) {
- pBind->RemoveChildAndNotify(pBind->GetFirstChild(), true);
- }
+ } else {
+ size_t iDelNodes = valueNodes.size() - wsSaveTextArray.size();
+ for (size_t i = 0; i < iDelNodes; ++i)
+ pBind->RemoveChildAndNotify(valueNodes[i], true);
}
- int32_t i = 0;
- for (CXFA_Node* pValueNode = pBind->GetFirstChild(); pValueNode;
- pValueNode = pValueNode->GetNextSibling()) {
- pValueNode->JSObject()->SetAttributeValue(
- wsSaveTextArray[i], wsSaveTextArray[i], false, false);
- i++;
- }
+ valueNodes = pBind->GetNodeListForType(XFA_Element::DataValue);
+ }
+ ASSERT(valueNodes.size() == wsSaveTextArray.size());
+ size_t i = 0;
+ for (CXFA_Node* pValueNode : valueNodes) {
+ pValueNode->JSObject()->SetAttributeValue(
+ wsSaveTextArray[i], wsSaveTextArray[i], false, false);
+ i++;
}
for (auto* pArrayNode : *(pBind->GetBindItems())) {
if (pArrayNode != ToNode(GetXFAObject())) {
diff --git a/testing/resources/javascript/xfa_specific/bug_985781.in b/testing/resources/javascript/xfa_specific/bug_985781.in
new file mode 100644
index 0000000..1390d35
--- /dev/null
+++ b/testing/resources/javascript/xfa_specific/bug_985781.in
@@ -0,0 +1,72 @@
+{{header}}
+{{include ../../xfa_catalog_1_0.fragment}}
+{{include ../../xfa_object_2_0.fragment}}
+{{include ../../xfa_preamble_3_0.fragment}}
+{{include ../../xfa_config_4_0.fragment}}
+{{object 5 0}} <<
+>>
+stream
+<template>
+<subform allowMacro="1" layout="lr" mergeMode="consumeData" name="subform_0">
+ <pageSet name="page_0">
+ <pageArea name="pagearea_0">
+ <contentArea h="0.5in" w="0.25in" x="1.0in" y="10.0in"></contentArea>
+ <subform allowMacro="1" layout="table" mergeMode="consumeData" name="subform_1">
+ <validate>
+ <script contentType="application/x-javascript">
+ xfa.resolveNode("xfa.form.subform_0.subform_3.field_5").execCalculate();
+ </script>
+ </validate>
+ </subform>
+ </pageArea>
+ </pageSet>
+ <subform allowMacro="0" layout="position" mergeMode="matchTemplate" name="subform_2">
+ <field name="field_3">
+ <items presence="visible" save="0">
+ <text>a</text>
+ <text>b</text>
+ <text>c</text>
+ <text>d</text>
+ </items>
+ <ui>
+ <numericEdit hScrollPolicy="on"></numericEdit>
+ </ui>
+ <validate>
+ <script contentType="application/x-javascript">
+ xfa.template.remerge();
+ </script>
+ </validate>
+ <value override="0">
+ <text>37</text>
+ </value>
+ </field>
+ </subform>
+ <subform allowMacro="0" layout="tb" mergeMode="matchTemplate" name="subform_3">
+ <bind match="dataRef" ref="a"></bind>
+ <field name="field_5">
+ <bind match="dataRef" ref="a"></bind>
+ <calculate>
+ <script contentType="application/x-javascript">
+ xfa.resolveNode("xfa.form.subform_0.page_0.pagearea_0.subform_1").leader = "test";
+ app.alert("Finished !!!");
+ </script>
+ </calculate>
+ <items presence="invisible" save="0">
+ <text>254</text>
+ </items>
+ <ui>
+ <choiceList commitOn="select" open="multiSelect" textEntry="0"></choiceList>
+ </ui>
+ </field>
+ </subform>
+</subform>
+</template>
+endstream
+endobj
+{{include ../../xfa_locale_6_0.fragment}}
+{{include ../../xfa_postamble_7_0.fragment}}
+{{include ../../xfa_pages_8_0.fragment}}
+{{xref}}
+{{trailer}}
+{{startxref}}
+%%EOF
diff --git a/testing/resources/javascript/xfa_specific/bug_985781_expected.txt b/testing/resources/javascript/xfa_specific/bug_985781_expected.txt
new file mode 100644
index 0000000..cc87d2b
--- /dev/null
+++ b/testing/resources/javascript/xfa_specific/bug_985781_expected.txt
@@ -0,0 +1 @@
+Alert: Finished !!!
diff --git a/xfa/fxfa/parser/cxfa_node.cpp b/xfa/fxfa/parser/cxfa_node.cpp
index 26a201a..1de4a9e 100644
--- a/xfa/fxfa/parser/cxfa_node.cpp
+++ b/xfa/fxfa/parser/cxfa_node.cpp
@@ -1520,7 +1520,8 @@
void CXFA_Node::RemoveChildAndNotify(CXFA_Node* pNode, bool bNotify) {
CHECK(pNode);
- CHECK_EQ(pNode->GetParent(), this);
+ if (pNode->GetParent() != this)
+ return;
pNode->SetFlag(XFA_NodeFlag_HasRemovedChildren);
TreeNode<CXFA_Node>::RemoveChild(pNode);
diff --git a/xfa/fxfa/parser/cxfa_node_unittest.cpp b/xfa/fxfa/parser/cxfa_node_unittest.cpp
index b8ab68b..620704f 100644
--- a/xfa/fxfa/parser/cxfa_node_unittest.cpp
+++ b/xfa/fxfa/parser/cxfa_node_unittest.cpp
@@ -376,5 +376,6 @@
GetDoc()->CreateNode(XFA_PacketType::Form, XFA_Element::Ui);
child0->InsertChildAndNotify(-1, child1);
- EXPECT_DEATH_IF_SUPPORTED(GetNode()->RemoveChildAndNotify(child1, false), "");
+ GetNode()->RemoveChildAndNotify(child1, false);
+ EXPECT_EQ(child0, child1->GetParent());
}
