Bug 66241

Summary:

Crash when inserting text with a trailing newline into a textarea via JS

Product:

WebKit

Reporter:

Shajith <demerzel>

Component:

HTML Editing

Assignee:

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:

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-

Shajith

Reported 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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Shajith

Comment 1 2011-08-15 11:16:54 PDT

Created attachment 103930 [details] Test markup to reproduce the crash

Shajith

Comment 2 2011-08-15 11:18:06 PDT

Created attachment 103931 [details] Crash file

Ryosuke Niwa

Comment 3 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.

Ryosuke Niwa

Comment 4 2011-08-15 17:51:53 PDT

Created attachment 103987 [details] demo

Kent Tamura

Comment 5 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>.

Ryosuke Niwa

Comment 6 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.

Kent Tamura

Comment 7 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?

Ryosuke Niwa

Comment 8 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.

Kent Tamura

Comment 9 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.)

Ryosuke Niwa

Comment 10 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.

Dimitri Glazkov (Google)

Comment 11 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?

Dimitri Glazkov (Google)

Comment 12 2011-08-29 10:51:31 PDT

Created attachment 105501 [details] Cleaned-up repro Here's a slightly cleaned-up repro.

Darin Adler

Comment 13 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?

Ryosuke Niwa

Comment 14 2011-08-30 23:07:55 PDT

Created attachment 105744 [details] work in progress

Ryosuke Niwa

Comment 15 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.

Kent Tamura

Comment 16 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.

Ryosuke Niwa

Comment 17 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.

Ryosuke Niwa

Comment 18 2011-08-31 18:38:23 PDT

Created attachment 105878 [details] fixes the bug

Kent Tamura

Comment 19 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.

Ryosuke Niwa

Comment 20 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.

WebKit Review Bot

Comment 21 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

Darin Adler

Comment 22 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.

Ryosuke Niwa

Comment 23 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.

Ryosuke Niwa

Comment 24 2011-08-31 23:10:05 PDT

Committed r94274: <http://trac.webkit.org/changeset/94274>

Note You need to log in before you can comment on or make changes to this bug.