Bug 20471 - Crash when dragging text in Google Docs
Summary: Crash when dragging text in Google Docs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL: http://docs.google.com
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-08-20 17:26 PDT by Eric Roman
Modified: 2010-08-29 19:10 PDT (History)
8 users (show)

See Also:


Attachments
Crash on WebKit r35844 Mac OS X (25.91 KB, text/plain)
2008-08-20 17:32 PDT, Eric Roman
no flags Details
Crash on WebKit r35806 WinXP (2.83 KB, text/plain)
2008-08-20 17:36 PDT, Eric Roman
no flags Details
Reduction (1.53 KB, text/html)
2010-08-27 15:01 PDT, Enrica Casucci
no flags Details
Patch (5.78 KB, patch)
2010-08-27 15:25 PDT, Enrica Casucci
adele: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Roman 2008-08-20 17:26:20 PDT
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
Comment 1 Eric Roman 2008-08-20 17:32:25 PDT
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
Comment 2 Eric Roman 2008-08-20 17:36:32 PDT
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
Comment 3 Mark Rowe (bdash) 2008-08-20 19:12:37 PDT
<rdar://problem/5625820>
Comment 4 Eric Roman 2008-08-25 14:26:55 PDT
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
Comment 5 Yaar Schnitman 2009-08-24 17:40:54 PDT
Might be related to https://bugs.webkit.org/show_bug.cgi?id=28697
Comment 6 Enrica Casucci 2010-08-27 15:01:33 PDT
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.
Comment 7 Enrica Casucci 2010-08-27 15:06:03 PDT
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.
Comment 8 Enrica Casucci 2010-08-27 15:25:47 PDT
Created attachment 65775 [details]
Patch
Comment 9 Adele Peterson 2010-08-27 18:08:39 PDT
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!
Comment 10 Darin Adler 2010-08-27 18:35:24 PDT
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.
Comment 11 Enrica Casucci 2010-08-29 17:52:34 PDT
(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.
Comment 12 Enrica Casucci 2010-08-29 18:32:31 PDT
Committed revision 66344.
Comment 13 WebKit Review Bot 2010-08-29 19:10:31 PDT
http://trac.webkit.org/changeset/66344 might have broken Qt Linux Release