Avoid null deref after inserting a text field with a list attribute
https://bugs.webkit.org/show_bug.cgi?id=209909
<rdar://problem/60742229>
Reviewed by Ryosuke Niwa.
Source/WebCore:
On macOS, when painting a text field with an associated datalist (i.e. `HTMLInputElement::list()` is non-null),
we assume that the datalist suggestions dropdown button has a renderer (in other words, it does not have a style
of `display: none`).
Existing logic in `TextFieldInputType` is responsible for upholding this invariant -- when the list attribute
changes on an input field (e.g. when we parse the list attribute, or when it is set by JavaScript), we update
the inline display style of `m_dataListDropdownIndicator`, such that it is set to `display: none` only if there
is either no list attribute, or the list attribute is empty, or the list does not refer to a connected datalist
element. However, there is one scenario in which this invariant is violated. Consider the following:
1. An input field is created, and its list attribute is set to "foo". Importantly, it is not connected yet.
2. A datalist element with id "foo" is then created and then added to the document.
3. The input field created in (1) is then added to the document.
In this scenario, `listAttributeTargetChanged()` is invoked after (1), but since it is not connected, it has no
datalist yet, and so `m_dataListDropdownIndicator` will remain non-rendered. When it is later added to the DOM,
nothing attempts to `m_dataListDropdownIndicator` even though its list attribute now refers to a datalist, so
it remains hidden. When we later go to paint the input's datalist dropdown button in
`RenderThemeMac::paintListButtonForInput`, we assume that the dropdown button must be rendered because the input
has a datalist and subsequently crash since `buttonElement->renderer()` remains null.
To fix this, we add logic to update the datalist dropdown button's inline display style when it is connected to
the document with an existing, non-empty list attribute.
Test: fast/forms/datalist/append-input-with-list-attribute.html
* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::parseAttribute):
(WebCore::HTMLInputElement::didFinishInsertingNode):
Notify the InputType subclass that the datalist element may have changed after an input element is connected
to the document with a non-empty list attribute.
(WebCore::HTMLInputElement::dataListMayHaveChanged):
(WebCore::ListAttributeTargetObserver::idTargetChanged):
(WebCore::HTMLInputElement::listAttributeTargetChanged): Deleted.
Rename listAttributeTargetChanged to dataListMayHaveChanged, since it is no longer called only when the list
attribute changes value, but rather when the input's datalist element may have changed.
* html/HTMLInputElement.h:
* html/InputType.cpp:
(WebCore::InputType::dataListMayHaveChanged):
(WebCore::InputType::listAttributeTargetChanged): Deleted.
* html/InputType.h:
* html/RangeInputType.cpp:
(WebCore::RangeInputType::dataListMayHaveChanged):
(WebCore::RangeInputType::listAttributeTargetChanged): Deleted.
* html/RangeInputType.h:
* html/TextFieldInputType.cpp:
(WebCore::TextFieldInputType::dataListMayHaveChanged):
(WebCore::TextFieldInputType::listAttributeTargetChanged): Deleted.
* html/TextFieldInputType.h:
LayoutTests:
Add a layout test to exercise the crashing scenario, and verify that the end result of programmatically
inserting the text field is identical to simply putting an input field with a datalist in the markup.
* fast/forms/datalist/append-input-with-list-attribute-expected.html: Added.
* fast/forms/datalist/append-input-with-list-attribute.html: Added.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@259402 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index c8ecf24..e7b32fa 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2020-04-02 Wenson Hsieh <wenson_hsieh@apple.com>
+
+ Avoid null deref after inserting a text field with a list attribute
+ https://bugs.webkit.org/show_bug.cgi?id=209909
+ <rdar://problem/60742229>
+
+ Reviewed by Ryosuke Niwa.
+
+ Add a layout test to exercise the crashing scenario, and verify that the end result of programmatically
+ inserting the text field is identical to simply putting an input field with a datalist in the markup.
+
+ * fast/forms/datalist/append-input-with-list-attribute-expected.html: Added.
+ * fast/forms/datalist/append-input-with-list-attribute.html: Added.
+
2020-04-01 Darin Adler <darin@apple.com>
Remove all uses of live ranges from TextIterator
diff --git a/LayoutTests/fast/forms/datalist/append-input-with-list-attribute-expected.html b/LayoutTests/fast/forms/datalist/append-input-with-list-attribute-expected.html
new file mode 100644
index 0000000..177672d
--- /dev/null
+++ b/LayoutTests/fast/forms/datalist/append-input-with-list-attribute-expected.html
@@ -0,0 +1,6 @@
+<!DOCTYPE html>
+<body>
+ <p>This test passes if it shows an input field with a datalist dropdown button (on platforms that show a dropdown button by default).</p>
+ <datalist id="datalist"></datalist>
+ <input list="datalist">
+</body>
\ No newline at end of file
diff --git a/LayoutTests/fast/forms/datalist/append-input-with-list-attribute.html b/LayoutTests/fast/forms/datalist/append-input-with-list-attribute.html
new file mode 100644
index 0000000..867cea8
--- /dev/null
+++ b/LayoutTests/fast/forms/datalist/append-input-with-list-attribute.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<body>
+ <p>This test passes if it shows an input field with a datalist dropdown button (on platforms that show a dropdown button by default).</p>
+ <script>
+ const input = document.createElement("input");
+ input.setAttribute("list", "datalist");
+ const datalist = document.createElement("datalist");
+ datalist.setAttribute("id", "datalist");
+ document.body.appendChild(datalist);
+ document.body.appendChild(input);
+ </script>
+</body>
\ No newline at end of file
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 7d8f2b9..fee93dd 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,65 @@
+2020-04-02 Wenson Hsieh <wenson_hsieh@apple.com>
+
+ Avoid null deref after inserting a text field with a list attribute
+ https://bugs.webkit.org/show_bug.cgi?id=209909
+ <rdar://problem/60742229>
+
+ Reviewed by Ryosuke Niwa.
+
+ On macOS, when painting a text field with an associated datalist (i.e. `HTMLInputElement::list()` is non-null),
+ we assume that the datalist suggestions dropdown button has a renderer (in other words, it does not have a style
+ of `display: none`).
+
+ Existing logic in `TextFieldInputType` is responsible for upholding this invariant -- when the list attribute
+ changes on an input field (e.g. when we parse the list attribute, or when it is set by JavaScript), we update
+ the inline display style of `m_dataListDropdownIndicator`, such that it is set to `display: none` only if there
+ is either no list attribute, or the list attribute is empty, or the list does not refer to a connected datalist
+ element. However, there is one scenario in which this invariant is violated. Consider the following:
+
+ 1. An input field is created, and its list attribute is set to "foo". Importantly, it is not connected yet.
+ 2. A datalist element with id "foo" is then created and then added to the document.
+ 3. The input field created in (1) is then added to the document.
+
+ In this scenario, `listAttributeTargetChanged()` is invoked after (1), but since it is not connected, it has no
+ datalist yet, and so `m_dataListDropdownIndicator` will remain non-rendered. When it is later added to the DOM,
+ nothing attempts to `m_dataListDropdownIndicator` even though its list attribute now refers to a datalist, so
+ it remains hidden. When we later go to paint the input's datalist dropdown button in
+ `RenderThemeMac::paintListButtonForInput`, we assume that the dropdown button must be rendered because the input
+ has a datalist and subsequently crash since `buttonElement->renderer()` remains null.
+
+ To fix this, we add logic to update the datalist dropdown button's inline display style when it is connected to
+ the document with an existing, non-empty list attribute.
+
+ Test: fast/forms/datalist/append-input-with-list-attribute.html
+
+ * html/HTMLInputElement.cpp:
+ (WebCore::HTMLInputElement::parseAttribute):
+ (WebCore::HTMLInputElement::didFinishInsertingNode):
+
+ Notify the InputType subclass that the datalist element may have changed after an input element is connected
+ to the document with a non-empty list attribute.
+
+ (WebCore::HTMLInputElement::dataListMayHaveChanged):
+ (WebCore::ListAttributeTargetObserver::idTargetChanged):
+ (WebCore::HTMLInputElement::listAttributeTargetChanged): Deleted.
+
+ Rename listAttributeTargetChanged to dataListMayHaveChanged, since it is no longer called only when the list
+ attribute changes value, but rather when the input's datalist element may have changed.
+
+ * html/HTMLInputElement.h:
+ * html/InputType.cpp:
+ (WebCore::InputType::dataListMayHaveChanged):
+ (WebCore::InputType::listAttributeTargetChanged): Deleted.
+ * html/InputType.h:
+ * html/RangeInputType.cpp:
+ (WebCore::RangeInputType::dataListMayHaveChanged):
+ (WebCore::RangeInputType::listAttributeTargetChanged): Deleted.
+ * html/RangeInputType.h:
+ * html/TextFieldInputType.cpp:
+ (WebCore::TextFieldInputType::dataListMayHaveChanged):
+ (WebCore::TextFieldInputType::listAttributeTargetChanged): Deleted.
+ * html/TextFieldInputType.h:
+
2020-04-01 Darin Adler <darin@apple.com>
Remove all uses of live ranges from TextIterator
diff --git a/Source/WebCore/html/HTMLInputElement.cpp b/Source/WebCore/html/HTMLInputElement.cpp
index f8c4dcc..b4a2033 100644
--- a/Source/WebCore/html/HTMLInputElement.cpp
+++ b/Source/WebCore/html/HTMLInputElement.cpp
@@ -798,7 +798,7 @@
m_hasNonEmptyList = !value.isEmpty();
if (m_hasNonEmptyList) {
resetListAttributeTargetObserver();
- listAttributeTargetChanged();
+ dataListMayHaveChanged();
}
}
#endif
@@ -1552,6 +1552,10 @@
HTMLTextFormControlElement::didFinishInsertingNode();
if (isInTreeScope() && !form())
addToRadioButtonGroup();
+#if ENABLE(DATALIST_ELEMENT)
+ if (isConnected() && m_hasNonEmptyList)
+ dataListMayHaveChanged();
+#endif
}
void HTMLInputElement::removedFromAncestor(RemovalType removalType, ContainerNode& oldParentOfRemovedTree)
@@ -1653,9 +1657,9 @@
m_listAttributeTargetObserver = nullptr;
}
-void HTMLInputElement::listAttributeTargetChanged()
+void HTMLInputElement::dataListMayHaveChanged()
{
- m_inputType->listAttributeTargetChanged();
+ m_inputType->dataListMayHaveChanged();
}
#endif // ENABLE(DATALIST_ELEMENT)
@@ -1965,7 +1969,7 @@
void ListAttributeTargetObserver::idTargetChanged()
{
- m_element->listAttributeTargetChanged();
+ m_element->dataListMayHaveChanged();
}
#endif
diff --git a/Source/WebCore/html/HTMLInputElement.h b/Source/WebCore/html/HTMLInputElement.h
index 3deab6b..dfc3f90 100644
--- a/Source/WebCore/html/HTMLInputElement.h
+++ b/Source/WebCore/html/HTMLInputElement.h
@@ -276,7 +276,7 @@
#if ENABLE(DATALIST_ELEMENT)
WEBCORE_EXPORT RefPtr<HTMLElement> list() const;
RefPtr<HTMLDataListElement> dataList() const;
- void listAttributeTargetChanged();
+ void dataListMayHaveChanged();
#endif
Vector<Ref<HTMLInputElement>> radioButtonGroup() const;
diff --git a/Source/WebCore/html/InputType.cpp b/Source/WebCore/html/InputType.cpp
index f57fb6a..10c81cc 100644
--- a/Source/WebCore/html/InputType.cpp
+++ b/Source/WebCore/html/InputType.cpp
@@ -926,7 +926,7 @@
}
#if ENABLE(DATALIST_ELEMENT)
-void InputType::listAttributeTargetChanged()
+void InputType::dataListMayHaveChanged()
{
}
diff --git a/Source/WebCore/html/InputType.h b/Source/WebCore/html/InputType.h
index 208bf25..128a8ee 100644
--- a/Source/WebCore/html/InputType.h
+++ b/Source/WebCore/html/InputType.h
@@ -301,7 +301,7 @@
void dispatchSimulatedClickIfActive(KeyboardEvent&) const;
#if ENABLE(DATALIST_ELEMENT)
- virtual void listAttributeTargetChanged();
+ virtual void dataListMayHaveChanged();
virtual Optional<Decimal> findClosestTickMarkValue(const Decimal&);
#endif
diff --git a/Source/WebCore/html/RangeInputType.cpp b/Source/WebCore/html/RangeInputType.cpp
index d9d2612..bc649df 100644
--- a/Source/WebCore/html/RangeInputType.cpp
+++ b/Source/WebCore/html/RangeInputType.cpp
@@ -374,7 +374,7 @@
}
#if ENABLE(DATALIST_ELEMENT)
-void RangeInputType::listAttributeTargetChanged()
+void RangeInputType::dataListMayHaveChanged()
{
m_tickMarkValuesDirty = true;
RefPtr<HTMLElement> sliderTrackElement = this->sliderTrackElement();
diff --git a/Source/WebCore/html/RangeInputType.h b/Source/WebCore/html/RangeInputType.h
index 51fb79d..4327c948 100644
--- a/Source/WebCore/html/RangeInputType.h
+++ b/Source/WebCore/html/RangeInputType.h
@@ -68,7 +68,7 @@
SliderThumbElement& typedSliderThumbElement() const;
#if ENABLE(DATALIST_ELEMENT)
- void listAttributeTargetChanged() final;
+ void dataListMayHaveChanged() final;
void updateTickMarkValues();
Optional<Decimal> findClosestTickMarkValue(const Decimal&) final;
diff --git a/Source/WebCore/html/TextFieldInputType.cpp b/Source/WebCore/html/TextFieldInputType.cpp
index 7895a7e..743c32a 100644
--- a/Source/WebCore/html/TextFieldInputType.cpp
+++ b/Source/WebCore/html/TextFieldInputType.cpp
@@ -835,7 +835,7 @@
#if ENABLE(DATALIST_ELEMENT)
-void TextFieldInputType::listAttributeTargetChanged()
+void TextFieldInputType::dataListMayHaveChanged()
{
m_cachedSuggestions = { };
diff --git a/Source/WebCore/html/TextFieldInputType.h b/Source/WebCore/html/TextFieldInputType.h
index d1d89a7..2c45722 100644
--- a/Source/WebCore/html/TextFieldInputType.h
+++ b/Source/WebCore/html/TextFieldInputType.h
@@ -126,7 +126,7 @@
#if ENABLE(DATALIST_ELEMENT)
void createDataListDropdownIndicator();
bool isPresentingAttachedView() const final;
- void listAttributeTargetChanged() final;
+ void dataListMayHaveChanged() final;
void displaySuggestions(DataListSuggestionActivationType);
void closeSuggestions();
