Title: [212025] trunk
- Revision
- 212025
- Author
- bfulg...@apple.com
- Date
- 2017-02-09 18:13:07 -0800 (Thu, 09 Feb 2017)
Log Message
Crash under HTMLFormElement::registerFormElement()
https://bugs.webkit.org/show_bug.cgi?id=167162
Patch by Chris Dumez <cdu...@apple.com> on 2017-02-09
Reviewed by Ryosuke Niwa.
Source/WebCore:
didMoveToNewDocument() was re-registering FormAttributeTargetObserver
even if the element's inDocument was not set yet. As a result, it was
possible for FormAssociatedElement::resetFormOwner() to be called
when the element was in the tree but with its inDocument still being
false (because insertedInto() has not been called yet). This could
end up calling HTMLFormElement::registerFormElement() even though
the element is still recognized as detached. This is an issue because
HTMLFormElement::m_associatedElements's order and its corresponding
indexes (m_associatedElementsBeforeIndex / m_associatedElementsAfterIndex)
rely on the position of the element with regards to the form element
(before / inside / after).
To address the issue, we now only register the FormAttributeTargetObserver
in didMoveToNewDocument() if the inDocument flag is set to true. This
is similar to what is done at other call sites of
resetFormAttributeTargetObserver(). We also ignore the form content
attribute in HTMLFormElement::formElementIndex() if the element is
not connected.
As per the HTML specification [1], the form content attribute is only
taken if the element is connected (i.e. inDocument flag is true).
Note that FormAssociatedElement::findAssociatedForm() was already
ignoring the form content attribute if the element is disconnected.
[1] https://html.spec.whatwg.org/#reset-the-form-owner (step 3)
Test: fast/forms/registerFormElement-crash.html
* html/FormAssociatedElement.cpp:
(WebCore::FormAssociatedElement::didMoveToNewDocument):
Only call resetFormAttributeTargetObserver() if inDocument flag is set,
similarly to what is done at other call sites.
(WebCore::FormAssociatedElement::resetFormAttributeTargetObserver):
Add an assertion to make sure no one call this method on an element that
is not connected.
* html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::formElementIndex):
Ignore the form content attribute if the element is not connected, as
per the HTML specification [1].
LayoutTests:
Add layout test coverage.
* fast/forms/registerFormElement-crash-expected.txt: Added.
* fast/forms/registerFormElement-crash.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (212024 => 212025)
--- trunk/LayoutTests/ChangeLog 2017-02-10 02:11:28 UTC (rev 212024)
+++ trunk/LayoutTests/ChangeLog 2017-02-10 02:13:07 UTC (rev 212025)
@@ -1,3 +1,15 @@
+2017-02-09 Chris Dumez <cdu...@apple.com>
+
+ Crash under HTMLFormElement::registerFormElement()
+ https://bugs.webkit.org/show_bug.cgi?id=167162
+
+ Reviewed by Ryosuke Niwa.
+
+ Add layout test coverage.
+
+ * fast/forms/registerFormElement-crash-expected.txt: Added.
+ * fast/forms/registerFormElement-crash.html: Added.
+
2017-02-09 Antti Koivisto <an...@apple.com>
Tear down existing renderers when adding a shadow root.
Added: trunk/LayoutTests/fast/forms/registerFormElement-crash-expected.txt (0 => 212025)
--- trunk/LayoutTests/fast/forms/registerFormElement-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/forms/registerFormElement-crash-expected.txt 2017-02-10 02:13:07 UTC (rev 212025)
@@ -0,0 +1,2 @@
+CONSOLE MESSAGE: line 17: PASS: did not crash
+
Added: trunk/LayoutTests/fast/forms/registerFormElement-crash.html (0 => 212025)
--- trunk/LayoutTests/fast/forms/registerFormElement-crash.html (rev 0)
+++ trunk/LayoutTests/fast/forms/registerFormElement-crash.html 2017-02-10 02:13:07 UTC (rev 212025)
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+function runTest() {
+ var iframe = document.getElementById("iframe");
+ var iframeWindow = window[0];
+ var toInsert = div;
+ var iframeBody = iframeWindow.document.body;
+ iframeBody.before(document.body);
+ iframe.after(toInsert);
+ console.log("PASS: did not crash");
+ if (window.testRunner)
+ testRunner.notifyDone();
+}
+</script>
+</head>
+<body _onload_="runTest()">
+ <p>This test passes if it does not crash.</p>
+ <form id="form">
+ <textarea form="form">aaaaaaaa</textarea>
+ <iframe id="iframe"></iframe>
+ </form>
+ <div id="div">
+ <dir><object id=“wtf"></object></dir>
+ <div>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (212024 => 212025)
--- trunk/Source/WebCore/ChangeLog 2017-02-10 02:11:28 UTC (rev 212024)
+++ trunk/Source/WebCore/ChangeLog 2017-02-10 02:13:07 UTC (rev 212025)
@@ -1,3 +1,53 @@
+2017-02-09 Chris Dumez <cdu...@apple.com>
+
+ Crash under HTMLFormElement::registerFormElement()
+ https://bugs.webkit.org/show_bug.cgi?id=167162
+
+ Reviewed by Ryosuke Niwa.
+
+ didMoveToNewDocument() was re-registering FormAttributeTargetObserver
+ even if the element's inDocument was not set yet. As a result, it was
+ possible for FormAssociatedElement::resetFormOwner() to be called
+ when the element was in the tree but with its inDocument still being
+ false (because insertedInto() has not been called yet). This could
+ end up calling HTMLFormElement::registerFormElement() even though
+ the element is still recognized as detached. This is an issue because
+ HTMLFormElement::m_associatedElements's order and its corresponding
+ indexes (m_associatedElementsBeforeIndex / m_associatedElementsAfterIndex)
+ rely on the position of the element with regards to the form element
+ (before / inside / after).
+
+ To address the issue, we now only register the FormAttributeTargetObserver
+ in didMoveToNewDocument() if the inDocument flag is set to true. This
+ is similar to what is done at other call sites of
+ resetFormAttributeTargetObserver(). We also ignore the form content
+ attribute in HTMLFormElement::formElementIndex() if the element is
+ not connected.
+
+ As per the HTML specification [1], the form content attribute is only
+ taken if the element is connected (i.e. inDocument flag is true).
+
+ Note that FormAssociatedElement::findAssociatedForm() was already
+ ignoring the form content attribute if the element is disconnected.
+
+ [1] https://html.spec.whatwg.org/#reset-the-form-owner (step 3)
+
+ Test: fast/forms/registerFormElement-crash.html
+
+ * html/FormAssociatedElement.cpp:
+ (WebCore::FormAssociatedElement::didMoveToNewDocument):
+ Only call resetFormAttributeTargetObserver() if inDocument flag is set,
+ similarly to what is done at other call sites.
+
+ (WebCore::FormAssociatedElement::resetFormAttributeTargetObserver):
+ Add an assertion to make sure no one call this method on an element that
+ is not connected.
+
+ * html/HTMLFormElement.cpp:
+ (WebCore::HTMLFormElement::formElementIndex):
+ Ignore the form content attribute if the element is not connected, as
+ per the HTML specification [1].
+
2017-02-09 Antti Koivisto <an...@apple.com>
Tear down existing renderers when adding a shadow root.
Modified: trunk/Source/WebCore/html/FormAssociatedElement.cpp (212024 => 212025)
--- trunk/Source/WebCore/html/FormAssociatedElement.cpp 2017-02-10 02:11:28 UTC (rev 212024)
+++ trunk/Source/WebCore/html/FormAssociatedElement.cpp 2017-02-10 02:13:07 UTC (rev 212025)
@@ -63,7 +63,7 @@
void FormAssociatedElement::didMoveToNewDocument(Document&)
{
HTMLElement& element = asHTMLElement();
- if (element.hasAttributeWithoutSynchronization(formAttr))
+ if (element.hasAttributeWithoutSynchronization(formAttr) && element.inDocument())
resetFormAttributeTargetObserver();
}
@@ -266,6 +266,7 @@
void FormAssociatedElement::resetFormAttributeTargetObserver()
{
+ ASSERT_WITH_SECURITY_IMPLICATION(asHTMLElement().inDocument());
m_formAttributeTargetObserver = std::make_unique<FormAttributeTargetObserver>(asHTMLElement().attributeWithoutSynchronization(formAttr), *this);
}
Modified: trunk/Source/WebCore/html/HTMLFormElement.cpp (212024 => 212025)
--- trunk/Source/WebCore/html/HTMLFormElement.cpp 2017-02-10 02:11:28 UTC (rev 212024)
+++ trunk/Source/WebCore/html/HTMLFormElement.cpp 2017-02-10 02:13:07 UTC (rev 212025)
@@ -547,8 +547,9 @@
// Treats separately the case where this element has the form attribute
// for performance consideration.
- if (associatedHTMLElement.hasAttributeWithoutSynchronization(formAttr)) {
+ if (associatedHTMLElement.hasAttributeWithoutSynchronization(formAttr) && associatedHTMLElement.inDocument()) {
unsigned short position = compareDocumentPosition(associatedHTMLElement);
+ ASSERT_WITH_SECURITY_IMPLICATION(!(position & DOCUMENT_POSITION_DISCONNECTED));
if (position & DOCUMENT_POSITION_PRECEDING) {
++m_associatedElementsBeforeIndex;
++m_associatedElementsAfterIndex;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
