Bug 64766 – Crash under WebPage::platformDragEnded when dragging on Mac

• Component: WebKit2
• Status: RESOLVED FIXED

[Brian Weinstein]

<rdar://problem/9548174>

There is a crash under WebPage::platformDragEnded when dragging on Mac:

* BACKTRACE ('>' indicates stack frame used for CrashTracer aggregation)

   1 libobjc.A.dylib                0x7fff9592de10 objc_msgSend_vtable5 + 0x10
>  2 com.apple.AppKit               0x7fff8ed9105c -[NSFilePromiseDragSource draggedImage:endedAt:operation:] + 0x65
   3 com.apple.WebKit2              0x7fff8f51e6c5 WebKit::WebPage::platformDragEnded() + 0x47
   4 com.apple.WebKit2              0x7fff8f517e32 WebKit::WebPage::dragEnded(WebCore::IntPoint, WebCore::IntPoint, unsigned long long) + 0x38
   5 com.apple.WebKit2              0x7fff8f548c9b void CoreIPC::handleMessage<Messages::WebPage::DragEnded, WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::IntPoint, WebCore::IntPoint, unsigned long long)>(CoreIPC::ArgumentDecoder*, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::IntPoint, WebCore::IntPoint, unsigned long long)) + 0x3d
   6 com.apple.WebKit2              0x7fff8f4ba540 CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::ArgumentDecoder>&) + 0xac
   7 com.apple.WebKit2              0x7fff8f4ba47b CoreIPC::Connection::dispatchMessages() + 0x67
   8 com.apple.WebKit2              0x7fff8f4b7b3d RunLoop::performWork() + 0x6f
   9 com.apple.WebKit2              0x7fff8f4b7aae RunLoop::performWork(void*) + 0x4c
  10 com.apple.CoreFoundation       0x7fff8a2cc591 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 0x11
  11 com.apple.CoreFoundation       0x7fff8a2cbdfd __CFRunLoopDoSources0 + 0xfd
  12 com.apple.CoreFoundation       0x7fff8a2f2aa9 __CFRunLoopRun + 0x389
  13 com.apple.CoreFoundation       0x7fff8a2f23e6 CFRunLoopRunSpecific + 0xe6
  14 com.apple.HIToolbox            0x7fff9276aa1b RunCurrentEventLoopInMode + 0x115
  15 com.apple.HIToolbox            0x7fff9277213d ReceiveNextEventCommon + 0x163
  16 com.apple.HIToolbox            0x7fff92771fca BlockUntilNextEventMatchingListInMode + 0x3e
  17 com.apple.AppKit               0x7fff8e6cb6a9 _DPSNextEvent + 0x293
  18 com.apple.AppKit               0x7fff8e6cafb0 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 0x87
  19 com.apple.AppKit               0x7fff8e6c795b -[NSApplication run] + 0x1c8
  20 com.apple.WebKit2              0x7fff8f530e57 WebKit::WebProcessMain(WebKit::CommandLine const&) + 0x24b
  21 com.apple.WebKit2              0x7fff8f5162b0 WebKitMain + 0x10c
  22 com.apple.WebProcess           0x106f0cda4 start + 0x0

[Brian Weinstein]

Created attachment 101236 [details]
[PATCH] Prep Work

[Brian Weinstein]

Comment on attachment 101236 [details]
[PATCH] Prep Work

View in context: https://bugs.webkit.org/attachment.cgi?id=101236&action=review

> Source/WebCore/page/DragClient.h:73
> +        virtual void declareAndWriteDragImage(NSPasteboard *, DOMElement*, NSURL *, NSString*, Frame *) {}

Oops, I meant to make this NSString *, Frame*. Fixed locally.

[Darin Adler]

Comment on attachment 101236 [details]
[PATCH] Prep Work

View in context: https://bugs.webkit.org/attachment.cgi?id=101236&action=review

> Source/WebCore/page/DragClient.h:70
> +        // Mac specific helper functions to allow access to functionality in webkit -- such as 

I’d put a hyphen in Mac-specific. And I’d capitalize WebKit. And I’d try to make it grammatical:

    ... functionality in WebKit, for use in places such as
    web archives and NSPasteboard extras.

Maybe rewrite it entirely for clarity.

> Source/WebCore/page/DragClient.h:78
> +        virtual void declareAndWriteDragImage(NSPasteboard *, DOMElement*, NSURL *, NSString*, Frame *) {}
>  #endif
>          
> -        virtual ~DragClient() {};
> +        virtual void dragEnded() {}
> +
> +        virtual ~DragClient() {}

Our style is to put a space between the braces "{ }".

[WebKit Review Bot]

Attachment 101236 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/page/DragClient.h:73:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/page/DragClient.h:76:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/page/DragClient.h:78:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Weinstein]

(In reply to comment #3)
> (From update of attachment 101236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101236&action=review
> 
> > Source/WebCore/page/DragClient.h:70
> > +        // Mac specific helper functions to allow access to functionality in webkit -- such as 
> 
> I’d put a hyphen in Mac-specific. And I’d capitalize WebKit. And I’d try to make it grammatical:
> 
>     ... functionality in WebKit, for use in places such as
>     web archives and NSPasteboard extras.
> 
> Maybe rewrite it entirely for clarity.

Rewritten: // Mac-specific helper function to allow access to web archives and NSPasteboard extras in WebKit.

> 
> > Source/WebCore/page/DragClient.h:78
> > +        virtual void declareAndWriteDragImage(NSPasteboard *, DOMElement*, NSURL *, NSString*, Frame *) {}
> >  #endif
> >          
> > -        virtual ~DragClient() {};
> > +        virtual void dragEnded() {}
> > +
> > +        virtual ~DragClient() {}
> 
> Our style is to put a space between the braces "{ }".

Fixed, style bot caught that one too!

[Brian Weinstein]

Prep patch landed in r91222.

[Brian Weinstein]

Created attachment 101260 [details]
[PATCH] Speculative Fix

[Early Warning System Bot]

Comment on attachment 101260 [details]
[PATCH] Speculative Fix

Attachment 101260 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9139009

[Brian Weinstein]

Fixed Windows and Qt build by removing stub implementation of WebPage::platformDragEnd locally.

[Enrica Casucci]

Comment on attachment 101260 [details]
[PATCH] Speculative Fix

The approach looks very reasonable to me.

[Brian Weinstein]

Landed in r91266.

[Darin Adler]

Comment on attachment 101260 [details]
[PATCH] Speculative Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=101260&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:178
> +    // The drag source we care about here is NSFilePromiseDragSource, which doesn't look at
> +    // the arguments. It's OK to just pass arbitrary constant values, so we just pass all zeroes.
> +    [m_filePromiseOwner.get() draggedImage:nil endedAt:NSZeroPoint operation:NSDragOperationNone];

You need to retain m_filePromiseOwner an extra time here because draggedImage releases it. Without that retain, this code does an overrelease.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:-690
> -    // The draggedImage method releases its responder; we retain here to balance that.
> -    [m_dragSource.get() retain];

This code is needed! You removed it!

[Darin Adler]

This patch reintroduces bug 57654 by removing the fix for that bug. Please add it back. See <http://trac.webkit.org/changeset/82853> for the original fix.

[Darin Adler]

(In reply to comment #13)
> This patch reintroduces bug 57654 by removing the fix for that bug. Please add it back. See <http://trac.webkit.org/changeset/82853> for the original fix.

By removing half of the fix for that bug, I mean.