Fix an edge case where HTMLFormElement::removeFormElement is invoked twice with the...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Mar 2019 23:18:26 +0000 (23:18 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Mar 2019 23:18:26 +0000 (23:18 +0000)
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

LayoutTests/ChangeLog
LayoutTests/fast/forms/remove-associated-element-after-gc-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/remove-associated-element-after-gc.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLFormControlElement.cpp
Source/WebCore/html/HTMLFormElement.cpp

index d27abea..adbccfa 100644 (file)
@@ -1,3 +1,16 @@
+2019-03-13  Wenson Hsieh  <wenson_hsieh@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-13  Nikita Vasilyev  <nvasilyev@apple.com>
 
         REGRESSION(r240946): Web Inspector: Styles: removing selected property doesn't update overridden status
diff --git a/LayoutTests/fast/forms/remove-associated-element-after-gc-expected.txt b/LayoutTests/fast/forms/remove-associated-element-after-gc-expected.txt
new file mode 100644 (file)
index 0000000..7ef22e9
--- /dev/null
@@ -0,0 +1 @@
+PASS
diff --git a/LayoutTests/fast/forms/remove-associated-element-after-gc.html b/LayoutTests/fast/forms/remove-associated-element-after-gc.html
new file mode 100644 (file)
index 0000000..e8db38f
--- /dev/null
@@ -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 = location.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 = `${href.substring(0, index)}#${currentIteration + 1}`;
+    location.reload();
+}, false);
+</script>
+</html>
index 7b63b55..24d5223 100644 (file)
@@ -1,3 +1,26 @@
+2019-03-13  Wenson Hsieh  <wenson_hsieh@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-13  Daniel Bates  <dabates@apple.com>
 
         Remove some unnecessary !USE(UIKIT_KEYBOARD_ADDITIONS) guards
index 47cac57..0f76279 100644 (file)
@@ -41,6 +41,7 @@
 #include "Quirks.h"
 #include "RenderBox.h"
 #include "RenderTheme.h"
+#include "ScriptDisallowedScope.h"
 #include "Settings.h"
 #include "StyleTreeResolver.h"
 #include "ValidationMessage.h"
@@ -557,8 +558,10 @@ void HTMLFormControlElement::willChangeForm()
 
 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);
     }
index bbf1648..ab63b89 100644 (file)
@@ -704,7 +704,9 @@ void HTMLFormElement::resetDefaultButton()
         return;
     }
 
-    RefPtr<HTMLFormControlElement> oldDefault = m_defaultButton;
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+
+    auto* oldDefault = m_defaultButton;
     m_defaultButton = nullptr;
     defaultButton();
     if (m_defaultButton != oldDefault) {