Title: [243572] branches/safari-607-branch
- Revision
- 243572
- Author
- alanc...@apple.com
- Date
- 2019-03-27 16:43:37 -0700 (Wed, 27 Mar 2019)
Log Message
Cherry-pick r242917. rdar://problem/49307956
Fix an edge case where HTMLFormElement::removeFormElement is invoked twice with the same element
https://bugs.webkit.org/show_bug.cgi?id=195663
<rdar://problem/48576391>
Reviewed by Ryosuke Niwa.
Source/WebCore:
Currently, it's possible for HTMLFormControlElement's destructor to be reentrant. This may happen if the form
control element is ref'd while carrying out its destructor's logic. This may happen in two places in
HTMLFormControlElement (didChangeForm and resetDefaultButton), both of which actually don't require ensuring a
protected reference to the form control element since they should never result in any script execution.
To fix the bug, convert these strong references into raw pointers, and add ScriptDisallowedScope to ensure that
we don't change these codepaths in the future, such that they trigger arbitrary script execution.
Test: fast/forms/remove-associated-element-after-gc.html
* html/HTMLFormControlElement.cpp:
(WebCore::HTMLFormControlElement::didChangeForm):
* html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::resetDefaultButton):
LayoutTests:
Add a layout test to exercise the scenario described in the WebCore ChangeLog.
* fast/forms/remove-associated-element-after-gc-expected.txt: Added.
* fast/forms/remove-associated-element-after-gc.html: Added.
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242917 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-607-branch/LayoutTests/ChangeLog (243571 => 243572)
--- branches/safari-607-branch/LayoutTests/ChangeLog 2019-03-27 23:43:34 UTC (rev 243571)
+++ branches/safari-607-branch/LayoutTests/ChangeLog 2019-03-27 23:43:37 UTC (rev 243572)
@@ -1,3 +1,52 @@
+2019-03-27 Alan Coon <alanc...@apple.com>
+
+ Cherry-pick r242917. rdar://problem/49307956
+
+ Fix an edge case where HTMLFormElement::removeFormElement is invoked twice with the same element
+ https://bugs.webkit.org/show_bug.cgi?id=195663
+ <rdar://problem/48576391>
+
+ Reviewed by Ryosuke Niwa.
+
+ Source/WebCore:
+
+ Currently, it's possible for HTMLFormControlElement's destructor to be reentrant. This may happen if the form
+ control element is ref'd while carrying out its destructor's logic. This may happen in two places in
+ HTMLFormControlElement (didChangeForm and resetDefaultButton), both of which actually don't require ensuring a
+ protected reference to the form control element since they should never result in any script execution.
+
+ To fix the bug, convert these strong references into raw pointers, and add ScriptDisallowedScope to ensure that
+ we don't change these codepaths in the future, such that they trigger arbitrary script execution.
+
+ Test: fast/forms/remove-associated-element-after-gc.html
+
+ * html/HTMLFormControlElement.cpp:
+ (WebCore::HTMLFormControlElement::didChangeForm):
+ * html/HTMLFormElement.cpp:
+ (WebCore::HTMLFormElement::resetDefaultButton):
+
+ LayoutTests:
+
+ Add a layout test to exercise the scenario described in the WebCore ChangeLog.
+
+ * fast/forms/remove-associated-element-after-gc-expected.txt: Added.
+ * fast/forms/remove-associated-element-after-gc.html: Added.
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242917 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-03-13 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Fix an edge case where HTMLFormElement::removeFormElement is invoked twice with the same element
+ https://bugs.webkit.org/show_bug.cgi?id=195663
+ <rdar://problem/48576391>
+
+ Reviewed by Ryosuke Niwa.
+
+ Add a layout test to exercise the scenario described in the WebCore ChangeLog.
+
+ * fast/forms/remove-associated-element-after-gc-expected.txt: Added.
+ * fast/forms/remove-associated-element-after-gc.html: Added.
+
2019-03-18 Kocsen Chung <kocsen_ch...@apple.com>
Apply patch. rdar://problem/48839387
Added: branches/safari-607-branch/LayoutTests/fast/forms/remove-associated-element-after-gc-expected.txt (0 => 243572)
--- branches/safari-607-branch/LayoutTests/fast/forms/remove-associated-element-after-gc-expected.txt (rev 0)
+++ branches/safari-607-branch/LayoutTests/fast/forms/remove-associated-element-after-gc-expected.txt 2019-03-27 23:43:37 UTC (rev 243572)
@@ -0,0 +1 @@
+PASS
Added: branches/safari-607-branch/LayoutTests/fast/forms/remove-associated-element-after-gc.html (0 => 243572)
--- branches/safari-607-branch/LayoutTests/fast/forms/remove-associated-element-after-gc.html (rev 0)
+++ branches/safari-607-branch/LayoutTests/fast/forms/remove-associated-element-after-gc.html 2019-03-27 23:43:37 UTC (rev 243572)
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+input:default {
+ color: red;
+}
+</style>
+</head>
+<body>
+<table>
+<form><input type="submit"></form>
+</table>
+<p>This test passes if we avoid crashing, and if the green text "PASS" appears. This test requires DumpRenderTree or WebKitTestRunner.</p>
+</body>
+<script>
+if (window.testRunner) {
+ testRunner.waitUntilDone();
+ testRunner.dumpAsText();
+}
+
+document.addEventListener("DOMContentLoaded", () => {
+ let currentIteration = 1;
+ const href = ""
+ const index = href.lastIndexOf("#");
+ if (index !== -1)
+ currentIteration = parseInt(href.substring(index + 1));
+
+ if (currentIteration === 5) {
+ document.writeln("<pre style='color: green'>PASS</pre>");
+ if (window.testRunner)
+ testRunner.notifyDone();
+ return;
+ }
+
+ if (window.GCController)
+ GCController.collect();
+
+ location.href = "" index)}#${currentIteration + 1}`;
+ location.reload();
+}, false);
+</script>
+</html>
Modified: branches/safari-607-branch/Source/WebCore/ChangeLog (243571 => 243572)
--- branches/safari-607-branch/Source/WebCore/ChangeLog 2019-03-27 23:43:34 UTC (rev 243571)
+++ branches/safari-607-branch/Source/WebCore/ChangeLog 2019-03-27 23:43:37 UTC (rev 243572)
@@ -1,3 +1,62 @@
+2019-03-27 Alan Coon <alanc...@apple.com>
+
+ Cherry-pick r242917. rdar://problem/49307956
+
+ Fix an edge case where HTMLFormElement::removeFormElement is invoked twice with the same element
+ https://bugs.webkit.org/show_bug.cgi?id=195663
+ <rdar://problem/48576391>
+
+ Reviewed by Ryosuke Niwa.
+
+ Source/WebCore:
+
+ Currently, it's possible for HTMLFormControlElement's destructor to be reentrant. This may happen if the form
+ control element is ref'd while carrying out its destructor's logic. This may happen in two places in
+ HTMLFormControlElement (didChangeForm and resetDefaultButton), both of which actually don't require ensuring a
+ protected reference to the form control element since they should never result in any script execution.
+
+ To fix the bug, convert these strong references into raw pointers, and add ScriptDisallowedScope to ensure that
+ we don't change these codepaths in the future, such that they trigger arbitrary script execution.
+
+ Test: fast/forms/remove-associated-element-after-gc.html
+
+ * html/HTMLFormControlElement.cpp:
+ (WebCore::HTMLFormControlElement::didChangeForm):
+ * html/HTMLFormElement.cpp:
+ (WebCore::HTMLFormElement::resetDefaultButton):
+
+ LayoutTests:
+
+ Add a layout test to exercise the scenario described in the WebCore ChangeLog.
+
+ * fast/forms/remove-associated-element-after-gc-expected.txt: Added.
+ * fast/forms/remove-associated-element-after-gc.html: Added.
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242917 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-03-13 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Fix an edge case where HTMLFormElement::removeFormElement is invoked twice with the same element
+ https://bugs.webkit.org/show_bug.cgi?id=195663
+ <rdar://problem/48576391>
+
+ Reviewed by Ryosuke Niwa.
+
+ Currently, it's possible for HTMLFormControlElement's destructor to be reentrant. This may happen if the form
+ control element is ref'd while carrying out its destructor's logic. This may happen in two places in
+ HTMLFormControlElement (didChangeForm and resetDefaultButton), both of which actually don't require ensuring a
+ protected reference to the form control element since they should never result in any script execution.
+
+ To fix the bug, convert these strong references into raw pointers, and add ScriptDisallowedScope to ensure that
+ we don't change these codepaths in the future, such that they trigger arbitrary script execution.
+
+ Test: fast/forms/remove-associated-element-after-gc.html
+
+ * html/HTMLFormControlElement.cpp:
+ (WebCore::HTMLFormControlElement::didChangeForm):
+ * html/HTMLFormElement.cpp:
+ (WebCore::HTMLFormElement::resetDefaultButton):
+
2019-03-18 Kocsen Chung <kocsen_ch...@apple.com>
Apply patch. rdar://problem/48839387
Modified: branches/safari-607-branch/Source/WebCore/html/HTMLFormControlElement.cpp (243571 => 243572)
--- branches/safari-607-branch/Source/WebCore/html/HTMLFormControlElement.cpp 2019-03-27 23:43:34 UTC (rev 243571)
+++ branches/safari-607-branch/Source/WebCore/html/HTMLFormControlElement.cpp 2019-03-27 23:43:37 UTC (rev 243572)
@@ -40,6 +40,7 @@
#include "HTMLTextAreaElement.h"
#include "RenderBox.h"
#include "RenderTheme.h"
+#include "ScriptDisallowedScope.h"
#include "Settings.h"
#include "StyleTreeResolver.h"
#include "ValidationMessage.h"
@@ -545,8 +546,10 @@
void HTMLFormControlElement::didChangeForm()
{
+ ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+
FormAssociatedElement::didChangeForm();
- if (RefPtr<HTMLFormElement> form = this->form()) {
+ if (auto* form = this->form()) {
if (m_willValidateInitialized && m_willValidate && !isValidFormControlElement())
form->registerInvalidAssociatedFormControl(*this);
}
Modified: branches/safari-607-branch/Source/WebCore/html/HTMLFormElement.cpp (243571 => 243572)
--- branches/safari-607-branch/Source/WebCore/html/HTMLFormElement.cpp 2019-03-27 23:43:34 UTC (rev 243571)
+++ branches/safari-607-branch/Source/WebCore/html/HTMLFormElement.cpp 2019-03-27 23:43:37 UTC (rev 243572)
@@ -704,7 +704,9 @@
return;
}
- RefPtr<HTMLFormControlElement> oldDefault = m_defaultButton;
+ ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+
+ auto* oldDefault = m_defaultButton;
m_defaultButton = nullptr;
defaultButton();
if (m_defaultButton != oldDefault) {
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
