Bug 26872 – Crash indenting a table cell

• Component: HTML Editing
• Status: RESOLVED FIXED

[David Levin]

I hit this while indenting something that I pasted into GMail.

If you open the attachment, it should repro.

[David Levin]

Created attachment 32105 [details]
Crash repro.

The attached file should be suitable for use as a layout test (perhaps in  LayoutTests/editing/execCommand/).

[David Levin]

Created attachment 32106 [details]
Crash reporter information.

[David Levin]

A snippet of the crashing stack for easier searching:

0. WebCore::SplitElementCommand::SplitElementCommand(WTF::PassRefPtr<WebCore::Element>, WTF::PassRefPtr<WebCore::Node>) + 29
1. WebCore::CompositeEditCommand::splitTreeToNode(WebCore::Node*, WebCore::Node*, bool) + 497
2. WebCore::IndentOutdentCommand::indentIntoBlockquote(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WTF::RefPtr<WebCore::Element>&) + 561
3. WebCore::IndentOutdentCommand::indentRegion() + 1033

[Eric Seidel (no email)]

If this is a reproducible crash, it should be a P1, no?

Do we believe this is a regression?

[David Levin]

I don't know if it's a regression.  It has existed for a few months at least.

(About the priority, thanks -- I didn't know what's assigned each priority.)

[David Levin]

btw, I managed to find the crash in the Google Chrome crash tracker, and it is only in the top 1800 :( but I hope with this test is will be an easy fix.

[Ojan Vafai]

Created attachment 32167 [details]
Fixes crash.

 7 files changed, 68 insertions(+), 2 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 32167 [details]
Fixes crash.

The fix looks fine to me.  I'm not sure if it's at the right layer or not.  Justin would know that better.

I assume <td> and root contentEditable nodes are the only non-splitable nodes?

Justin has historically been very slow to respond to Bugzilla bugs, so I'm going to go ahead and r+ this and we can roll it out or fix it differently if Justin later comments and feels it should be done at a different level.

[Ojan Vafai]

I tried to think of other unsplittable elements than root contentEditable nodes and TDs. I couldn't. Maybe jparent has ideas of others?

[Julie Parent]

I don't really know the what you mean by 'unsplittble elements' ... would things like <img>, <iframe>, etc count?

[Ojan Vafai]

I guess unsplittable *container* elements would be more accurate. They're elements such that when you split a tree, you stop at those elements because they shouldn't be split. Splitting contentEditable nodes obviously doesn't make sense. But splitting TDs also doesn't make sense (e.g. hitting enter in a TD shouldn't cause the TD to split).

[Julie Parent]

body!  (while I know this one should be obvious, I've seen a bunch of bugs where we get multiple bodies ... although that may have been in Firefox, not WebKit).

Can't think of any other ones though, besides TD ancestors, but I think you'd never hit those cases since a TD isn't splittable.

[Ojan Vafai]

Good call. Although, in this case, the code is still correct, since it gets rootEditableElement, which stops at the body element.

[Ryosuke Niwa]

So this patch is good works but we can arguably change indentIntoBlockquote, particularly where we have the splitTreeToNode.   We can have something like

if (nodeToSplitTo == start.node) {
... special case bail out code...
}
else {
...
}

I but then we still need to change the selection, so it's not an elegant solution to the problem...

[Ryosuke Niwa]

Can we instead have a function that returns td/th or root editable element?  Call it unsplitableEditableRoot and it does:

Node* rootUnsplittableEditingNode( Node* node ) {
        Position start = startOfCurrentParagraph.deepEquivalent();
        Node* enclosingCell = enclosingNodeOfType(start, &isTableCell);
        return enclosingCell ? enclosingCell : editableRootForPosition(start);
}

That way, we can use that in indentIIntoBlockquote and also for the special case in indentRegion.  And I feel like editableRootForPosition should be replaced by this function in many places...

[Ojan Vafai]

I don't think rootUnsplittableEditingNode make sense. I started going down that route, but in the case of a table cell, we're not actually getting the root, we're getting the first enclosing table cell. I agree that this functionality may need to be pulled into a helper function or two, but I think that would better be done when dealing with the other cases where we need this.

That all said, it's not clear to me that the rootEditableElement is always what we care about. It might make sense to replace some uses of rootEditableElement with something like enclosingUnsplittableEditingNode, which returns the deepest matching ancestor.

[Ojan Vafai]

http://trac.webkit.org/changeset/45561