Bug 26872 - Crash indenting a table cell
Summary: Crash indenting a table cell
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Ojan Vafai
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2009-06-30 20:04 PDT by David Levin
Modified: 2009-07-06 13:40 PDT (History)
6 users (show)

See Also:


Attachments
Crash repro. (393 bytes, text/html)
2009-06-30 20:06 PDT, David Levin
no flags Details
Crash reporter information. (42.65 KB, text/plain)
2009-06-30 20:10 PDT, David Levin
no flags Details
Fixes crash. (5.67 KB, patch)
2009-07-01 18:22 PDT, Ojan Vafai
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-06-30 20:04:04 PDT
I hit this while indenting something that I pasted into GMail.

If you open the attachment, it should repro.
Comment 1 David Levin 2009-06-30 20:06:16 PDT
Created attachment 32105 [details]
Crash repro.

The attached file should be suitable for use as a layout test (perhaps in  LayoutTests/editing/execCommand/).
Comment 2 David Levin 2009-06-30 20:10:17 PDT
Created attachment 32106 [details]
Crash reporter information.
Comment 3 David Levin 2009-06-30 20:17:46 PDT
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
Comment 4 Eric Seidel (no email) 2009-07-01 11:52:53 PDT
If this is a reproducible crash, it should be a P1, no?

Do we believe this is a regression?
Comment 5 David Levin 2009-07-01 13:25:40 PDT
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.)
Comment 6 David Levin 2009-07-01 13:27:20 PDT
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.
Comment 7 Ojan Vafai 2009-07-01 18:22:15 PDT
Created attachment 32167 [details]
Fixes crash.

 7 files changed, 68 insertions(+), 2 deletions(-)
Comment 8 Eric Seidel (no email) 2009-07-02 14:34:07 PDT
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.
Comment 9 Ojan Vafai 2009-07-02 14:38:18 PDT
I tried to think of other unsplittable elements than root contentEditable nodes and TDs. I couldn't. Maybe jparent has ideas of others?
Comment 10 Julie Parent 2009-07-02 15:32:54 PDT
I don't really know the what you mean by 'unsplittble elements' ... would things like <img>, <iframe>, etc count?
Comment 11 Ojan Vafai 2009-07-02 15:41:11 PDT
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).
Comment 12 Julie Parent 2009-07-02 15:53:02 PDT
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.
Comment 13 Ojan Vafai 2009-07-02 16:29:07 PDT
Good call. Although, in this case, the code is still correct, since it gets rootEditableElement, which stops at the body element.
Comment 14 Ryosuke Niwa 2009-07-03 00:45:36 PDT
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...
Comment 15 Ryosuke Niwa 2009-07-03 00:56:09 PDT
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...
Comment 16 Ojan Vafai 2009-07-06 13:36:40 PDT
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.
Comment 17 Ojan Vafai 2009-07-06 13:40:35 PDT
http://trac.webkit.org/changeset/45561