Safari crashes when I try to do a drag-and-drop of selected text in Google presentations

• Component: New Bugs
• Status: RESOLVED FIXED

[Anantha Keesara]

Steps:
1. Login into google docs and open a Presently with text.
2. Highlight some text.
3. Try to drag and drop it to a new location in the slide (a small plus-sign icon appears next to the mouse pointer)

Issue: Browser crashes.
Nightly tested: 38794

[Eric Seidel (no email)]

Should be easy find and to fix.  I'll investigate today.

[Scott Violet]

Things seem to get confused in ReplaceSelectionCommand::doApply. Specifically the second time through we end up here:

    if (shouldMergeStart(selectionStartWasStartOfParagraph, fragment.hasInterchangeNewlineAtStart())) {
        ....
        if (startOfParagraph(endOfInsertedContent) == startOfParagraphToMove)
            insertNodeAt(createBreakElement(document()).get(), endOfInsertedContent.deepEquivalent());


The problem is with insertNodeAt. The page has some javascript such that when insertNodeAt is invoked the script ends up calling back with the command "delete" to delete the text we're trying to insert at. Here's the trace showing the remove being invoked:

 	chrome.dll!WebCore::ContainerNode::removeChild(WebCore::Node * oldChild=0x064cc4c8, int & ec=-858993460)  Line 308	C++
 	chrome.dll!WebCore::Node::remove(int & ec=-858993460)  Line 521 + 0x17 bytes	C++
 	chrome.dll!WebCore::RemoveNodeCommand::doApply()  Line 53	C++
 	chrome.dll!WebCore::EditCommand::apply()  Line 92 + 0xf bytes	C++
 	chrome.dll!WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::EditCommand> cmd={...})  Line 99	C++
 	chrome.dll!WebCore::CompositeEditCommand::removeNode(WTF::PassRefPtr<WebCore::Node> node={...})  Line 199 + 0x28 bytes	C++
 	chrome.dll!WebCore::DeleteSelectionCommand::removeNode(WTF::PassRefPtr<WebCore::Node> node={...})  Line 377	C++
 	chrome.dll!WebCore::DeleteSelectionCommand::handleGeneralDelete()  Line 472	C++
 	chrome.dll!WebCore::DeleteSelectionCommand::doApply()  Line 766	C++
 	chrome.dll!WebCore::EditCommand::apply()  Line 92 + 0xf bytes	C++
 	chrome.dll!WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::EditCommand> cmd={...})  Line 99	C++
 	chrome.dll!WebCore::CompositeEditCommand::deleteSelection(const WebCore::VisibleSelection & selection={...}, bool smartDelete=false, bool mergeBlocksAfterDelete=true, bool replace=false, bool expandForSpecialElements=true)  Line 351 + 0x30 bytes	C++
 	chrome.dll!WebCore::TypingCommand::deleteKeyPressed(WebCore::TextGranularity granularity=CharacterGranularity, bool killRing=false)  Line 448	C++
 	chrome.dll!WebCore::TypingCommand::doApply()  Line 256	C++
 	chrome.dll!WebCore::EditCommand::apply()  Line 92 + 0xf bytes	C++
 	chrome.dll!WebCore::TypingCommand::deleteKeyPressed(WebCore::Document * document=0x07f9a378, bool smartDelete=false, WebCore::TextGranularity granularity=CharacterGranularity, bool killRing=false)  Line 97	C++
 	chrome.dll!WebCore::executeDelete(WebCore::Frame * frame=0x07f99ce0, WebCore::Event * __formal=0x00000000, WebCore::EditorCommandSource source=CommandFromDOM, WebCore::Event * __formal=0x00000000)  Line 289 + 0x21 bytes	C++
 	chrome.dll!WebCore::Editor::Command::execute(const WebCore::String & parameter={...}, WebCore::Event * triggeringEvent=0x00000000)  Line 1450 + 0x24 bytes	C++
>	chrome.dll!WebCore::Document::execCommand(const WebCore::String & commandName={...}, bool userInterface=false, const WebCore::String & value={...})  Line 3386 + 0x25 bytes	C++
 	chrome.dll!WebCore::DocumentInternal::execCommandCallback(const v8::Arguments & args={...})  Line 657 + 0x14 bytes	C++
 	chrome.dll!v8::internal::Builtin_HandleApiCall(int __argc__=4, v8::internal::Object * * __argv__=0x052fe740)  Line 380 + 0xe bytes	C++
 	0598016c()	
 	chrome.dll!v8::internal::Invoke(bool construct=false, v8::internal::Handle<v8::internal::JSFunction> func={...}, v8::internal::Handle<v8::internal::Object> receiver={...}, int argc=1, v8::internal::Object * * * args=0x052fe9dc, bool * has_pending_exception=0x052fe96b)  Line 90 + 0x34 bytes	C++
 	chrome.dll!v8::internal::Execution::Call(v8::internal::Handle<v8::internal::JSFunction> func={...}, v8::internal::Handle<v8::internal::Object> receiver={...}, int argc=1, v8::internal::Object * * * args=0x052fe9dc, bool * pending_exception=0x052fe96b)  Line 116 + 0x1f bytes	C++
 	chrome.dll!v8::Function::Call(v8::Handle<v8::Object> recv={...}, int argc=1, v8::Handle<v8::Value> * argv=0x052fe9dc)  Line 1939 + 0x1d bytes	C++
 	chrome.dll!WebCore::V8Proxy::CallFunction(v8::Handle<v8::Function> function={...}, v8::Handle<v8::Object> receiver={...}, int argc=1, v8::Handle<v8::Value> * args=0x052fe9dc)  Line 1460 + 0x1f bytes	C++
 	chrome.dll!WebCore::V8EventListener::CallListenerFunction(v8::Handle<v8::Value> jsevent={...}, WebCore::Event * event=0x07ccea80, bool isWindowEvent=false)  Line 225 + 0x26 bytes	C++
 	chrome.dll!WebCore::V8AbstractEventListener::handleEvent(WebCore::Event * event=0x07ccea80, bool isWindowEvent=false)  Line 111 + 0x22 bytes	C++
 	chrome.dll!WebCore::Node::handleLocalEvents(WebCore::Event * event=0x07ccea80, bool useCapture=false)  Line 2312 + 0x20 bytes	C++
 	chrome.dll!WebCore::Node::dispatchGenericEvent(WTF::PassRefPtr<WebCore::Event> prpEvent={...})  Line 2445 + 0x1d bytes	C++
 	chrome.dll!WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event> e={...}, int & ec=0)  Line 2366 + 0x12 bytes	C++
 	chrome.dll!WebCore::dispatchChildInsertionEvents(WebCore::Node * child=0x063ce8b0, int & ec=0)  Line 890 + 0x74 bytes	C++
 	chrome.dll!WebCore::ContainerNode::appendChild(WTF::PassRefPtr<WebCore::Node> newChild={...}, int & ec=0, bool shouldLazyAttach=false)  Line 490 + 0x12 bytes	C++
 	chrome.dll!WebCore::AppendNodeCommand::doApply()  Line 49	C++
 	chrome.dll!WebCore::EditCommand::apply()  Line 92 + 0xf bytes	C++
 	chrome.dll!WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::EditCommand> cmd={...})  Line 99	C++
 	chrome.dll!WebCore::CompositeEditCommand::appendNode(WTF::PassRefPtr<WebCore::Node> node={...}, WTF::PassRefPtr<WebCore::Element> parent={...})  Line 182 + 0x34 bytes	C++
 	chrome.dll!WebCore::CompositeEditCommand::insertNodeAfter(WTF::PassRefPtr<WebCore::Node> insertChild={...}, WTF::PassRefPtr<WebCore::Node> refChild={...})  Line 147	C++
 	chrome.dll!WebCore::CompositeEditCommand::insertNodeAt(WTF::PassRefPtr<WebCore::Node> insertChild={...}, const WebCore::Position & editingPosition={...})  Line 177	C++
 	chrome.dll!WebCore::ReplaceSelectionCommand::doApply()  Line 900 + 0x43 bytes	C++
 	chrome.dll!WebCore::EditCommand::apply()  Line 92 + 0xf bytes	C++
 	chrome.dll!WebCore::applyCommand(WTF::PassRefPtr<WebCore::EditCommand> command={...})  Line 228	C++
 	chrome.dll!WebCore::DragController::concludeEditDrag(WebCore::DragData * dragData=0x052ff6b4)  Line 410 + 0x4a bytes	C++
 	chrome.dll!WebCore::DragController::performDrag(WebCore::DragData * dragData=0x052ff6b4)  Line 192 + 0x17 bytes	C++
 	chrome.dll!WebViewImpl::DragTargetDrop(int client_x=353, int client_y=526, int screen_x=1789, int screen_y=1093)  Line 1522	C++
 	chrome.dll!RenderView::OnDragTargetDrop(const gfx::Point & client_pt={...}, const gfx::Point & screen_pt={...})  Line 2631 + 0x41 bytes	C++
 	chrome.dll!DispatchToMethod<RenderView,void (__thiscall RenderView::*)(gfx::Point const &,gfx::Point const &),gfx::Point,gfx::Point>(RenderView * obj=0x06365198, void (const gfx::Point &, const gfx::Point &)* method=0x010513f0, const Tuple2<gfx::Point,gfx::Point> & arg={...})  Line 398 + 0x26 bytes	C++
 	chrome.dll!IPC::MessageWithTuple<Tuple2<gfx::Point,gfx::Point> >::Dispatch<RenderView,void (__thiscall RenderView::*)(gfx::Point const &,gfx::Point const &)>(const IPC::Message * msg=0x07ccfdb0, RenderView * obj=0x06365198, void (const gfx::Point &, const gfx::Point &)* func=0x010513f0)  Line 1157 + 0x23 bytes	C++
 	chrome.dll!RenderView::OnMessageReceived(const IPC::Message & message={...})  Line 383 + 0x4a bytes	C++
 	chrome.dll!MessageRouter::RouteMessage(const IPC::Message & msg={...})  Line 39 + 0x13 bytes	C++
 	chrome.dll!MessageRouter::OnMessageReceived(const IPC::Message & msg={...})  Line 30 + 0x13 bytes	C++
 	chrome.dll!ChildThread::OnMessageReceived(const IPC::Message & msg={...})  Line 64 + 0x17 bytes	C++
 	chrome.dll!IPC::ChannelProxy::Context::OnDispatchMessage(const IPC::Message & message={...})  Line 174 + 0x1b bytes	C++
 	chrome.dll!DispatchToMethod<IPC::ChannelProxy::Context,void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &),IPC::Message>(IPC::ChannelProxy::Context * obj=0x04d75e18, void (const IPC::Message &)* method=0x010d5930, const Tuple1<IPC::Message> & arg={...})  Line 393 + 0xf bytes	C++
 	chrome.dll!RunnableMethod<IPC::ChannelProxy::Context,void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &),Tuple1<IPC::Message> >::Run()  Line 308 + 0x1e bytes	C++
 	chrome.dll!MessageLoop::RunTask(Task * task=0x07ccfd88)  Line 308 + 0xf bytes	C++
 	chrome.dll!MessageLoop::DeferOrRunPendingTask(const MessageLoop::PendingTask & pending_task={...})  Line 319	C++
 	chrome.dll!MessageLoop::DoWork()  Line 408 + 0xc bytes	C++
 	chrome.dll!base::MessagePumpForUI::DoRunLoop()  Line 208 + 0x1d bytes	C++
 	chrome.dll!base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate * delegate=0x052ffeb4, base::MessagePumpWin::Dispatcher * dispatcher=0x00000000)  Line 52 + 0xf bytes	C++
 	chrome.dll!base::MessagePumpWin::Run(base::MessagePump::Delegate * delegate=0x052ffeb4)  Line 78 + 0x1c bytes	C++
 	chrome.dll!MessageLoop::RunInternal()  Line 197 + 0x2a bytes	C++
 	chrome.dll!MessageLoop::RunHandler()  Line 181	C++
 	chrome.dll!MessageLoop::Run()  Line 155	C++
 	chrome.dll!base::Thread::ThreadMain()  Line 159	C++
 	chrome.dll!`anonymous namespace'::ThreadFunc(void * closure=0x04d7582c)  Line 26 + 0xf bytes	C++
 	kernel32.dll!7c80b713() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	

Once the node has been deleted everything gets confused.

Perhaps we should bail after insertNodeAt if the parent is null.

[Eric Seidel (no email)]

This may be the same as bug 19066.  Marking it as "depends on" even though it might not actually "depend on" fixing that bug.  Investigating both today.

[Scott Violet]

Adding:

            if (!startOfParagraphToMove.deepEquivalent().node()->parent()) {
                // Inserting the break resulted in deleting the node we're going to move.
                return;
            }

Right after:

        if (startOfParagraph(endOfInsertedContent) == startOfParagraphToMove) {
            insertNodeAt(createBreakElement(document()).get(), endOfInsertedContent.deepEquivalent());

In ReplaceSelectionCommand fixes the bug, but when attempting to drag the third time a different crash occurs.

Seems to me any place in ReplaceSelectionCommand that does mutation needs to make sure the document is sane before continuing. This is based on my limited knowledge of webkit though, so I could of course be completely wrong.

[Eric Seidel (no email)]

Here is the stack trace of when we hit the first ASSERT:
    ASSERT(isStartOfParagraph(startOfParagraphToMove));

startOfParagraphToMove is a CharacterData node (or a TextNode?) which is not in any document.  It's got a null parent and null document.

#0	0x035cbe2e in WebCore::CompositeEditCommand::moveParagraph at CompositeEditCommand.cpp:732
#1	0x03bb009e in WebCore::ReplaceSelectionCommand::doApply at ReplaceSelectionCommand.cpp:904
#2	0x0370073d in WebCore::EditCommand::apply at EditCommand.cpp:92
#3	0x037007b5 in WebCore::applyCommand at EditCommand.cpp:227
#4	0x036fd1ab in WebCore::DragController::concludeEditDrag at DragController.cpp:410
#5	0x036fd699 in WebCore::DragController::performDrag at DragController.cpp:192
#6	0x00385891 in -[WebView performDragOperation:] at WebView.mm:3196
#7	0x93957931 in NSCoreDragReceiveProc
#8	0x9001a1b0 in DoDropMessage
#9	0x9001a126 in SendDropMessage
#10	0x9001748e in DragInApplication
#11	0x90015f32 in CoreDragStartDragging
#12	0x939557b5 in -[NSCoreDragManager _dragUntilMouseUp:accepted:]
#13	0x939546d6 in -[NSCoreDragManager dragImage:fromWindow:at:offset:event:pasteboard:source:slideBack:]
#14	0x93954120 in -[NSWindow(NSDrag) dragImage:at:offset:event:pasteboard:source:slideBack:]
#15	0x00318235 in -[WebHTMLView dragImage:at:offset:event:pasteboard:source:slideBack:] at WebHTMLView.mm:3262
#16	0x002f06fb in WebDragClient::startDrag at WebDragClient.mm:116
#17	0x036fa7cb in WebCore::DragController::doSystemDrag at DragController.cpp:751
#18	0x036fb9f2 in WebCore::DragController::startDrag at DragController.cpp:686
#19	0x037208cd in WebCore::EventHandler::handleDrag at EventHandler.cpp:2098
#20	0x037209bc in WebCore::EventHandler::handleMouseDraggedEvent at EventHandler.cpp:394
#21	0x03721386 in WebCore::EventHandler::handleMouseMoveEvent at EventHandler.cpp:1271
#22	0x03725a50 in WebCore::EventHandler::mouseDragged at EventHandlerMac.mm:505
#23	0x0031bc10 in -[WebHTMLView mouseDragged:] at WebHTMLView.mm:3275

[Eric Seidel (no email)]

I'm a little surprised at the code path taken by this code:

        if (dragIsMove(innerFrame->selection())) {
            bool smartMove = innerFrame->selectionGranularity() == WordGranularity 
                          && innerFrame->editor()->smartInsertDeleteEnabled() 
                          && dragData->canSmartReplace();
            applyCommand(MoveSelectionCommand::create(fragment, dragCaret.base(), smartMove));
        } else {
            if (setSelectionToDragCaret(innerFrame, dragCaret, range, point))
                applyCommand(ReplaceSelectionCommand::create(m_document, fragment, true, dragData->canSmartReplace(), chosePlainText));   // WE TAKE THIS PATH
        }    

I would have expected that the drag would be a "move" since I just dragged and dropped from the rich text field to the same rich text field.

    return m_document == m_dragInitiator && selection->isContentEditable() && !isCopyKeyDown();

Maybe presently (Google presentations) is doing something funny with the drag events.

[Eric Seidel (no email)]

(gdb) p m_document
$1 = (class WebCore::Document *) 0x754ca00
(gdb) p m_dragInitiator
$2 = (class WebCore::Document *) 0x7459800

Yup, clearly we're dragging and dropping between separate documents in presently, even though it doesn't appear that way to the user.

[Scott Violet]

This fixes the crashers I'm seeing:

Index: WebCore/editing/ReplaceSelectionCommand.cpp

===================================================================

--- WebCore/editing/ReplaceSelectionCommand.cpp	(revision 10629)
+++ WebCore/editing/ReplaceSelectionCommand.cpp	(working copy)
@@ -839,6 +839,11 @@
     
     fragment.removeNode(refNode);
     insertNodeAtAndUpdateNodesInserted(refNode, insertionPos);
+
+    if (!refNode->parent()) {
+        // Inserting the node resulted in deleting the node we're going to move.
+        return;
+    }
     
     while (node) {
         Node* next = node->nextSibling();
@@ -896,8 +901,13 @@
         // Insert a line break just after the inserted content to separate it from what 
         // comes after and prevent that from happening.
         VisiblePosition endOfInsertedContent = positionAtEndOfInsertedContent();
-        if (startOfParagraph(endOfInsertedContent) == startOfParagraphToMove)
+        if (startOfParagraph(endOfInsertedContent) == startOfParagraphToMove) {
             insertNodeAt(createBreakElement(document()).get(), endOfInsertedContent.deepEquivalent());
+            if (!startOfParagraphToMove.deepEquivalent().node()->parent()) {
+                // Inserting the break resulted in deleting the node we're going to move.
+                return;
+            }
+        }
         
         // FIXME: Maintain positions for the start and end of inserted content instead of keeping nodes.  The nodes are
         // only ever used to create positions where inserted content starts/ends.


Eric, if you think this is the right approach and looks good I'll clean it up, try and create a layout test and submit a proper fix. What do you think?

[Eric Seidel (no email)]

I'm not able to reproduce this in "writely" (docs.google.com, the word-like Documents portion)

[Eric Seidel (no email)]

Ok, found out that Presently is creating an overlay iframe, which snags the drag.  So the drag does go cross-document.  Also, in this fancy iframe, they're signing up for the DOMNodeInserted and calling:

document.execCommand("SelectAll");
document.execCommand("Delete");
document.execCommand("SelectAll");

from within their DOMNodeInserted handler. :)

No wonder WebKit is having kittens here.

[Eric Seidel (no email)]

Created attachment 28197 [details]
manual test case

This test case could possibly be reduced further.  But it shows what presently is doing at least.

[Eric Seidel (no email)]

Created attachment 28201 [details]
Total hack, but it works

 WebCore/editing/CompositeEditCommand.cpp    |    4 ++++
 WebCore/editing/ReplaceSelectionCommand.cpp |   19 ++++++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

[Eric Seidel (no email)]

This is not a fix I'm proud of.  But I don't know of a more elegant way to do this yet.  We need to find a nice way to re-write large chunks of the editing code to be mutation event safe!

[Eric Seidel (no email)]

The attached fix seems to fix bug 24302 as well.  I'll dupe them.

[Eric Seidel (no email)]

*** Bug 24302 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Created attachment 28205 [details]
Fix this crash by adding ugly null checks

 LayoutTests/ChangeLog                              |   11 +++++
 ...crash-on-drag-with-mutation-events-expected.txt |    1 +
 .../crash-on-drag-with-mutation-events.html        |   47 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   19 ++++++++
 WebCore/editing/CompositeEditCommand.cpp           |    4 ++
 WebCore/editing/ReplaceSelectionCommand.cpp        |   19 +++++++-
 6 files changed, 98 insertions(+), 3 deletions(-)

[Darin Fisher (:fishd, Google)]

Comment on attachment 28205 [details]
Fix this crash by adding ugly null checks

This patch looks safe and reasonable to me.  Were you hoping for a more expert opinion on the editor changes?

[Justin Garcia]

looks fine to me, r=me2

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/editing/selection/crash-on-drag-with-mutation-events-expected.txt
	A	LayoutTests/editing/selection/crash-on-drag-with-mutation-events.html
	M	WebCore/ChangeLog
	M	WebCore/editing/CompositeEditCommand.cpp
	M	WebCore/editing/ReplaceSelectionCommand.cpp
Committed r41645