Bug 39989 - Applying inline style to a text node whose parent is an inline editable root causes crash
Summary: Applying inline style to a text node whose parent is an inline editable root ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Nobody
URL: http://veitlehmann.de/webkit-crash.html
Keywords: HasReduction
: 37197 (view as bug list)
Depends on: 42793 42937
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-01 04:22 PDT by Veit Lehmann
Modified: 2010-07-27 11:38 PDT (History)
10 users (show)

See Also:


Attachments
Very simple HTML5 test case for this bug (247 bytes, text/html)
2010-07-06 23:48 PDT, Patrick Quinn-Graham
no flags Details
Crash log (33.30 KB, text/plain)
2010-07-06 23:52 PDT, Patrick Quinn-Graham
no flags Details
cleans up splitText(Element)At(Start|End)IfNeed and fixes the bug (9.03 KB, patch)
2010-07-24 12:35 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
complete patch since 42793 has been resolved (19.60 KB, patch)
2010-07-24 16:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
simple fix with large test (18.09 KB, patch)
2010-07-26 11:18 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Veit Lehmann 2010-06-01 04:22:25 PDT
Elements styled as inline, inline-block or inline-table with contenteditable=true crash WebKit browsers when RichText is applied.

To reproduce the bug:
- Go to http://veitlehmann.de/webkit-crash.html
- OR: try this snippet: <span contenteditable="true">select text, press ctrl+b</span>
- Select some text in the contenteditable areas and press ctrl+b or ctrl+i

If the element ist styled as inline* and doesn't float, the browser will crash.

This happens regardless of the element type - the styling counts.
It also happens with insite text editors like http://www.nicedit.com applied on such elements.

Contenteditable on inline-styled elements with RichText is useful for CMS with insite editing.
Comment 1 Tobias Struckmeier 2010-06-01 08:04:15 PDT
I was able to reproduce the bug on two machines as well (chrome 5.0.375.55)
Comment 2 Tobias Struckmeier 2010-06-01 08:05:19 PDT
(In reply to comment #1)
> I was able to reproduce the bug on two machines as well (chrome 5.0.375.55)

One is a fresh and clean Windows XP installation. With no extra software which hooks into the browser.
Comment 3 Alexey Proskuryakov 2010-06-02 11:30:31 PDT
Confirmed with r60522.

#0	0x01bf7635 in WebCore::Node::getFlag at Node.h:651
#1	0x01ad5e41 in WebCore::Node::isContainerNode at Node.h:185
#2	0x01c97341 in WebCore::Node::lastChild at Node.h:144
#3	0x01b3b8e8 in WebCore::ApplyStyleCommand::splitTextElementAtEndIfNeeded at ApplyStyleCommand.cpp:1580
#4	0x01b41e9f in WebCore::ApplyStyleCommand::applyInlineStyle at ApplyStyleCommand.cpp:918
#5	0x01b43c1c in WebCore::ApplyStyleCommand::doApply at ApplyStyleCommand.cpp:562
#6	0x01dcbb99 in WebCore::EditCommand::apply at EditCommand.cpp:91
#7	0x01dcbc25 in WebCore::applyCommand at EditCommand.cpp:212
#8	0x01dd2082 in WebCore::Editor::applyStyle at Editor.cpp:729
#9	0x01dd5bf2 in WebCore::Editor::applyStyleToSelection at Editor.cpp:759
#10	0x01ddc19f in WebCore::applyCommandToFrame at EditorCommand.cpp:102
#11	0x01ddc3bf in WebCore::executeToggleStyle at EditorCommand.cpp:175
#12	0x01ddc463 in WebCore::executeToggleBold at EditorCommand.cpp:1005

See also: bug 37197.
Comment 4 Alexey Proskuryakov 2010-06-02 11:32:19 PDT
This also happens with Safari/WebKit 4.0.5, so it's not a recent regression.
Comment 5 Kalle Vahlman 2010-06-06 05:51:27 PDT
*** Bug 37197 has been marked as a duplicate of this bug. ***
Comment 6 Veit Lehmann 2010-06-08 12:02:22 PDT
I have tested this in some other WebKit-based browsers.

These browsers don't crash:
- Arora 0.10.0 (current)
- Old Chrome 1.0.154.53 and 3.0.197.0
- Old Safari 3.2.1 (525.27.1)

The new Safari 5 still crashes on these elements. I think Chrome crashes starting from version 4.x.
Comment 7 Patrick Quinn-Graham 2010-07-06 23:48:36 PDT
Created attachment 60692 [details]
Very simple HTML5 test case for this bug

I'm hitting this bug in Safari 5, Chromium 6.0.459.0 (51684) (nightly downloaded today) and WebKit r62241.

Can hit it two ways: using document.execCommand or by using the cmd+b keyboard shortcut in Safari (Chromium doesn't seem to support that).
Comment 8 Patrick Quinn-Graham 2010-07-06 23:52:44 PDT
Created attachment 60693 [details]
Crash log

Example crash log from WebKit when this bug is triggered
Comment 9 Ryosuke Niwa 2010-07-24 00:52:18 PDT
This is crash is caused when splitTextElementAtEndIfNeeded is called on a text node with non-editable parent.  As in https://bugs.webkit.org/show_bug.cgi?id=27156, we should carefully verify the pre-condition to call splitTextElementAtEndIfNeeded.  The bug does not reproduce when the entire text node is selected.
Comment 10 Ryosuke Niwa 2010-07-24 12:35:59 PDT
Created attachment 62507 [details]
cleans up splitText(Element)At(Start|End)IfNeed and fixes the bug

Work in progress.  This patch moves "IfNeeded" check from splitText(Element)AtEndIfNeed and isolate as isValidCaretPositionInTextNode, thanks to Eric Seidel's patch for the bug 25056.  I then made splitTextElementAt(Start|End) smarter by adding a check to see if the parent of the element we're about to split is editable (if the element itself was the root editable element, then we can't split).  If the check fails, we only split the text node, which is always possible.

This patch can be split into cleanup and actual fix.  It also requires rebaseline of editing/execCommand/hilitecolor.html and editing/style/remove-underline-from-stylesheet.html but we must wait for my patch for the bug 42793 to be landed because hilitecolor.html is a currently render test.
Comment 11 Ryosuke Niwa 2010-07-24 16:38:34 PDT
Created attachment 62511 [details]
complete patch since 42793 has been resolved

This patch definitely needs to be broken down into two pieces.  I'd ask for your early feedbacks first.
Comment 12 Ryosuke Niwa 2010-07-26 11:18:16 PDT
Created attachment 62586 [details]
simple fix with large test

Ready for review.  Since I wanted to test throughly with variety of inline styles, I added 12KB test but the change in code is simple and small so should be easy to review.
Comment 13 Darin Adler 2010-07-26 13:55:11 PDT
Comment on attachment 62586 [details]
simple fix with large test

> +    Node* parent = start.node()->parentNode();
> +    if (parent && parent->parentElement() && parent->parentElement()->isContentEditable()) {
> +        int endOffsetAdjustment = start.node() == end.node() ? start.deprecatedEditingOffset() : 0;
> +        Text* text = static_cast<Text*>(start.node());
> +        splitTextNodeContainingElement(text, start.deprecatedEditingOffset());
> +        updateStartEnd(Position(start.node()->parentNode(), start.node()->nodeIndex()), Position(end.node(), end.deprecatedEditingOffset() - endOffsetAdjustment));
> +    } else {
> +        splitTextAtStart(start, end);
> +    }

In WebKit we often take the "early exit" approach, checking for an unusual case and handling that case, then using a return. The non-unusual case then continues without be indented inside an if statement. Doing that here would have two benefits besides the usual ones:

    1) The diff would be smaller because we wouldn't be re-indenting all the code.
    2) The body of the if statement would be two lines so you could use braces around it, unlike the one line else body.

It’s not WebKit style to have braces around a 1-line else body, but such things are pretty ugly without the braces, so I think it would be nice to avoid it.

Patch otherwise looks fine, but I think it would be better with early exit.
Comment 14 Ryosuke Niwa 2010-07-26 14:38:27 PDT
(In reply to comment #13)
> Patch otherwise looks fine, but I think it would be better with early exit.

oops, I don't know why I didn't think of doing that here.  Now my changes are 4 lines per function.  I'll commit the early exit version as soon as I updated my local checkout.

Thanks for the review!
Comment 15 Ryosuke Niwa 2010-07-26 15:49:24 PDT
Landed as http://trac.webkit.org/changeset/64083.
Comment 16 Veit Lehmann 2010-07-26 16:27:03 PDT
Cool, you rock! I will test the next nightly on Mac an Windows and give you feedback.
Comment 17 Veit Lehmann 2010-07-27 07:08:22 PDT
I just tested WebKit r64084 (Mac) and r64088 (Win) with my test case (http://veitlehmann.de/webkit-crash.html). It works, doesn't crash anymore. Thanks a lot, guys!
Comment 18 Ryosuke Niwa 2010-07-27 11:38:32 PDT
(In reply to comment #17)
> I just tested WebKit r64084 (Mac) and r64088 (Win) with my test case (http://veitlehmann.de/webkit-crash.html). It works, doesn't crash anymore. Thanks a lot, guys!

Thank you for filing a bug with a reduction steps. Having a reduction definitely helped us spotting the bug and fixing it promptly.