Bug 66241 - Crash when inserting text with a trailing newline into a textarea via JS
Summary: Crash when inserting text with a trailing newline into a textarea via JS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P1 Major
Assignee: Ryosuke Niwa
URL:
Keywords: HasReduction
Depends on: 67141 67152 67320
Blocks: 67360 67361
  Show dependency treegraph
 
Reported: 2011-08-15 11:16 PDT by Shajith
Modified: 2011-08-31 23:10 PDT (History)
17 users (show)

See Also:


Attachments
Test markup to reproduce the crash (211 bytes, text/html)
2011-08-15 11:16 PDT, Shajith
no flags Details
Crash file (33.54 KB, text/plain)
2011-08-15 11:18 PDT, Shajith
no flags Details
demo (377 bytes, text/html)
2011-08-15 17:51 PDT, Ryosuke Niwa
no flags Details
Cleaned-up repro (56 bytes, text/html)
2011-08-29 10:51 PDT, Dimitri Glazkov (Google)
no flags Details
work in progress (4.55 KB, patch)
2011-08-30 23:07 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress 2 (20.22 KB, patch)
2011-08-31 13:50 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixes the bug (14.28 KB, patch)
2011-08-31 18:38 PDT, Ryosuke Niwa
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shajith 2011-08-15 11:16:19 PDT
Steps to reproduce:


1. Open an HTML file that contains the following (also attached as foo.html)
<html>
<body onload="document.getElementById('comment_value').innerHTML='a\n';">
<style>
#container + * { clear: both; }
</style>
<p id="container">
<textarea id="comment_value"></textarea>
</p>
</body>
</html>


2. Click into the textarea, move cursor to the end of the 'a' in it.

3. Hit the Return key.

What is the expected result?

Cursor should go to the next line, inserting a new line into the text area.

What happens instead?

The page reloads, and I see a new crash report in Console.app. Attached.

Notes:
- Removing the "#container + *" CSS rule prevents the crash.
- I saw this crash first in the latest Chrome, and reported it on their bugtracker, here: 
   http://code.google.com/p/chromium/issues/detail?id=92757
Comment 1 Shajith 2011-08-15 11:16:54 PDT
Created attachment 103930 [details]
Test markup to reproduce the crash
Comment 2 Shajith 2011-08-15 11:18:06 PDT
Created attachment 103931 [details]
Crash file
Comment 3 Ryosuke Niwa 2011-08-15 17:51:32 PDT
The problem is that FrameSelection::textWillBeReplaced is triggering layout and renewing the shadow DOM when we split the text node in line 141 of InsertLineBreakCommand.cpp::doApply in InsertLineBreakCommand.cpp:
    } else if (pos.deprecatedNode()->isTextNode()) {
        // Split a text node
        Text* textNode = static_cast<Text*>(pos.deprecatedNode());
        splitTextNode(textNode, pos.deprecatedEditingOffset());
        insertNodeBefore(nodeToInsert, textNode);
        Position endingPosition = firstPositionInNode(textNode);

stack trace:
#0	0x102ab3a2a in WebCore::CharacterData::setDataAndUpdate at CharacterData.cpp:177
#1	0x102ab3c90 in WebCore::CharacterData::deleteData at CharacterData.cpp:130
#2	0x103739103 in WebCore::SplitTextNodeCommand::insertText1AndTrimText2 at SplitTextNodeCommand.cpp:104
#3	0x103739502 in WebCore::SplitTextNodeCommand::doApply at SplitTextNodeCommand.cpp:66
#4	0x102d82967 in WebCore::EditCommand::apply at EditCommand.cpp:92
#5	0x102add980 in WebCore::CompositeEditCommand::applyCommandToComposite at CompositeEditCommand.cpp:102
#6	0x102adf83d in WebCore::CompositeEditCommand::splitTextNode at CompositeEditCommand.cpp:279
#7	0x102fc899f in WebCore::InsertLineBreakCommand::doApply at InsertLineBreakCommand.cpp:141
#8	0x102d82967 in WebCore::EditCommand::apply at EditCommand.cpp:92

This is a quite serious design flaw.  On one hand, we have to update selection when CharacterData is modified but on the other hand, we can't lose the shadow DOM while we're in the middle of modifying DOM nodes in editing.
Comment 4 Ryosuke Niwa 2011-08-15 17:51:53 PDT
Created attachment 103987 [details]
demo
Comment 5 Kent Tamura 2011-08-24 01:32:26 PDT
Why is the textNode detached by m_frame->document()->updateLayout()?

Why does "#container + * { clear: both; }" affect? It looks to be unrelated to <textarea>.
Comment 6 Ryosuke Niwa 2011-08-24 19:25:50 PDT
(In reply to comment #5)
> Why is the textNode detached by m_frame->document()->updateLayout()?

It's causing the shadow DOM tree to be re-created.  The text node is in the shadow tree.
Comment 7 Kent Tamura 2011-08-24 21:20:23 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Why is the textNode detached by m_frame->document()->updateLayout()?
> 
> It's causing the shadow DOM tree to be re-created.  The text node is in the shadow tree.

Do you mean HTMLTextElement::innerTextElement() is re-created? HTMLTextAreaElement and RenderTextControl* doesn't recreate it, so the editing code does it?
Comment 8 Ryosuke Niwa 2011-08-24 21:27:33 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > It's causing the shadow DOM tree to be re-created.  The text node is in the shadow tree.
> 
> Do you mean HTMLTextElement::innerTextElement() is re-created?

Yes.
Comment 9 Kent Tamura 2011-08-25 23:46:25 PDT
ok, I understand.

> FrameSelection::textWillBeReplaced is triggering layout and renewing the shadow DOM

  m_frame->document()->updateLayout() in FrameSelection::textWillBeReplaced()
  ...
  HTMLFormControlElement::recalcStyle()
  ...
  updateFromElementCallback() in HTMLFormControlElement.cpp
  RenderTextControlMultiLine::updateFromElement()
  setInnerTextValeu(...) resets the content of innerTextElement().
    (Note that innerTextElement() itself is not re-created.)
Comment 10 Ryosuke Niwa 2011-08-28 09:44:54 PDT
Apparently, this is the top renderer crasher in Chromium.

The problem is that the innerTextElement()'s children are re-created via RenderTextControlMultiLine::updateFromElement when the text node is split around line 140 in InsertLineBreakCommand::doApply:

        Text* textNode = static_cast<Text*>(pos.deprecatedNode());
        splitTextNode(textNode, pos.deprecatedEditingOffset());
        insertNodeBefore(nodeToInsert, textNode);

because it triggers layout.

I don't think we should be marking the shadow host dirty when the text nodes in the shadow DOM changes but I'm not an expert in style recalculation and style resolution.
Comment 11 Dimitri Glazkov (Google) 2011-08-28 21:31:44 PDT
(In reply to comment #10)

>   m_frame->document()->updateLayout() in FrameSelection::textWillBeReplaced()
>   ...
>   HTMLFormControlElement::recalcStyle()
>   ...
>   updateFromElementCallback() in HTMLFormControlElement.cpp
>   RenderTextControlMultiLine::updateFromElement()
>   setInnerTextValeu(...) resets the content of innerTextElement().

This is the line that bugs me. Why is it even here?
Comment 12 Dimitri Glazkov (Google) 2011-08-29 10:51:31 PDT
Created attachment 105501 [details]
Cleaned-up repro

Here's a slightly cleaned-up repro.
Comment 13 Darin Adler 2011-08-29 10:54:42 PDT
(In reply to comment #11)
> (In reply to comment #10)
> 
> >   setInnerTextValeu(...) resets the content of innerTextElement().
> 
> This is the line that bugs me. Why is it even here?

Are you asking why it resets the content when the text is already correct? Or why it would ever reset the content of the inner text element?
Comment 14 Ryosuke Niwa 2011-08-30 23:07:55 PDT
Created attachment 105744 [details]
work in progress
Comment 15 Ryosuke Niwa 2011-08-30 23:09:20 PDT
Comment on attachment 105744 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=105744&action=review

> Source/WebCore/html/HTMLTextFormControlElement.cpp:465
> +static String textInShadowTree(HTMLElement* innerText)

This function is basically RenderText::text(). I should probably move it from RenderTextControl to HTMLTextFormControlElement because there's no reason it should live in RenderTextControl anymore.
Comment 16 Kent Tamura 2011-08-31 00:17:25 PDT
Comment on attachment 105744 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=105744&action=review

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:465

> 
> This function is basically RenderText::text(). I should probably move it from RenderTextControl to HTMLTextFormControlElement because there's no reason it should live in RenderTextControl anymore.

I agree moving RenderTextControl::text().

BTW, the name "textInShadowTree()" sounds implementation-dependent.  I prefer "textInEditableElement()", "editingValue()", or something.
Comment 17 Ryosuke Niwa 2011-08-31 13:50:06 PDT
Created attachment 105825 [details]
work in progress 2

The following tests fail with this patch.
  fast/forms/search-disabled-readonly.html
  fast/forms/search-rtl.html
  fast/forms/text-control-intrinsic-widths.html
  fast/forms/textarea-submit-crash.html

Will investigate further but I can probably proceed with RenderText::text move.
Comment 18 Ryosuke Niwa 2011-08-31 18:38:23 PDT
Created attachment 105878 [details]
fixes the bug
Comment 19 Kent Tamura 2011-08-31 18:57:32 PDT
Comment on attachment 105878 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=105878&action=review

I'll set r+ when cr-linux EWS becomes green.

> Source/WebCore/html/HTMLInputElement.cpp:627
> +    if (!isTextField() && !isPasswordField())

Type=password is also a text field.  So "if (!isTextField())" is enough.

nit: Such type-specific operation should be in specific InputType.
 1. Add an empty InputType::updateInnerTextValue()
 2. Add TextFieldInputType::updateInnerTextValue()
 3. Change the call sites of updateInnerTextValue()  to m_inputType->updateInnerTextValue().

> Source/WebCore/html/HTMLInputElement.cpp:633
> +        // For HTMLInputElement, update the renderer value if the formControlValueMatchesRenderer()

"For HTMLInputElement, " is not needed.
Comment 20 Ryosuke Niwa 2011-08-31 19:42:18 PDT
Comment on attachment 105878 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=105878&action=review

>> Source/WebCore/html/HTMLInputElement.cpp:627
>> +    if (!isTextField() && !isPasswordField())
> 
> Type=password is also a text field.  So "if (!isTextField())" is enough.
> 
> nit: Such type-specific operation should be in specific InputType.
>  1. Add an empty InputType::updateInnerTextValue()
>  2. Add TextFieldInputType::updateInnerTextValue()
>  3. Change the call sites of updateInnerTextValue()  to m_inputType->updateInnerTextValue().

Oh oops, forgot to remove isPasswordField().  Will do.

>> Source/WebCore/html/HTMLInputElement.cpp:633
>> +        // For HTMLInputElement, update the renderer value if the formControlValueMatchesRenderer()
> 
> "For HTMLInputElement, " is not needed.

Will fix.
Comment 21 WebKit Review Bot 2011-08-31 21:26:55 PDT
Comment on attachment 105878 [details]
fixes the bug

Attachment 105878 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9582054

New failing tests:
fast/speech/input-appearance-searchandspeech.html
fast/speech/input-appearance-numberandspeech.html
fast/speech/input-appearance-speechbutton.html
Comment 22 Darin Adler 2011-08-31 22:01:01 PDT
Comment on attachment 105878 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=105878&action=review

>>> Source/WebCore/html/HTMLInputElement.cpp:627
>>> +    if (!isTextField() && !isPasswordField())
>> 
>> Type=password is also a text field.  So "if (!isTextField())" is enough.
>> 
>> nit: Such type-specific operation should be in specific InputType.
>>  1. Add an empty InputType::updateInnerTextValue()
>>  2. Add TextFieldInputType::updateInnerTextValue()
>>  3. Change the call sites of updateInnerTextValue()  to m_inputType->updateInnerTextValue().
> 
> Oh oops, forgot to remove isPasswordField().  Will do.

Putting the type-specific operation on the specific InputType the way Kent is telling you to is better. The isXXX functions are deprecated within the class.

> Source/WebCore/html/HTMLInputElement.cpp:1105
>      if (isTextField()) {
> +        if (valueChanged)
> +            updateInnerTextValue();

Since updateInnerTextValue already has the isTextField check, it might be clearer to leave the call outside the if statement. Because later we’d probably like to refactor the code within the isTextField block into an InputType function.

> Source/WebCore/html/HTMLInputElement.h:148
> +    void updateInnerTextValue();

A shame this has to public just so it can be called by one of the InputType functions.

> Source/WebCore/html/HTMLTextFormControlElement.h:107
>      String valueWithHardLineBreaks() const;
> +
>  private:

Nice to add this blank line--it’s the preferred style--but not sure this should be in this patch.
Comment 23 Ryosuke Niwa 2011-08-31 22:55:11 PDT
Thanks for the review, tkent & darin!

(In reply to comment #21)
> (From update of attachment 105878 [details])
> Attachment 105878 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/9582054
> 
> New failing tests:
> fast/speech/input-appearance-searchandspeech.html
> fast/speech/input-appearance-numberandspeech.html
> fast/speech/input-appearance-speechbutton.html

I figured out why they're failing.  I need to call setFormControlValueMatchesRenderer(false) when -webkit-speech changes because we always recreate the shadow DOM when that attribute changes.

(In reply to comment #22)
> (From update of attachment 105878 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105878&action=review
> 
> >>> Source/WebCore/html/HTMLInputElement.cpp:627
> >>> +    if (!isTextField() && !isPasswordField())
> >> 
> >> Type=password is also a text field.  So "if (!isTextField())" is enough.
> >> 
> >> nit: Such type-specific operation should be in specific InputType.
> >>  1. Add an empty InputType::updateInnerTextValue()
> >>  2. Add TextFieldInputType::updateInnerTextValue()
> >>  3. Change the call sites of updateInnerTextValue()  to m_inputType->updateInnerTextValue().
> > 
> > Oh oops, forgot to remove isPasswordField().  Will do.
> 
> Putting the type-specific operation on the specific InputType the way Kent is telling you to is better. The isXXX functions are deprecated within the class.

Yeah, I'm going to do that in a separate patch. Since I have to create a patch for chromium/835 branch, and I'd really like to minimize the code changes in this patch.

> > Source/WebCore/html/HTMLInputElement.cpp:1105
> >      if (isTextField()) {
> > +        if (valueChanged)
> > +            updateInnerTextValue();
> 
> Since updateInnerTextValue already has the isTextField check, it might be clearer to leave the call outside the if statement. Because later we’d probably like to refactor the code within the isTextField block into an InputType function.

Good point.  Will fix.

> > Source/WebCore/html/HTMLInputElement.h:148
> > +    void updateInnerTextValue();
> 
> A shame this has to public just so it can be called by one of the InputType functions.

Yeah, it's very unfortunate.
Comment 24 Ryosuke Niwa 2011-08-31 23:10:05 PDT
Committed r94274: <http://trac.webkit.org/changeset/94274>