WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
66241
Crash when inserting text with a trailing newline into a textarea via JS
https://bugs.webkit.org/show_bug.cgi?id=66241
Summary
Crash when inserting text with a trailing newline into a textarea via JS
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.
Top of Page
Format For Printing
XML
Clone This Bug