Bug 64766 - Crash under WebPage::platformDragEnded when dragging on Mac
Summary: Crash under WebPage::platformDragEnded when dragging on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2011-07-18 16:49 PDT by Brian Weinstein
Modified: 2011-07-19 10:21 PDT (History)
3 users (show)

See Also:


Attachments
[PATCH] Prep Work (3.44 KB, patch)
2011-07-18 16:52 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Speculative Fix (8.91 KB, patch)
2011-07-18 19:38 PDT, Brian Weinstein
enrica: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2011-07-18 16:49:54 PDT
<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
Comment 1 Brian Weinstein 2011-07-18 16:52:00 PDT
Created attachment 101236 [details]
[PATCH] Prep Work
Comment 2 Brian Weinstein 2011-07-18 16:52:33 PDT
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.
Comment 3 Darin Adler 2011-07-18 16:55:38 PDT
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 "{ }".
Comment 4 WebKit Review Bot 2011-07-18 16:55:44 PDT
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.
Comment 5 Brian Weinstein 2011-07-18 16:59:08 PDT
(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!
Comment 6 Brian Weinstein 2011-07-18 17:15:30 PDT
Prep patch landed in r91222.
Comment 7 Brian Weinstein 2011-07-18 19:38:31 PDT
Created attachment 101260 [details]
[PATCH] Speculative Fix
Comment 8 Early Warning System Bot 2011-07-18 19:46:59 PDT
Comment on attachment 101260 [details]
[PATCH] Speculative Fix

Attachment 101260 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9139009
Comment 9 Brian Weinstein 2011-07-19 09:57:15 PDT
Fixed Windows and Qt build by removing stub implementation of WebPage::platformDragEnd locally.
Comment 10 Enrica Casucci 2011-07-19 10:11:44 PDT
Comment on attachment 101260 [details]
[PATCH] Speculative Fix

The approach looks very reasonable to me.
Comment 11 Brian Weinstein 2011-07-19 10:17:15 PDT
Landed in r91266.
Comment 12 Darin Adler 2011-07-19 10:18:42 PDT
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!
Comment 13 Darin Adler 2011-07-19 10:20:51 PDT
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.
Comment 14 Darin Adler 2011-07-19 10:21:23 PDT
(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.