Title: [242309] trunk
- Revision
- 242309
- Author
- [email protected]
- Date
- 2019-03-01 22:09:31 -0800 (Fri, 01 Mar 2019)
Log Message
[Datalist] fast/forms/datalist/datalist-child-validation.html crashes with a debug assertion in isValidFormControlElement()
https://bugs.webkit.org/show_bug.cgi?id=190620
<rdar://problem/19226679>
Reviewed by Ryosuke Niwa.
Source/WebCore:
Fixes and re-enables an existing layout test that is asserting on debug builds (and failing on release builds).
To understand why we hit this assertion, we first note several observations:
- The validity of a form control (`isValid()`) depends on the value of `willValidate()`.
- Both of these results are cached in member variables: `m_isValid` and `m_willValidate`, respectively.
- `willValidate()` changes the cached value of `m_willValidate` if necessary, but `isValid()` uses the
cached value without update.
Now, consider the following scenario:
1. Something changes in the DOM that changes the result of `willValidate()`. This can happen as a result of
several things:
a. The form control changes readonly state
b. The form control changes disabled state
c. The form control is added to a datalist element
d. The form control is removed from a datalist element
2. Call `willValidate()`.
3. Call `isValid()`.
In scenarios (a) - (c), we ensure that cached form control validity (`m_isValid`) is updated alongside
`m_willValidate` by invoking `setNeedsWillValidateCheck()`, such that the result of `isValid()` matches the
result of `m_isValid` in step (3). However, in the last scenario (d), we don't do this, which causes form
control validity to fall out of sync with the result of `isValid()`. To fix the bug, we update willValidate and
isValid when a form control is removed from an ancestor, only if one of its ancestors is a datalist element.
* html/HTMLFormControlElement.cpp:
(WebCore::HTMLFormControlElement::insertedIntoAncestor):
(WebCore::HTMLFormControlElement::removedFromAncestor):
Make a couple of minor tweaks:
- Currently, we always invalidate `m_dataListAncestorState` by resetting the state to `Unknown` when the form
control is removed from an ancestor or inserted. Instead, we only need to reset it when the form control
already has an ancestor that is a datalist (in the case where it's being removed) or when the form control
does not yet have an ancestor (in the case where it is being added).
- If the form control was inside a datalist prior to removal, recompute its cached value of `m_willValidate`,
as well as its cached validity (`m_isValid`).
LayoutTests:
Re-enables a crashing layout test. See WebCore ChangeLog for more details.
* platform/ios/TestExpectations:
* platform/mac/TestExpectations:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (242308 => 242309)
--- trunk/LayoutTests/ChangeLog 2019-03-02 05:45:11 UTC (rev 242308)
+++ trunk/LayoutTests/ChangeLog 2019-03-02 06:09:31 UTC (rev 242309)
@@ -1,3 +1,16 @@
+2019-03-01 Wenson Hsieh <[email protected]>
+
+ [Datalist] fast/forms/datalist/datalist-child-validation.html crashes with a debug assertion in isValidFormControlElement()
+ https://bugs.webkit.org/show_bug.cgi?id=190620
+ <rdar://problem/19226679>
+
+ Reviewed by Ryosuke Niwa.
+
+ Re-enables a crashing layout test. See WebCore ChangeLog for more details.
+
+ * platform/ios/TestExpectations:
+ * platform/mac/TestExpectations:
+
2019-03-01 Zalan Bujtas <[email protected]>
[ContentChangeObserver] Check for pending style recalcs at the end of each timer run.
Modified: trunk/LayoutTests/platform/ios/TestExpectations (242308 => 242309)
--- trunk/LayoutTests/platform/ios/TestExpectations 2019-03-02 05:45:11 UTC (rev 242308)
+++ trunk/LayoutTests/platform/ios/TestExpectations 2019-03-02 06:09:31 UTC (rev 242309)
@@ -1962,8 +1962,6 @@
# <rdar://problem/19226623> ASSERT(isIdentifierStart<CharacterType>()) fails in CSSParser::parseIdentifier()
fast/css/css-selector-text.html
-webkit.org/b/190620 fast/forms/datalist/datalist-child-validation.html [ Skip ]
-
# <rdar://problem/19227549> ASSERT(!m_webFrame->_private->provisionalURL) fails in WebFrameLoaderClient::dispatchDidStartProvisionalLoad()
fast/forms/validation-message-user-modify.html
Modified: trunk/LayoutTests/platform/mac/TestExpectations (242308 => 242309)
--- trunk/LayoutTests/platform/mac/TestExpectations 2019-03-02 05:45:11 UTC (rev 242308)
+++ trunk/LayoutTests/platform/mac/TestExpectations 2019-03-02 06:09:31 UTC (rev 242309)
@@ -131,9 +131,6 @@
# EventSender::dumpFilenameBeingDragged not implemented.
webkit.org/b/61827 fast/events/drag-image-filename.html
-# Datalist
-webkit.org/b/190620 fast/forms/datalist/datalist-child-validation.html [ Skip ]
-
# ENABLE_INPUT_TYPE_* are not enabled.
# https://bugs.webkit.org/show_bug.cgi?id=29359
fast/forms/date
Modified: trunk/Source/WebCore/ChangeLog (242308 => 242309)
--- trunk/Source/WebCore/ChangeLog 2019-03-02 05:45:11 UTC (rev 242308)
+++ trunk/Source/WebCore/ChangeLog 2019-03-02 06:09:31 UTC (rev 242309)
@@ -1,3 +1,48 @@
+2019-03-01 Wenson Hsieh <[email protected]>
+
+ [Datalist] fast/forms/datalist/datalist-child-validation.html crashes with a debug assertion in isValidFormControlElement()
+ https://bugs.webkit.org/show_bug.cgi?id=190620
+ <rdar://problem/19226679>
+
+ Reviewed by Ryosuke Niwa.
+
+ Fixes and re-enables an existing layout test that is asserting on debug builds (and failing on release builds).
+ To understand why we hit this assertion, we first note several observations:
+
+ - The validity of a form control (`isValid()`) depends on the value of `willValidate()`.
+ - Both of these results are cached in member variables: `m_isValid` and `m_willValidate`, respectively.
+ - `willValidate()` changes the cached value of `m_willValidate` if necessary, but `isValid()` uses the
+ cached value without update.
+
+ Now, consider the following scenario:
+
+ 1. Something changes in the DOM that changes the result of `willValidate()`. This can happen as a result of
+ several things:
+ a. The form control changes readonly state
+ b. The form control changes disabled state
+ c. The form control is added to a datalist element
+ d. The form control is removed from a datalist element
+ 2. Call `willValidate()`.
+ 3. Call `isValid()`.
+
+ In scenarios (a) - (c), we ensure that cached form control validity (`m_isValid`) is updated alongside
+ `m_willValidate` by invoking `setNeedsWillValidateCheck()`, such that the result of `isValid()` matches the
+ result of `m_isValid` in step (3). However, in the last scenario (d), we don't do this, which causes form
+ control validity to fall out of sync with the result of `isValid()`. To fix the bug, we update willValidate and
+ isValid when a form control is removed from an ancestor, only if one of its ancestors is a datalist element.
+
+ * html/HTMLFormControlElement.cpp:
+ (WebCore::HTMLFormControlElement::insertedIntoAncestor):
+ (WebCore::HTMLFormControlElement::removedFromAncestor):
+
+ Make a couple of minor tweaks:
+ - Currently, we always invalidate `m_dataListAncestorState` by resetting the state to `Unknown` when the form
+ control is removed from an ancestor or inserted. Instead, we only need to reset it when the form control
+ already has an ancestor that is a datalist (in the case where it's being removed) or when the form control
+ does not yet have an ancestor (in the case where it is being added).
+ - If the form control was inside a datalist prior to removal, recompute its cached value of `m_willValidate`,
+ as well as its cached validity (`m_isValid`).
+
2019-03-01 Darin Adler <[email protected]>
Finish removing String::format
Modified: trunk/Source/WebCore/html/HTMLFormControlElement.cpp (242308 => 242309)
--- trunk/Source/WebCore/html/HTMLFormControlElement.cpp 2019-03-02 05:45:11 UTC (rev 242308)
+++ trunk/Source/WebCore/html/HTMLFormControlElement.cpp 2019-03-02 06:09:31 UTC (rev 242309)
@@ -280,7 +280,9 @@
Node::InsertedIntoAncestorResult HTMLFormControlElement::insertedIntoAncestor(InsertionType insertionType, ContainerNode& parentOfInsertedTree)
{
- m_dataListAncestorState = Unknown;
+ if (m_dataListAncestorState == NotInsideDataList)
+ m_dataListAncestorState = Unknown;
+
setNeedsWillValidateCheck();
if (willValidate() && !isValidFormControlElement())
addInvalidElementToAncestorFromInsertionPoint(*this, &parentOfInsertedTree);
@@ -303,12 +305,21 @@
m_validationMessage = nullptr;
if (m_disabledByAncestorFieldset)
setAncestorDisabled(computeIsDisabledByFieldsetAncestor());
- m_dataListAncestorState = Unknown;
+
+ bool wasInsideDataList = false;
+ if (m_dataListAncestorState == InsideDataList) {
+ m_dataListAncestorState = Unknown;
+ wasInsideDataList = true;
+ }
+
HTMLElement::removedFromAncestor(removalType, oldParentOfRemovedTree);
FormAssociatedElement::removedFromAncestor(removalType, oldParentOfRemovedTree);
if (wasMatchingInvalidPseudoClass)
removeInvalidElementToAncestorFromInsertionPoint(*this, &oldParentOfRemovedTree);
+
+ if (wasInsideDataList)
+ setNeedsWillValidateCheck();
}
void HTMLFormControlElement::setChangedSinceLastFormControlChangeEvent(bool changed)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
