57654 2011-04-01 13:26:55 -0700 WK2: Reproducible crash when dragging out of or over Safari window 2011-04-04 12:31:51 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED InRadar P2 Normal --- 1 enrica enrica adele ap darin sam oldest_to_newest 378034 0 enrica 2011-04-01 13:26:55 -0700 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. 378037 1 enrica 2011-04-01 13:28:06 -0700 <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 378043 2 87908 enrica 2011-04-01 13:43:09 -0700 Created attachment 87908 Patch 378223 3 87908 ap 2011-04-01 17:07:13 -0700 Comment on attachment 87908 Patch This looks strange. We no longer release WKPasteboardFilePromiseOwner in declareAndWriteDragImage(), but when do we release it now? 378228 4 enrica 2011-04-01 17:11:58 -0700 (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. 378235 5 87908 darin 2011-04-01 17:25:54 -0700 Comment on attachment 87908 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. 378238 6 darin 2011-04-01 17:29:07 -0700 (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. 378594 7 87908 mjs 2011-04-03 16:16:05 -0700 Comment on attachment 87908 Patch Marking r- based on Darin's comments. 378672 8 darin 2011-04-03 23:51:18 -0700 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. 378900 9 enrica 2011-04-04 11:19:46 -0700 (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. 378962 10 enrica 2011-04-04 12:31:51 -0700 The updated patch was reviewed in person by Darin Adler. http://trac.webkit.org/changeset/82853 87908 2011-04-01 13:43:09 -0700 2011-04-03 16:16:04 -0700 Patch dragimagecrash.txt text/plain 4004 enrica SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDgyNzEyKQorKysgU291cmNlL1dlYktpdDIvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjUgQEAKKzIwMTEtMDQtMDEgIEVucmljYSBD YXN1Y2NpICA8ZW5yaWNhQGFwcGxlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBXSzI6IFJlcHJvZHVjaWJsZSBjcmFzaCB3aGVuIGRyYWdnaW5n IG91dCBvZiBvciBvdmVyIFNhZmFyaSB3aW5kb3cuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD01NzY1NAorICAgICAgICA8cmRhcjovL3Byb2JsZW0vOTEz OTc1NT4KKyAgICAgICAgCisgICAgICAgIFdoZW4gdGhlIHBhc3RlYm9hcmQgb3duZXJzaGlwIGNo YW5nZXMsIEFwcEtpdCBub3RpZmllcyB0aGUgcHJldmlvdXMKKyAgICAgICAgb3duZXIgb2YgdGhl IGNoYW5nZSBieSBjYWxsaW5nIHBhc3RlYm9hcmRDaGFuZ2VkT3duZXIuIFdlIG5lZWQgdG8gZW5z dXJlCisgICAgICAgIHRoYXQgdGhlIG93bmVyIG9iamVjdCBkb2Vzbid0IGdldCByZWxlYXNlZCBi ZWZvcmUgdGhlIGNoYW5nZSBvZgorICAgICAgICBvd25lcnNoaXAgdGFrZXMgcGxhY2UuCisgICAg ICAgIAorICAgICAgICAqIFdlYlByb2Nlc3MvV2ViQ29yZVN1cHBvcnQvbWFjL1dlYkRyYWdDbGll bnRNYWMubW06CisgICAgICAgIChXZWJLaXQ6OldlYkRyYWdDbGllbnQ6OmRlY2xhcmVBbmRXcml0 ZURyYWdJbWFnZSk6IFJlbW92ZWQKKyAgICAgICAgUmV0YWluUHRyIG9uIFdLUGFzdGVib2FyZE93 bmVyIGFuZCBXS1Bhc3RlYm9hcmRGaWxlUHJvbWlzZU93bmVyCisgICAgICAgIHNpbmNlIEFwcEtp dCBkb2VzIG5vdCBwZXJmb3JtIGEgcmV0YWluIG9uIHRob3NlIHBvaW50ZXJzLgorICAgICAgICAq IFdlYlByb2Nlc3MvV2ViUGFnZS9tYWMvV2ViUGFnZU1hYy5tbToKKyAgICAgICAgKFdlYktpdDo6 V2ViUGFnZTo6cGxhdGZvcm1EcmFnRW5kZWQpOiBFbnN1cmluZyBjaGFuZ2Ugb2YKKyAgICAgICAg cGFzdGVib2FyZCBvd25lcnNoaXAgb2NjdXJzIGJlZm9yZSByZWxlYXNpbmcgdGhlIHByZXZpb3Vz CisgICAgICAgIG93bmVyLgorCiAyMDExLTA0LTAxICBDaGFuZyBTaHUgIDxjc2h1QHdlYmtpdC5v cmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRGFyaW4gQWRsZXIuCkluZGV4OiBTb3VyY2UvV2Vi S2l0Mi9XZWJQcm9jZXNzL1dlYkNvcmVTdXBwb3J0L21hYy9XZWJEcmFnQ2xpZW50TWFjLm1tCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViQ29yZVN1cHBvcnQvbWFj L1dlYkRyYWdDbGllbnRNYWMubW0JKHJldmlzaW9uIDgyNjA1KQorKysgU291cmNlL1dlYktpdDIv V2ViUHJvY2Vzcy9XZWJDb3JlU3VwcG9ydC9tYWMvV2ViRHJhZ0NsaWVudE1hYy5tbQkod29ya2lu ZyBjb3B5KQpAQCAtMTQ3LDE2ICsxNDcsMTYgQEAgdm9pZCBXZWJEcmFnQ2xpZW50OjpkZWNsYXJl QW5kV3JpdGVEcmFnSQogICAgIFJldGFpblB0cjxOU011dGFibGVBcnJheT4gdHlwZXMoQWRvcHRO UywgW1tOU011dGFibGVBcnJheSBhbGxvY10gaW5pdFdpdGhPYmplY3RzOk5TRmlsZXNQcm9taXNl UGJvYXJkVHlwZSwgbmlsXSk7CiAgICAgW3R5cGVzLmdldCgpIGFkZE9iamVjdHNGcm9tQXJyYXk6 YXJjaGl2ZSA/IFBhc3RlYm9hcmRUeXBlczo6Zm9ySW1hZ2VzV2l0aEFyY2hpdmUoKSA6IFBhc3Rl Ym9hcmRUeXBlczo6Zm9ySW1hZ2VzKCldOwogCi0gICAgUmV0YWluUHRyPFdLUGFzdGVib2FyZE93 bmVyPiBwYXN0ZWJvYXJkT3duZXIoQWRvcHROUywgW1tXS1Bhc3RlYm9hcmRPd25lciBhbGxvY10g aW5pdFdpdGhJbWFnZTppbWFnZV0pOworICAgIFdLUGFzdGVib2FyZE93bmVyKiBwYXN0ZWJvYXJk T3duZXIgPSBbW1dLUGFzdGVib2FyZE93bmVyIGFsbG9jXSBpbml0V2l0aEltYWdlOmltYWdlXTsK IAotICAgIFJldGFpblB0cjxXS1Bhc3RlYm9hcmRGaWxlUHJvbWlzZU93bmVyPiBmaWxlUHJvbWlz ZU93bmVyKEFkb3B0TlMsIFsoV0tQYXN0ZWJvYXJkRmlsZVByb21pc2VPd25lciAqKVtXS1Bhc3Rl Ym9hcmRGaWxlUHJvbWlzZU93bmVyIGFsbG9jXSBpbml0V2l0aFNvdXJjZTpwYXN0ZWJvYXJkT3du ZXIuZ2V0KCldKTsKLSAgICBtX3BhZ2UtPnNldERyYWdTb3VyY2UoZmlsZVByb21pc2VPd25lci5n ZXQoKSk7CisgICAgV0tQYXN0ZWJvYXJkRmlsZVByb21pc2VPd25lciogZmlsZVByb21pc2VPd25l ciA9IFsoV0tQYXN0ZWJvYXJkRmlsZVByb21pc2VPd25lciopW1dLUGFzdGVib2FyZEZpbGVQcm9t aXNlT3duZXIgYWxsb2NdIGluaXRXaXRoU291cmNlOnBhc3RlYm9hcmRPd25lcl07CisgICAgbV9w YWdlLT5zZXREcmFnU291cmNlKGZpbGVQcm9taXNlT3duZXIpOwogCi0gICAgW3Bhc3RlYm9hcmQg ZGVjbGFyZVR5cGVzOnR5cGVzLmdldCgpIG93bmVyOnBhc3RlYm9hcmRPd25lci5nZXQoKV07ICAg IAorICAgIFtwYXN0ZWJvYXJkIGRlY2xhcmVUeXBlczp0eXBlcy5nZXQoKSBvd25lcjpwYXN0ZWJv YXJkT3duZXJdOyAgICAKIAogICAgIFtwYXN0ZWJvYXJkIHNldFByb3BlcnR5TGlzdDpbTlNBcnJh eSBhcnJheVdpdGhPYmplY3Q6ZXh0ZW5zaW9uXSBmb3JUeXBlOk5TRmlsZXNQcm9taXNlUGJvYXJk VHlwZV07CiAKLSAgICBbZmlsZVByb21pc2VPd25lci5nZXQoKSBzZXRUeXBlczpbcGFzdGVib2Fy ZCBwcm9wZXJ0eUxpc3RGb3JUeXBlOk5TRmlsZXNQcm9taXNlUGJvYXJkVHlwZV0gb25QYXN0ZWJv YXJkOnBhc3RlYm9hcmRdOworICAgIFtmaWxlUHJvbWlzZU93bmVyIHNldFR5cGVzOltwYXN0ZWJv YXJkIHByb3BlcnR5TGlzdEZvclR5cGU6TlNGaWxlc1Byb21pc2VQYm9hcmRUeXBlXSBvblBhc3Rl Ym9hcmQ6cGFzdGVib2FyZF07CiAKICAgICBbVVJMIHdyaXRlVG9QYXN0ZWJvYXJkOnBhc3RlYm9h cmRdOwogCkluZGV4OiBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvbWFjL1dlYlBh Z2VNYWMubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdl L21hYy9XZWJQYWdlTWFjLm1tCShyZXZpc2lvbiA4MjYwNSkKKysrIFNvdXJjZS9XZWJLaXQyL1dl YlByb2Nlc3MvV2ViUGFnZS9tYWMvV2ViUGFnZU1hYy5tbQkod29ya2luZyBjb3B5KQpAQCAtNTUx LDYgKzU1MSw5IEBAIHZvaWQgV2ViUGFnZTo6cGxhdGZvcm1EcmFnRW5kZWQoKQogICAgIC8vIFRo ZSBkcmFnIHNvdXJjZSB3ZSBjYXJlIGFib3V0IGhlcmUgaXMgTlNGaWxlUHJvbWlzZURyYWdTb3Vy Y2UsIHdoaWNoIGRvZXNuJ3QgbG9vayBhdAogICAgIC8vIHRoZSBhcmd1bWVudHMuIEl0J3MgT0sg dG8ganVzdCBwYXNzIGFyYml0cmFyeSBjb25zdGFudCB2YWx1ZXMsIHNvIHdlIGp1c3QgcGFzcyBh bGwgemVyb2VzLgogICAgIFttX2RyYWdTb3VyY2UuZ2V0KCkgZHJhZ2dlZEltYWdlOm5pbCBlbmRl ZEF0Ok5TWmVyb1BvaW50IG9wZXJhdGlvbjpOU0RyYWdPcGVyYXRpb25Ob25lXTsKKyAgICAvLyBX ZSBuZWVkIHRvIG1ha2Ugc3VyZSB0byByZXNldCB0aGUgcGFzdGVib2FyZCBvd25lciBiZWZvcmUg dGhlIFdLUGFzdGVib2FyZEZpbGVQcm9taXNlT3duZXIKKyAgICAvLyBvYmplY3QgZ2V0cyByZWxl YXNlZCwgc2luY2UgQXBwS2l0IHdpbGwgY2FsbCBwYXN0ZWJvYXJkQ2hhbmdlZE93bmVyIG9uIGl0 LgorICAgIFtbTlNQYXN0ZWJvYXJkIHBhc3RlYm9hcmRXaXRoTmFtZTpOU0RyYWdQYm9hcmRdIGRl Y2xhcmVUeXBlczpbTlNBcnJheSBhcnJheV0gb3duZXI6bmlsXTsKICAgICBtX2RyYWdTb3VyY2Ug PSBudWxscHRyOwogfQo=