commit 9cc1c1f03a0291bf1861a54d12e0519ac4c2fbd2
author Tom Sepez <tsepez@chromium.org> Wed Mar 03 23:09:41 2021 +0000
committer Chromium commit bot <commit-bot@chromium.org> Wed Mar 03 23:09:41 2021 +0000
tree 3605a36e62420a78ddecb9a388a970dc943721c6
parent 29d377b3b951eccc01a70e0b458ae9b01c230eeb

Fix endless revalidation in when validation makes identical changes.

As seen in popular NFFBAR.pdf form.

-- Short-circuit update where possible.
-- Remove bNotify parameter from notifiers and instead do not
   call them if notification is not intended.

Change-Id: Ic3e41a7f4f49e0dff94b81e5a475132e985f2eb6
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/78330
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>

fxjs/xfa/cjx_object.cpp

diff --git a/fxjs/xfa/cjx_object.cpp b/fxjs/xfa/cjx_object.cpp
index dec8d87..7e7dfd8 100644
--- a/fxjs/xfa/cjx_object.cpp
+++ b/fxjs/xfa/cjx_object.cpp
@@ -371,10 +371,14 @@
 void CJX_Object::SetMeasure(XFA_Attribute eAttr,
                             const CXFA_Measurement& mValue,
                             bool bNotify) {
+  // Can't short-circuit update here when the value is the same since it
+  // might have come from further up the chain from where we are setting it.
   uint32_t key = GetMapKey_Element(GetXFAObject()->GetElementType(), eAttr);
-  OnChanging(eAttr, bNotify);
+  if (bNotify)
+    OnChanging(eAttr);
   SetMapModuleMeasurement(key, mValue);
-  OnChanged(eAttr, bNotify, false);
+  if (bNotify)
+    OnChanged(eAttr, false);
 }
 
 Optional<CXFA_Measurement> CJX_Object::TryMeasure(XFA_Attribute eAttr,
@@ -418,11 +422,16 @@
                               bool bScriptModify) {
   CXFA_Node* xfaObj = GetXFANode();
   uint32_t key = GetMapKey_Element(xfaObj->GetElementType(), eAttr);
-  OnChanging(eAttr, bNotify);
-  SetMapModuleString(key, wsValue);
-  if (eAttr == XFA_Attribute::Name)
-    xfaObj->UpdateNameHash();
-  OnChanged(eAttr, bNotify, bScriptModify);
+  Optional<WideString> old_value = GetMapModuleString(key);
+  if (!old_value.has_value() || old_value.value() != wsValue) {
+    if (bNotify)
+      OnChanging(eAttr);
+    SetMapModuleString(key, wsValue);
+    if (eAttr == XFA_Attribute::Name)
+      xfaObj->UpdateNameHash();
+    if (bNotify)
+      OnChanged(eAttr, bScriptModify);
+  }
 
   if (!xfaObj->IsNeedSavingXMLNode() || eAttr == XFA_Attribute::QualifiedName ||
       eAttr == XFA_Attribute::BindingNode) {
@@ -461,12 +470,16 @@
   auto* xfaObj = GetXFANode();
   uint32_t key =
       GetMapKey_Element(xfaObj->GetElementType(), XFA_Attribute::Value);
-
-  OnChanging(XFA_Attribute::Value, bNotify);
-  SetMapModuleString(key, wsValue);
-  OnChanged(XFA_Attribute::Value, bNotify, bScriptModify);
-  if (xfaObj->IsNeedSavingXMLNode())
-    xfaObj->SetToXML(wsXMLValue);
+  Optional<WideString> old_value = GetMapModuleString(key);
+  if (!old_value.has_value() || old_value.value() != wsValue) {
+    if (bNotify)
+      OnChanging(XFA_Attribute::Value);
+    SetMapModuleString(key, wsValue);
+    if (bNotify)
+      OnChanged(XFA_Attribute::Value, bScriptModify);
+    if (xfaObj->IsNeedSavingXMLNode())
+      xfaObj->SetToXML(wsXMLValue);
+  }
 }
 
 Optional<WideString> CJX_Object::TryCData(XFA_Attribute eAttr,
@@ -478,6 +491,7 @@
 
   if (!bUseDefault)
     return pdfium::nullopt;
+
   return GetXFANode()->GetDefaultCData(eAttr);
 }
 
@@ -485,10 +499,14 @@
                                      int32_t value,
                                      bool bNotify) {
   uint32_t key = GetMapKey_Element(GetXFAObject()->GetElementType(), eAttr);
-  OnChanging(eAttr, bNotify);
-  SetMapModuleValue(key, value);
-  OnChanged(eAttr, bNotify, false);
-
+  Optional<int32_t> old_value = GetMapModuleValue(key);
+  if (!old_value.has_value() || old_value.value() != value) {
+    if (bNotify)
+      OnChanging(eAttr);
+    SetMapModuleValue(key, value);
+    if (bNotify)
+      OnChanged(eAttr, false);
+  }
   CXFA_Node* pNode = GetXFANode();
   return pNode->IsNeedSavingXMLNode() ? ToXMLElement(pNode->GetXMLMappingNode())
                                       : nullptr;
@@ -915,20 +933,23 @@
   ToNode(pSrcObj)->JSObject()->MoveBufferMapData(pDstObj);
 }
 
-void CJX_Object::OnChanging(XFA_Attribute eAttr, bool bNotify) {
-  if (!bNotify || !GetXFANode()->IsInitialized())
+void CJX_Object::OnChanging(XFA_Attribute eAttr) {
+  if (!GetXFANode()->IsInitialized())
     return;
 
   CXFA_FFNotify* pNotify = GetDocument()->GetNotify();
-  if (pNotify)
-    pNotify->OnValueChanging(GetXFANode(), eAttr);
+  if (!pNotify)
+    return;
+
+  pNotify->OnValueChanging(GetXFANode(), eAttr);
 }
 
 void CJX_Object::OnChanged(XFA_Attribute eAttr,
-                           bool bNotify,
                            bool bScriptModify) {
-  if (bNotify && GetXFANode()->IsInitialized())
-    GetXFANode()->SendAttributeChangeMessage(eAttr, bScriptModify);
+  if (!GetXFANode()->IsInitialized())
+    return;
+
+  GetXFANode()->SendAttributeChangeMessage(eAttr, bScriptModify);
 }
 
 CJX_Object::CalcData* CJX_Object::GetOrCreateCalcData(cppgc::Heap* heap) {

fxjs/xfa/cjx_object.h

diff --git a/fxjs/xfa/cjx_object.h b/fxjs/xfa/cjx_object.h
index d6e1872..b2a7ca9 100644
--- a/fxjs/xfa/cjx_object.h
+++ b/fxjs/xfa/cjx_object.h
@@ -248,8 +248,8 @@
                                                      XFA_Element eType) const;
   CXFA_Node* GetOrCreatePropertyInternal(int32_t index, XFA_Element eType);
 
-  void OnChanged(XFA_Attribute eAttr, bool bNotify, bool bScriptModify);
-  void OnChanging(XFA_Attribute eAttr, bool bNotify);
+  void OnChanging(XFA_Attribute eAttr);
+  void OnChanged(XFA_Attribute eAttr, bool bScriptModify);
 
   // Returns a pointer to the XML node that needs to be updated with the new
   // attribute value. |nullptr| if no update is needed.