From f67d206be236d017e161bd41aa104347a288bc0b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Tue, 4 Aug 2020 11:22:42 +0000
Subject: [PATCH] Bug 1640040 - Only handle updating the DOM state from
 TextControlState::SetValue on the outer caller. r=masayuki

The issue here is that AccessibleCaret flushes layout from the middle of
the SetValue call, because it hides the caret (see the call stack in
comment 11).

That changes the placeholder-shown state because it goes from empty to
non-empty but the SetValue call from UnbindFromFrame is not supposed to
change that state (because we're in the middle of restyling).

Call OnValueChanged later, at the outer caller instead.

Differential Revision: https://phabricator.services.mozilla.com/D85745
---
 dom/html/TextControlState.cpp           | 23 ++++++++++++-----------
 layout/style/crashtests/1640040.html    | 11 +++++++++++
 layout/style/crashtests/crashtests.list |  1 +
 3 files changed, 24 insertions(+), 11 deletions(-)
 create mode 100644 layout/style/crashtests/1640040.html

diff --git a/dom/html/TextControlState.cpp b/dom/html/TextControlState.cpp
index 56e4bdcb9037..dce5d14714e9 100644
--- a/dom/html/TextControlState.cpp
+++ b/dom/html/TextControlState.cpp
@@ -2568,6 +2568,9 @@ bool TextControlState::SetValue(const nsAString& aValue,
     aOldValue = nullptr;
   }
 
+  const bool wasHandlingSetValue =
+      mHandlingState && mHandlingState->IsHandling(TextControlAction::SetValue);
+
   ErrorResult error;
   AutoTextControlHandlingState handlingSetValue(
       *this, TextControlAction::SetValue, aValue, aOldValue, aFlags, error);
@@ -2646,25 +2649,23 @@ bool TextControlState::SetValue(const nsAString& aValue,
   }
 
   if (mTextEditor && mBoundFrame) {
-    AutoWeakFrame weakFrame(mBoundFrame);
-
     if (!SetValueWithTextEditor(handlingSetValue)) {
       return false;
     }
-
-    if (!weakFrame.IsAlive()) {
-      return true;
-    }
   } else if (!SetValueWithoutTextEditor(handlingSetValue)) {
     return false;
   }
 
-  // TODO(emilio): It seems wrong to pass ValueChangeKind::Script if
-  // BySetUserInput is in aFlags.
-  auto changeKind = (aFlags & eSetValue_Internal) ? ValueChangeKind::Internal
-                                                  : ValueChangeKind::Script;
+  // If we were handling SetValue() before, don't update the DOM state twice,
+  // just let the outer call do so.
+  if (!wasHandlingSetValue) {
+    // TODO(emilio): It seems wrong to pass ValueChangeKind::Script if
+    // BySetUserInput is in aFlags.
+    auto changeKind = (aFlags & eSetValue_Internal) ? ValueChangeKind::Internal
+                                                    : ValueChangeKind::Script;
 
-  handlingSetValue.GetTextControlElement()->OnValueChanged(changeKind);
+    handlingSetValue.GetTextControlElement()->OnValueChanged(changeKind);
+  }
   return true;
 }
 
diff --git a/layout/style/crashtests/1640040.html b/layout/style/crashtests/1640040.html
new file mode 100644
index 000000000000..a1bbb4e2afb6
--- /dev/null
+++ b/layout/style/crashtests/1640040.html
@@ -0,0 +1,11 @@
+<script>
+window.onload = () => {
+  b.type = ""
+  document.getSelection().selectAllChildren(a)
+  b.placeholder = ""
+  b.setRangeText(String.fromCodePoint(814688))
+}
+</script>
+<menu spellcheck="true">
+<input id="b" autofocus="" type="button">
+<menu id="a">
diff --git a/layout/style/crashtests/crashtests.list b/layout/style/crashtests/crashtests.list
index 97aba29a14df..51dd8a6fa910 100644
--- a/layout/style/crashtests/crashtests.list
+++ b/layout/style/crashtests/crashtests.list
@@ -320,3 +320,4 @@ pref(layout.css.motion-path.enabled,true) load 1609786.html
 pref(layout.css.constructable-stylesheets.enabled,true) load 1616433.html
 pref(layout.css.constructable-stylesheets.enabled,true) load 1616407.html
 load 1639533.html
+pref(layout.accessiblecaret.enabled,true) load 1640040.html