Bug 66241

Summary: Crash when inserting text with a trailing newline into a textarea via JS
Product: WebKit Reporter: Shajith <demerzel>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Major CC: darin, dglazkov, dominicc, enrica, hyatt, jamesr, jonlee, keishi, leviw, mitz, rniwa, sam, shinyak, sullivan, tkent, wcarss, webkit.review.bot
Priority: P1 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
Bug Depends on: 67141, 67152, 67320    
Bug Blocks: 67360, 67361    
Attachments:
Description Flags
Test markup to reproduce the crash
none
Crash file
none
demo
none
Cleaned-up repro
none
work in progress
none
work in progress 2
none
fixes the bug darin: review+, webkit.review.bot: commit-queue-

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>