Bug 20471 – Crash when dragging text in Google Docs

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Eric Roman]

Reproduces in google presentations.

(1) goto http://docs.google.com
(2) create new presentation
(3) enter some text in box
(4) select portion of it
(5) drag/drop the text
 (i can get it to crash fairly reliably when dropping on itself, and when dropping outside of text box)

Reproduced crash in:
Safari 3.1 (mac/winxp)
WebKit r35806 winxp
WebKit r35844 mac

[Eric Roman]

Created attachment 22910 [details]
Crash on WebKit r35844 Mac OS X

top of call stack:

WebCore::Node::nodeIndex() const + 6
WebCore::positionBeforeNode(WebCore::Node const*) + 26
WebCore::ReplaceSelectionCommand::positionAtStartOfInsertedContent() + 35
WebCore::ReplaceSelectionCommand::doApply() + 7044
WebCore::EditCommand::apply() + 76
WebCore::DragController::concludeDrag(WebCore::DragData*, WebCore::DragDestinationAction) + 2724

[Eric Roman]

Created attachment 22911 [details]
Crash on WebKit r35806 WinXP

top of call stack:

WebCore::InsertNodeBeforeCommand::doApply+0x2a
WebCore::EditCommand::apply+0xe2
WebCore::CompositeEditCommand::applyCommandToComposite+0x16
WebCore::CompositeEditCommand::insertNodeBefore+0x45
WebCore::RemoveNodePreservingChildrenCommand::doApply+0x39

[Mark Rowe (bdash)]

<rdar://problem/5625820>

[Eric Roman]

This hits the same assert as bug 19066, could be the same problem.

    ASSERT(isStartOfParagraph(startOfParagraphToMove));

Callstack:

WebCore::CompositeEditCommand::moveParagraph
WebCore::ReplaceSelectionCommand::doApply
WebCore::EditCommand::apply
WebCore::applyCommand

[Yaar Schnitman]

Might be related to https://bugs.webkit.org/show_bug.cgi?id=28697

[Enrica Casucci]

Created attachment 65772 [details]
Reduction

I'm fairly confident that this is the problem here.
Running this test in Debug, we hit an assert sooner that helps identify the problem.

[Enrica Casucci]

The problem is occurring when replacing at the border with a span.
if you have:
<p><span style='background-color: red'>hello</span>world</p>
and you try to replace 'wo' with '<p>Wo</p>
we end up in a situation where the replaced content will be inside the span.
This is not only wrong, but with some styles it crashes.

[Enrica Casucci]

Created attachment 65775 [details]
Patch

[Adele Peterson]

Comment on attachment 65775 [details]
Patch

The expected results are a little hard to read, but I get that there's not supposed to be a crash :)

Looks good!

[Darin Adler]

Comment on attachment 65775 [details]
Patch

WebCore/editing/ReplaceSelectionCommand.cpp:1000
 +              RefPtr<Node>placeholder = createBreakElement(document());
Need a space after the ">".

WebCore/editing/ReplaceSelectionCommand.cpp:1001
 +              insertNodeBefore(placeholder, refNode.get());
This could be placeholder.release() to avoid a tiny bit of reference count churn.

Or you could just put the createBreakElement function call right into the insertNodeBefore function call, which would also fix both of the above issues.

[Enrica Casucci]

(In reply to comment #10)
> (From update of attachment 65775 [details])
> WebCore/editing/ReplaceSelectionCommand.cpp:1000
>  +              RefPtr<Node>placeholder = createBreakElement(document());
> Need a space after the ">".
>
Sorry, this is a leftover from a previous version where I was keeping placeholder to remove it later, but it wasn't necessary. I've changed the code as you suggested.
 
> WebCore/editing/ReplaceSelectionCommand.cpp:1001
>  +              insertNodeBefore(placeholder, refNode.get());
> This could be placeholder.release() to avoid a tiny bit of reference count churn.
> 
> Or you could just put the createBreakElement function call right into the insertNodeBefore function call, which would also fix both of the above issues.

[Enrica Casucci]

Committed revision 66344.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66344 might have broken Qt Linux Release