Bug 39989 – Applying inline style to a text node whose parent is an inline editable root causes crash

• Component: HTML Editing
• Status: RESOLVED FIXED

[Veit Lehmann]

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.

[Tobias Struckmeier]

I was able to reproduce the bug on two machines as well (chrome 5.0.375.55)

[Tobias Struckmeier]

(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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

This also happens with Safari/WebKit 4.0.5, so it's not a recent regression.

[Kalle Vahlman]

*** Bug 37197 has been marked as a duplicate of this bug. ***

[Veit Lehmann]

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.

[Patrick Quinn-Graham]

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

[Patrick Quinn-Graham]

Created attachment 60693 [details]
Crash log

Example crash log from WebKit when this bug is triggered

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

(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!

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/64083.

[Veit Lehmann]

Cool, you rock! I will test the next nightly on Mac an Windows and give you feedback.

[Veit Lehmann]

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!

[Ryosuke Niwa]

(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.