Changeset 94274 in webkit

Timestamp:

Aug 31, 2011 11:11:07 PM (13 years ago)

Author:

rniwa@webkit.org

Message:

Crash when inserting text with a trailing newline into a textarea via JS
​https://bugs.webkit.org/show_bug.cgi?id=66241

Reviewed by Darin Adler and Kent Tamura.

Source/WebCore:

The crash was caused by updateFromElement biting on the editing code.

When there is a style rule that applies on text nodes inside the shadow DOM, DOM modifications made
by the editing code may trigger style recalculation on input or textarea elements in the midst of editing
commands. In response to this style recalculation, HTMLInputElement::updateFromElement and
HTMLTextAreaElement::updateFromElement call setInnerTextValue to re-create the text nodes in the
shadow DOM. The editing code blows up because setInnerTextValue detaches old text nodes referenced by
Positions and VisiblePositions held by the editing commands in progress.

Fixed the crash by stop calling setInnerTextValue in updateFromElement. Instead, WebKit now creates
the text nodes when attributes, descendent nodes, etc... of input or textarea element changes.

Tests: fast/forms/update-from-element-during-editing-crash-1.html

fast/forms/update-from-element-during-editing-crash-2.html

• html/HTMLInputElement.cpp:

(WebCore::HTMLInputElement::updateType): Force calling setInnerTextValue when input type changes.
(WebCore::HTMLInputElement::updateInnerTextValue): Extracted from RenderTextControlSingleLine's
updateElement.
(WebCore::HTMLInputElement::parseMappedAttribute): Calls updateInnerTextValue; force calling
setInnerTextValue when -webkit-speech attribute changes. In the theory, we should be able to call it less
frequently but there are too many cases to consider at the moment.
(WebCore::HTMLInputElement::setValue): Calls updateInnerTextValue when the value actually changed.
Note we need to call it before we set or restore selection.

• html/HTMLInputElement.h:
• html/HTMLTextAreaElement.cpp:

(WebCore::HTMLTextAreaElement::childrenChanged): Calls updateInnerTextValue when textarea's descendants
nodes are changed by parser or scripts.
(WebCore::HTMLTextAreaElement::setValueCommon): Calls updateInnerTextValue when the value changes.

• html/HTMLTextFormControlElement.h:
• html/NumberInputType.cpp:

(WebCore::NumberInputType::willBlur): Calls updateInnerTextValue because input[type=number] forces
the value to be valid on blur.

• rendering/RenderTextControlMultiLine.cpp: Removed RenderTextControlMultiLine::updateFromElement.
• rendering/RenderTextControlMultiLine.h: Ditto.
• rendering/RenderTextControlSingleLine.cpp:

(WebCore::RenderTextControlSingleLine::updateFromElement):

LayoutTests:

Add regressions tests to ensure WebKit doesn't crash even when style recalc happens
in the midst of editing commands.

• fast/forms/update-from-element-during-editing-crash-1-expected.txt: Added.
• fast/forms/update-from-element-during-editing-crash-1.html: Added.
• fast/forms/update-from-element-during-editing-crash-2-expected.txt: Added.
• fast/forms/update-from-element-during-editing-crash-2.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/forms/update-from-element-during-editing-crash-1-expected.txt (added)
• LayoutTests/fast/forms/update-from-element-during-editing-crash-1.html (added)
• LayoutTests/fast/forms/update-from-element-during-editing-crash-2-expected.txt (added)
• LayoutTests/fast/forms/update-from-element-during-editing-crash-2.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/HTMLInputElement.cpp (modified) (7 diffs)
• Source/WebCore/html/HTMLInputElement.h (modified) (1 diff)
• Source/WebCore/html/HTMLTextAreaElement.cpp (modified) (3 diffs)
• Source/WebCore/html/HTMLTextFormControlElement.h (modified) (1 diff)
• Source/WebCore/html/NumberInputType.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderTextControlMultiLine.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderTextControlMultiLine.h (modified) (1 diff)
• Source/WebCore/rendering/RenderTextControlSingleLine.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2011-08-31  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Crash when inserting text with a trailing newline into a textarea via JS
+        https://bugs.webkit.org/show_bug.cgi?id=66241
+
+        Reviewed by Darin Adler and Kent Tamura.
+
+        Add regressions tests to ensure WebKit doesn't crash even when style recalc happens
+        in the midst of editing commands.
+
+        * fast/forms/update-from-element-during-editing-crash-1-expected.txt: Added.
+        * fast/forms/update-from-element-during-editing-crash-1.html: Added.
+        * fast/forms/update-from-element-during-editing-crash-2-expected.txt: Added.
+        * fast/forms/update-from-element-during-editing-crash-2.html: Added.
+
 2011-08-31  David Levin  <levin@chromium.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-08-31  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Crash when inserting text with a trailing newline into a textarea via JS
+        https://bugs.webkit.org/show_bug.cgi?id=66241
+
+        Reviewed by Darin Adler and Kent Tamura.
+
+        The crash was caused by updateFromElement biting on the editing code.
+
+        When there is a style rule that applies on text nodes inside the shadow DOM, DOM modifications made
+        by the editing code may trigger style recalculation on input or textarea elements in the midst of editing
+        commands. In response to this style recalculation, HTMLInputElement::updateFromElement and
+        HTMLTextAreaElement::updateFromElement call setInnerTextValue to re-create the text nodes in the
+        shadow DOM. The editing code blows up because setInnerTextValue detaches old text nodes referenced by
+        Positions and VisiblePositions held by the editing commands in progress.
+
+        Fixed the crash by stop calling setInnerTextValue in updateFromElement. Instead, WebKit now creates
+        the text nodes when attributes, descendent nodes, etc... of input or textarea element changes.
+
+        Tests: fast/forms/update-from-element-during-editing-crash-1.html
+               fast/forms/update-from-element-during-editing-crash-2.html
+
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::updateType): Force calling setInnerTextValue when input type changes.
+        (WebCore::HTMLInputElement::updateInnerTextValue): Extracted from RenderTextControlSingleLine's
+        updateElement.
+        (WebCore::HTMLInputElement::parseMappedAttribute): Calls updateInnerTextValue; force calling
+        setInnerTextValue when -webkit-speech attribute changes. In the theory, we should be able to call it less
+        frequently but there are too many cases to consider at the moment.
+        (WebCore::HTMLInputElement::setValue): Calls updateInnerTextValue when the value actually changed.
+        Note we need to call it before we set or restore selection.
+        * html/HTMLInputElement.h:
+        * html/HTMLTextAreaElement.cpp:
+        (WebCore::HTMLTextAreaElement::childrenChanged): Calls updateInnerTextValue when textarea's descendants
+        nodes are changed by parser or scripts.
+        (WebCore::HTMLTextAreaElement::setValueCommon): Calls updateInnerTextValue when the value changes.
+        * html/HTMLTextFormControlElement.h:
+        * html/NumberInputType.cpp:
+        (WebCore::NumberInputType::willBlur): Calls updateInnerTextValue because input[type=number] forces
+        the value to be valid on blur.
+        * rendering/RenderTextControlMultiLine.cpp: Removed RenderTextControlMultiLine::updateFromElement.
+        * rendering/RenderTextControlMultiLine.h: Ditto.
+        * rendering/RenderTextControlSingleLine.cpp:
+        (WebCore::RenderTextControlSingleLine::updateFromElement):
+
 2011-08-31  Tom Zakrajsek  <tomz@codeaurora.org>
 

trunk/Source/WebCore/html/HTMLInputElement.cpp

@@ -50 +50 @@
 #include "RenderTextControlSingleLine.h"
 #include "RenderTheme.h"
-#include "RuntimeEnabledFeatures.h"
 #include "SearchInputType.h"
 #include "ScriptEventListener.h"
@@ -60 +59 @@
 #include "ColorChooser.h"
 #include "ColorInputType.h"
+#endif
+
+#if ENABLE(INPUT_SPEECH)
+#include "RuntimeEnabledFeatures.h"
 #endif
 
@@ -584 +587 @@
     else
         updateValueIfNeeded();
+
+    setFormControlValueMatchesRenderer(false);
+    updateInnerTextValue();
+
     m_wasModifiedByUser = false;
 
@@ -614 +621 @@
     setNeedsValidityCheck();
     notifyFormStateChanged();
+}
+
+void HTMLInputElement::updateInnerTextValue()
+{
+    if (!isTextField())
+        return;
+
+    if (!suggestedValue().isNull())
+        setInnerTextValue(suggestedValue());
+    else if (!formControlValueMatchesRenderer()) {
+        // Update the renderer value if the formControlValueMatchesRenderer() flag is false.
+        // It protects an unacceptable renderer value from being overwritten with the DOM value.
+        setInnerTextValue(visibleValue());
+    }
 }
 
@@ -823 +844 @@
             m_inputType->createShadowSubtree();
         }
+        setFormControlValueMatchesRenderer(false);
         setNeedsStyleRecalc();
     } else if (attr->name() == onwebkitspeechchangeAttr)
@@ -829 +851 @@
     else
         HTMLTextFormControlElement::parseMappedAttribute(attr);
+    updateInnerTextValue();
 }
 
@@ -1077 +1100 @@
 
     setNeedsValidityCheck();
+
+    if (valueChanged)
+        updateInnerTextValue();
 
     if (isTextField()) {

trunk/Source/WebCore/html/HTMLInputElement.h

@@ -146 +146 @@
     String sanitizeValue(const String&) const;
 
+    void updateInnerTextValue();
+
     // The value which is drawn by a renderer.
     String visibleValue() const;

trunk/Source/WebCore/html/HTMLTextAreaElement.cpp

@@ -37 +37 @@
 #include "Frame.h"
 #include "HTMLNames.h"
-#include "RenderStyle.h"
 #include "RenderTextControlMultiLine.h"
 #include "ShadowRoot.h"
@@ -102 +101 @@
     if (!m_isDirty)
         setNonDirtyValue(defaultValue());
+    setInnerTextValue(value());
     HTMLElement::childrenChanged(changedByParser, beforeChange, afterChange, childCountDelta);
 }
@@ -330 +330 @@
 
     m_value = normalizedValue;
+    setInnerTextValue(m_value);
     setLastChangeWasNotUserEdit();
     updatePlaceholderVisibility(false);

trunk/Source/WebCore/html/HTMLTextFormControlElement.h

@@ -104 +104 @@
 
     String valueWithHardLineBreaks() const;
+
 private:
     int computeSelectionStart() const;

trunk/Source/WebCore/html/NumberInputType.cpp

@@ -281 +281 @@
     // We need to reset the renderer value explicitly because an unacceptable
     // renderer value should be purged before style calculation.
-    if (element()->renderer())
-        element()->renderer()->updateFromElement();
+    element()->updateInnerTextValue();
 }
 

trunk/Source/WebCore/rendering/RenderTextControlMultiLine.cpp

@@ -81 +81 @@
 }
 
-void RenderTextControlMultiLine::updateFromElement()
-{
-    RenderTextControl::updateFromElement();
-
-    textFormControlElement()->setInnerTextValue(static_cast<HTMLTextAreaElement*>(node())->value());
-}
-
 PassRefPtr<RenderStyle> RenderTextControlMultiLine::createInnerTextStyle(const RenderStyle* startStyle) const
 {

trunk/Source/WebCore/rendering/RenderTextControlMultiLine.h

@@ -42 +42 @@
     virtual LayoutUnit baselinePosition(FontBaseline, bool firstLine, LineDirectionMode, LinePositionMode = PositionOnContainingLine) const;
 
-    virtual void updateFromElement();
-
     virtual RenderStyle* textBaseStyle() const;
     virtual PassRefPtr<RenderStyle> createInnerTextStyle(const RenderStyle* startStyle) const;

trunk/Source/WebCore/rendering/RenderTextControlSingleLine.cpp

@@ -477 +477 @@
         updateCancelButtonVisibility();
 
-    if (!inputElement()->suggestedValue().isNull())
-        textFormControlElement()->setInnerTextValue(inputElement()->suggestedValue());
-    else {
-        if (node()->hasTagName(inputTag)) {
-            // For HTMLInputElement, update the renderer value if the formControlValueMatchesRenderer()
-            // flag is false. It protects an unacceptable renderer value from
-            // being overwritten with the DOM value.
-            if (!inputElement()->formControlValueMatchesRenderer())
-                textFormControlElement()->setInnerTextValue(inputElement()->visibleValue());
-        }
-    }
-
     if (m_searchPopupIsVisible)
         m_searchPopup->popupMenu()->updateFromElement();