WK2: Reproducible crash when dragging out of or over Safari window

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Enrica Casucci]

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.

[Enrica Casucci]

<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

[Enrica Casucci]

Created attachment 87908 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 87908 [details]
Patch

This looks strange. We no longer release WKPasteboardFilePromiseOwner in declareAndWriteDragImage(), but when do we release it now?

[Enrica Casucci]

(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.

[Darin Adler]

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.

[Darin Adler]

(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.

[Maciej Stachowiak]

Comment on attachment 87908 [details]
Patch

Marking r- based on Darin's comments.

[Darin Adler]

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.

[Enrica Casucci]

(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.

[Enrica Casucci]

The updated patch was reviewed in person by Darin Adler.
http://trac.webkit.org/changeset/82853