Bug 57654 - WK2: Reproducible crash when dragging out of or over Safari window
Summary: WK2: Reproducible crash when dragging out of or over Safari window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-04-01 13:26 PDT by Enrica Casucci
Modified: 2011-04-04 12:31 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.91 KB, patch)
2011-04-01 13:43 PDT, Enrica Casucci
mjs: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2011-04-01 13:26:55 PDT
Repro steps:

1. Launch Safari and navigate to http://techshop.ws
2. Click on the "TechShop: Build your dreams here" image in the upper left corner and drag it to the desktop.
3. A file with the image is created on the desktop
4. Repeat step 2.

Expected:
You can drag again the image to the desktop

Actual:
The WebProcess crashes.
Comment 1 Enrica Casucci 2011-04-01 13:28:06 PDT
<rdar://problem/9139755>

Here is the stack trace


Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib               	0x00007fff8d6c10d0 objc_msgSend_vtable14 + 16
1   com.apple.CoreFoundation      	0x00007fff8e4e8e57 CFRelease + 167
2   com.apple.CoreFoundation      	0x00007fff8e501f52 __CFBasicHashDrain + 450
3   com.apple.CoreFoundation      	0x00007fff8e4e8f51 CFRelease + 417
4   com.apple.Foundation          	0x00007fff8c3e4c9b empty + 61
5   com.apple.Foundation          	0x00007fff8c3aa624 dealloc + 24
6   com.apple.Foundation          	0x00007fff8c3aa5e5 -[NSConcreteMapTable dealloc] + 64
7   com.apple.AppKit              	0x00007fff9139d44a -[_NSPasteboardOwnersCollection dealloc] + 41
8   com.apple.AppKit              	0x00007fff911134a2 _NSPasteboardReportChangedOwner + 74
9   com.apple.AppKit              	0x00007fff911133db -[NSPasteboard declareTypes:owner:] + 51
10  com.apple.WebCore             	0x00007fff8f35bd4c WebCore::EventHandler::createDraggingClipboard() const + 94
Comment 2 Enrica Casucci 2011-04-01 13:43:09 PDT
Created attachment 87908 [details]
Patch
Comment 3 Alexey Proskuryakov 2011-04-01 17:07:13 PDT
Comment on attachment 87908 [details]
Patch

This looks strange. We no longer release WKPasteboardFilePromiseOwner in declareAndWriteDragImage(), but when do we release it now?
Comment 4 Enrica Casucci 2011-04-01 17:11:58 PDT
(In reply to comment #3)
> (From update of attachment 87908 [details])
> This looks strange. We no longer release WKPasteboardFilePromiseOwner in declareAndWriteDragImage(), but when do we release it now?

It is stored in WebPage::setDragSource and released in WebPage::platformDragEnded(). I'll double check that I have not removed one RetainPtr too many.
Comment 5 Darin Adler 2011-04-01 17:25:54 PDT
Comment on attachment 87908 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:152
> -    RetainPtr<WKPasteboardOwner> pasteboardOwner(AdoptNS, [[WKPasteboardOwner alloc] initWithImage:image]);
> +    WKPasteboardOwner* pasteboardOwner = [[WKPasteboardOwner alloc] initWithImage:image];
>  
> -    RetainPtr<WKPasteboardFilePromiseOwner> filePromiseOwner(AdoptNS, [(WKPasteboardFilePromiseOwner *)[WKPasteboardFilePromiseOwner alloc] initWithSource:pasteboardOwner.get()]);
> -    m_page->setDragSource(filePromiseOwner.get());
> +    WKPasteboardFilePromiseOwner* filePromiseOwner = [(WKPasteboardFilePromiseOwner*)[WKPasteboardFilePromiseOwner alloc] initWithSource:pasteboardOwner];

This looks wrong to me. I don’t see any releases for each of these new objects. It seems to me we are just leaking them permanently. Could you add logging for when they are deallocated to prove they do get deallocated?

Also, as a matter of coding style, even if we don’t want to release these objects, we should still put them into a RetainPtr.

If we need to leak a reference to them, then we can call a leak function explicitly.
Comment 6 Darin Adler 2011-04-01 17:29:07 PDT
(In reply to comment #4)
> It is stored in WebPage::setDragSource and released in WebPage::platformDragEnded(). I'll double check that I have not removed one RetainPtr too many.

Yes, but setDragSource retains it. The problem is that the object starts with a retain count of 1 and if we don’t pair that with a release, then we have an immortal object. The current patch fixes the crash by leaking the objects. We need to find a way to fix it without leaking them.
Comment 7 Maciej Stachowiak 2011-04-03 16:16:05 PDT
Comment on attachment 87908 [details]
Patch

Marking r- based on Darin's comments.
Comment 8 Darin Adler 2011-04-03 23:51:18 PDT
I think a good fix for this is:

- Call pasteboardOwner.leakRef() in the call to declareTypes:owner: in WebDragClient::declareAndWriteDragImage, and
- Call CFRelease(self) in -[WKPasteboardOwner pasteboardChangedOwner:], with comments explaining the relationship between the two.

- Or, if you prefer, [pasteboardOwner.get() retain] just before the call to declareTypes:owner: in WebDragClient::declareAndWriteDragImage, and
- Call [self release] in -[WKPasteboardOwner pasteboardChangedOwner:], with comments explaining the relationship between the two.

I believe this will resolve the problem without adding a leak.
Comment 9 Enrica Casucci 2011-04-04 11:19:46 PDT
(In reply to comment #8)
> I think a good fix for this is:
> 
> - Call pasteboardOwner.leakRef() in the call to declareTypes:owner: in WebDragClient::declareAndWriteDragImage, and
> - Call CFRelease(self) in -[WKPasteboardOwner pasteboardChangedOwner:], with comments explaining the relationship between the two.
> 
> - Or, if you prefer, [pasteboardOwner.get() retain] just before the call to declareTypes:owner: in WebDragClient::declareAndWriteDragImage, and
> - Call [self release] in -[WKPasteboardOwner pasteboardChangedOwner:], with comments explaining the relationship between the two.
> 
> I believe this will resolve the problem without adding a leak.

I had already tried something like this, but I might have done something wrong, because I was still getting random crashes on dealloc. I want to track down the lifetime of these objects to fully understand what it going on here.
Also I thought that 

RetainPtr<WKPasteboardOwner> pasteboardOwner(AdoptNS, [[WKPasteboardOwner alloc] initWithImage:image]);

meant that the instance of the object being created was "adopted" by the RetainPtr producing a ref count of 1, therefore deallocating the object when RetainPtr was going out of scope.
Comment 10 Enrica Casucci 2011-04-04 12:31:51 PDT
The updated patch was reviewed in person by Darin Adler.
http://trac.webkit.org/changeset/82853