I hit this while indenting something that I pasted into GMail. If you open the attachment, it should repro.
Created attachment 32105 [details] Crash repro. The attached file should be suitable for use as a layout test (perhaps in LayoutTests/editing/execCommand/).
Created attachment 32106 [details] Crash reporter information.
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
If this is a reproducible crash, it should be a P1, no? Do we believe this is a regression?
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.)
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.
Created attachment 32167 [details] Fixes crash. 7 files changed, 68 insertions(+), 2 deletions(-)
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.
I tried to think of other unsplittable elements than root contentEditable nodes and TDs. I couldn't. Maybe jparent has ideas of others?
I don't really know the what you mean by 'unsplittble elements' ... would things like <img>, <iframe>, etc count?
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).
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.
Good call. Although, in this case, the code is still correct, since it gets rootEditableElement, which stops at the body element.
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...
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...
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.
http://trac.webkit.org/changeset/45561