2009-07-27 Ojan Vafai <ojan@chromium.org>
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 27 Jul 2009 22:45:37 +0000 (22:45 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 27 Jul 2009 22:45:37 +0000 (22:45 +0000)
        Reviewed by Darin Adler.

        https://bugs.webkit.org/show_bug.cgi?id=27474
        Tests crashes when calling select, setSelectionRange or setting
        selectionStart/selectionEnd on a textarea/input immediately after
        setting display:none.

        * fast/dom/text-control-crash-on-select-expected.txt: Added.
        * fast/dom/text-control-crash-on-select.html: Added.

2009-07-27  Ojan Vafai  <ojan@chromium.org>

        Reviewed by Darin Adler.

        https://bugs.webkit.org/show_bug.cgi?id=27474
        Fixes crashes due to renderer getting destroyed in updateLayout.
        We need to call updateLayout before we call into the renderer.
        Removed the updateLayout call from RenderTextControl and moved it
        into the calling sites.

        Also changes updateLayout to updateLayoutIgnorePendingStylesheets so
        this works with pending stylesheets. Unfortunately, this seems to be
        untestable. Loading an external stylesheet and then having an inline
        script hit this code did not result in an pending stylesheets.

        The are other cases of this bug in the rendering code. I'll file a
        followup bug to audit the calls to updateLayout.

        Test: fast/dom/text-control-crash-on-select.html

        * dom/Document.h:
        (WebCore::Document::inStyleRecalc): Added so the ASSERTs in updateFocusAppearance
            and setSelectionRange could deal with cases of reentrancy into updateLayout
            calls. This happens in a couple layout tests.
        * dom/InputElement.cpp:
        (WebCore::InputElement::updateSelectionRange):
        * html/HTMLInputElement.cpp:
        (WebCore::isTextFieldWithRendererAfterUpdateLayout):
        (WebCore::HTMLInputElement::setSelectionStart):
        (WebCore::HTMLInputElement::setSelectionEnd):
        (WebCore::HTMLInputElement::select):
        * html/HTMLTextAreaElement.cpp:
        (WebCore::rendererAfterUpdateLayout):
        (WebCore::HTMLTextAreaElement::setSelectionStart):
        (WebCore::HTMLTextAreaElement::setSelectionEnd):
        (WebCore::HTMLTextAreaElement::select):
        (WebCore::HTMLTextAreaElement::setSelectionRange):
        (WebCore::HTMLTextAreaElement::updateFocusAppearance):
        * rendering/RenderTextControl.cpp:
        (WebCore::RenderTextControl::setSelectionRange):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@46437 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/dom/text-control-crash-on-select-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/text-control-crash-on-select.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/dom/Document.cpp
WebCore/dom/Document.h
WebCore/dom/InputElement.cpp
WebCore/html/HTMLInputElement.cpp
WebCore/html/HTMLTextAreaElement.cpp
WebCore/rendering/RenderTextControl.cpp

index 562ae6e..f90fb72 100644 (file)
@@ -1,3 +1,15 @@
+2009-07-27  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        https://bugs.webkit.org/show_bug.cgi?id=27474
+        Tests crashes when calling select, setSelectionRange or setting
+        selectionStart/selectionEnd on a textarea/input immediately after
+        setting display:none.
+
+        * fast/dom/text-control-crash-on-select-expected.txt: Added.
+        * fast/dom/text-control-crash-on-select.html: Added.
+
 2009-07-27  Nikolas Zimmermann  <nikolas.zimmermann@torchmobile.com>
 
         Reviewed by George Staikos.
diff --git a/LayoutTests/fast/dom/text-control-crash-on-select-expected.txt b/LayoutTests/fast/dom/text-control-crash-on-select-expected.txt
new file mode 100644 (file)
index 0000000..598f982
--- /dev/null
@@ -0,0 +1 @@
+Tests that we don't crash when killing an text input's or textarea's renderer and then calling select.
diff --git a/LayoutTests/fast/dom/text-control-crash-on-select.html b/LayoutTests/fast/dom/text-control-crash-on-select.html
new file mode 100644 (file)
index 0000000..35199a9
--- /dev/null
@@ -0,0 +1,38 @@
+Tests that we don't crash when killing an text input's or textarea's renderer and then calling select.
+<textarea id="textarea1">textarea</textarea>
+<textarea id="textarea2">textarea</textarea>
+<textarea id="textarea3">textarea</textarea>
+<textarea id="textarea4">textarea</textarea>
+<input id="input1">
+<input id="input2">
+<input id="input3">
+<input id="input4">
+<script>
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+
+    function $(id) {
+        return document.getElementById(id);
+    }
+
+    function testSettingSelection(tagName) {
+        var id = tagName + '1';
+        $(id).style.display = "none";
+        $(id).select();
+
+        id = tagName + '2';
+        $(id).style.display = "none";
+        $(id).setSelectionRange(1, 2);
+
+        id = tagName + '3';
+        $(id).style.display = "none";
+        $(id).selectionStart = 2;
+
+        id = tagName + '4';
+        $(id).style.display = "none";
+        $(id).selectionEnd = 1;
+    }
+    
+    testSettingSelection('textarea');
+    testSettingSelection('input');
+</script>
index df562bc..14851b6 100644 (file)
@@ -1,3 +1,44 @@
+2009-07-27  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        https://bugs.webkit.org/show_bug.cgi?id=27474
+        Fixes crashes due to renderer getting destroyed in updateLayout.
+        We need to call updateLayout before we call into the renderer.
+        Removed the updateLayout call from RenderTextControl and moved it
+        into the calling sites.
+        
+        Also changes updateLayout to updateLayoutIgnorePendingStylesheets so
+        this works with pending stylesheets. Unfortunately, this seems to be
+        untestable. Loading an external stylesheet and then having an inline
+        script hit this code did not result in an pending stylesheets.
+        
+        The are other cases of this bug in the rendering code. I'll file a 
+        followup bug to audit the calls to updateLayout.
+
+        Test: fast/dom/text-control-crash-on-select.html
+
+        * dom/Document.h:
+        (WebCore::Document::inStyleRecalc): Added so the ASSERTs in updateFocusAppearance
+            and setSelectionRange could deal with cases of reentrancy into updateLayout
+            calls. This happens in a couple layout tests.
+        * dom/InputElement.cpp:
+        (WebCore::InputElement::updateSelectionRange):
+        * html/HTMLInputElement.cpp:
+        (WebCore::isTextFieldWithRendererAfterUpdateLayout):
+        (WebCore::HTMLInputElement::setSelectionStart):
+        (WebCore::HTMLInputElement::setSelectionEnd):
+        (WebCore::HTMLInputElement::select):
+        * html/HTMLTextAreaElement.cpp:
+        (WebCore::rendererAfterUpdateLayout):
+        (WebCore::HTMLTextAreaElement::setSelectionStart):
+        (WebCore::HTMLTextAreaElement::setSelectionEnd):
+        (WebCore::HTMLTextAreaElement::select):
+        (WebCore::HTMLTextAreaElement::setSelectionRange):
+        (WebCore::HTMLTextAreaElement::updateFocusAppearance):
+        * rendering/RenderTextControl.cpp:
+        (WebCore::RenderTextControl::setSelectionRange):
+
 2009-07-27  Dimitri Glazkov  <dglazkov@chromium.org>
 
         Reviewed by Dave Levin.
index 4c7d399..228cfde 100644 (file)
@@ -1138,6 +1138,11 @@ void Document::styleRecalcTimerFired(Timer<Document>*)
     updateStyleIfNeeded();
 }
 
+bool Document::childNeedsAndNotInStyleRecalc()
+{
+    return childNeedsStyleRecalc() && !m_inStyleRecalc;
+}
+
 void Document::recalcStyle(StyleChange change)
 {
     // we should not enter style recalc while painting
index 6e1d3c6..6655d9b 100644 (file)
@@ -406,6 +406,7 @@ public:
     PassRefPtr<EditingText> createEditingTextNode(const String&);
 
     virtual void recalcStyle(StyleChange = NoChange);
+    bool childNeedsAndNotInStyleRecalc();
     virtual void updateStyleIfNeeded();
     void updateLayout();
     void updateLayoutIgnorePendingStylesheets();
index 108d17e..b25cd5c 100644 (file)
@@ -116,6 +116,8 @@ void InputElement::updateSelectionRange(InputElement* inputElement, Element* ele
     if (!inputElement->isTextField())
         return;
 
+    element->document()->updateLayoutIgnorePendingStylesheets();
+
     if (RenderTextControl* renderer = toRenderTextControl(element->renderer()))
         renderer->setSelectionRange(start, end);
 }
index 2fc4efd..f35292f 100644 (file)
@@ -552,31 +552,34 @@ int HTMLInputElement::selectionEnd() const
     return toRenderTextControl(renderer())->selectionEnd();
 }
 
+static bool isTextFieldWithRenderer(HTMLInputElement* element)
+{
+    if (!element->isTextField())
+        return false;
+
+    element->document()->updateLayoutIgnorePendingStylesheets();
+    if (!element->renderer())
+        return false;
+
+    return true;
+}
+
 void HTMLInputElement::setSelectionStart(int start)
 {
-    if (!isTextField())
-        return;
-    if (!renderer())
-        return;
-    toRenderTextControl(renderer())->setSelectionStart(start);
+    if (isTextFieldWithRenderer(this))
+        toRenderTextControl(renderer())->setSelectionStart(start);
 }
 
 void HTMLInputElement::setSelectionEnd(int end)
 {
-    if (!isTextField())
-        return;
-    if (!renderer())
-        return;
-    toRenderTextControl(renderer())->setSelectionEnd(end);
+    if (isTextFieldWithRenderer(this))
+        toRenderTextControl(renderer())->setSelectionEnd(end);
 }
 
 void HTMLInputElement::select()
 {
-    if (!isTextField())
-        return;
-    if (!renderer())
-        return;
-    toRenderTextControl(renderer())->select();
+    if (isTextFieldWithRenderer(this))
+        toRenderTextControl(renderer())->select();
 }
 
 void HTMLInputElement::setSelectionRange(int start, int end)
index 8bda72b..6a1dcf8 100644 (file)
@@ -107,32 +107,34 @@ int HTMLTextAreaElement::selectionEnd()
     return toRenderTextControl(renderer())->selectionEnd();
 }
 
+static RenderTextControl* rendererAfterUpdateLayout(HTMLTextAreaElement* element)
+{
+    element->document()->updateLayoutIgnorePendingStylesheets();
+    return toRenderTextControl(element->renderer());
+}
+
 void HTMLTextAreaElement::setSelectionStart(int start)
 {
-    if (!renderer())
-        return;
-    toRenderTextControl(renderer())->setSelectionStart(start);
+    if (RenderTextControl* renderer = rendererAfterUpdateLayout(this))
+        renderer->setSelectionStart(start);
 }
 
 void HTMLTextAreaElement::setSelectionEnd(int end)
 {
-    if (!renderer())
-        return;
-    toRenderTextControl(renderer())->setSelectionEnd(end);
+    if (RenderTextControl* renderer = rendererAfterUpdateLayout(this))
+        renderer->setSelectionEnd(end);
 }
 
 void HTMLTextAreaElement::select()
 {
-    if (!renderer())
-        return;
-    toRenderTextControl(renderer())->select();
+    if (RenderTextControl* renderer = rendererAfterUpdateLayout(this))
+        renderer->select();
 }
 
 void HTMLTextAreaElement::setSelectionRange(int start, int end)
 {
-    if (!renderer())
-        return;
-    toRenderTextControl(renderer())->setSelectionRange(start, end);
+    if (RenderTextControl* renderer = rendererAfterUpdateLayout(this))
+        renderer->setSelectionRange(start, end);
 }
 
 void HTMLTextAreaElement::childrenChanged(bool changedByParser, Node* beforeChange, Node* afterChange, int childCountDelta)
@@ -239,7 +241,8 @@ bool HTMLTextAreaElement::isMouseFocusable() const
 void HTMLTextAreaElement::updateFocusAppearance(bool restorePreviousSelection)
 {
     ASSERT(renderer());
-    
+    ASSERT(!document()->childNeedsAndNotInStyleRecalc());
+
     if (!restorePreviousSelection || m_cachedSelectionStart < 0) {
 #if ENABLE(ON_FIRST_TEXTAREA_FOCUS_SELECT_ALL)
         // Devices with trackballs or d-pads may focus on a textarea in route
index 9acd9b2..95e71dd 100644 (file)
@@ -235,7 +235,7 @@ void RenderTextControl::setSelectionRange(int start, int end)
     end = max(end, 0);
     start = min(max(start, 0), end);
 
-    document()->updateLayout();
+    ASSERT(!document()->childNeedsAndNotInStyleRecalc());
 
     if (style()->visibility() == HIDDEN || !m_innerText || !m_innerText->renderer() || !m_innerText->renderBox()->height()) {
         cacheSelection(start, end);