74618 2011-12-15 09:57:09 -0800 [Qt][WK2] QWebDownloadItems are leaking 2011-12-19 16:13:00 -0800 1 1 1 Unclassified WebKit WebKit Qt 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Qt, QtTriaged P2 Normal --- 1 jesus jesus hausmann webkit.review.bot zoltan oldest_to_newest 521990 0 jesus 2011-12-15 09:57:09 -0800 QWebDownloadItems are leaking when WebProcess raises a download failure before sending didReceiveResponse back to UIProcess. This can happen when QtFileDownloader fails in determineFilename(), for instance. 522035 1 119457 jesus 2011-12-15 10:38:42 -0800 Created attachment 119457 Patch 522167 2 119457 kenneth 2011-12-15 12:57:21 -0800 Comment on attachment 119457 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119457&action=review Wouldn't it be easier to ref the download item instead and then transfer ownership when parenting it? > Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:98 > + // If this function is ever reached with an "empty" downloadItem > + // then it means WebProcess raised a download failure even before > + // we reached downloadReceivedResponse(). At this point > + // QQuickWebView::downloadRequested() signal hasn't been emitted and > + // the downloadItem will have null parent. Therefore, it will leak > + // unless we delete it. This could be more compact // If the url is empty at this point, the download failed before it received a response and downloadRequested // was emitted. Due to this the item will not be parented and we have to delete it manually at this point. 522745 3 jesus 2011-12-16 07:19:48 -0800 (In reply to comment #2) > Wouldn't it be easier to ref the download item instead and then transfer ownership when parenting it? But then we might end up with a bunch of (failed) download items during the entire life cycle of whoever is keeping the RefPtr, right? Those objects are useless since they never got to the "client" (the browser, let's say) and will just be kept in memory for no reason. I would rather delete them straight away. Plus, if we ever decide do to expose setDownloadEnabled API (https://bugs.webkit.org/show_bug.cgi?id=73318) we will have a similar situation in which we will need to delete downloadItems as well. > This could be more compact > > // If the url is empty at this point, the download failed before it received a response and downloadRequested > // was emitted. Due to this the item will not be parented and we have to delete it manually at this point. Yes, sure, thanks! I will update the patch later as soon as we reach a conclusion about deleting the the download items or to ref them. 523872 4 119457 hausmann 2011-12-19 06:02:58 -0800 Comment on attachment 119457 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119457&action=review > Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:103 > + According to your comment the item will have a null parent, so wouldn't it be easier and more readable to simply write: if (!downloadItem->parent()) { delete downloadItem; return; } 524199 5 119920 jesus 2011-12-19 14:43:29 -0800 Created attachment 119920 Patch 524255 6 119920 webkit.review.bot 2011-12-19 16:12:56 -0800 Comment on attachment 119920 Patch Clearing flags on attachment: 119920 Committed r103278: <http://trac.webkit.org/changeset/103278> 524256 7 webkit.review.bot 2011-12-19 16:13:00 -0800 All reviewed patches have been landed. Closing bug. 119457 2011-12-15 10:38:42 -0800 2011-12-19 14:43:19 -0800 Patch bug-74618-20111215153828.patch text/plain 2489 jesus U3VidmVyc2lvbiBSZXZpc2lvbjogMTAyOTUzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggZjFhZTdjY2JkNDVmNTVl YjZhOWI4YmU3YjM3MDVkYWRlOTlhNmRkNC4uNmJhODVlNzFmYzM0YWJhNmE4MDQ1NzM4OGJkODVm YjliYTE2NDYzYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDExLTEyLTE1ICBKZXN1 cyBTYW5jaGV6LVBhbGVuY2lhICA8amVzdXMucGFsZW5jaWFAb3BlbmJvc3NhLm9yZz4KKworICAg ICAgICBbUXRdW1dLMl0gUVdlYkRvd25sb2FkSXRlbXMgYXJlIGxlYWtpbmcKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTc0NjE4CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUVdlYkRvd25sb2FkSXRlbXMgYXJl IGxlYWtpbmcgd2hlbiBXZWJQcm9jZXNzIHJhaXNlcyBhCisgICAgICAgIGRvd25sb2FkIGZhaWx1 cmUgYmVmb3JlIHNlbmRpbmcgZGlkUmVjZWl2ZVJlc3BvbnNlIGJhY2sgdG8gVUlQcm9jZXNzLgor ICAgICAgICBUaGlzIGNhbiBoYXBwZW4gd2hlbiBRdEZpbGVEb3dubG9hZGVyIGZhaWxzIGluIGRl dGVybWluZUZpbGVuYW1lKCksIGZvciBpbnN0YW5jZS4KKyAgICAgICAgSXQgaGFwcGVucyB3aGVu IFF0RG93bmxvYWRNYW5hZ2VyOjpkb3dubG9hZEZhaWxlZCgpIGlzIHJlYWNoZWQKKyAgICAgICAg YW5kIGhhcyBhbiAiZW1wdHkiIGRvd25sb2FkSXRlbSwgd2hpY2ggbWVhbnMgd2UgbmV2ZXIgZ290 IHRvCisgICAgICAgIFF0RG93bmxvYWRNYW5hZ2VyOjpkb3dubG9hZFJlY2VpdmVkUmVzcG9uc2Uo KS4gQXQgdGhpcyBwb2ludAorICAgICAgICBRUXVpY2tXZWJWaWV3Ojpkb3dubG9hZFJlcXVlc3Rl ZCgpIHNpZ25hbCBoYXNuJ3QgYmVlbiBlbWl0dGVkIGFuZAorICAgICAgICB0aGUgZG93bmxvYWRJ dGVtIHdpbGwgaGF2ZSBudWxsIHBhcmVudC4gVGhlcmVmb3JlLCBpdCB3aWxsIGxlYWsKKyAgICAg ICAgdW5sZXNzIHdlIGRlbGV0ZSBpdCBvdXJzZWx2ZXMuCisKKyAgICAgICAgKiBVSVByb2Nlc3Mv cXQvUXREb3dubG9hZE1hbmFnZXIuY3BwOgorICAgICAgICAoV2ViS2l0OjpRdERvd25sb2FkTWFu YWdlcjo6ZG93bmxvYWRGYWlsZWQpOgorCiAyMDExLTEyLTE0ICBBbmRlcnMgQ2FybHNzb24gIDxh bmRlcnNjYUBhcHBsZS5jb20+CiAKICAgICAgICAgQWRkIFdURjo6RnVuY3Rpb24gdG8gd3RmL0Zv cndhcmQuaApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL3F0L1F0RG93bmxv YWRNYW5hZ2VyLmNwcCBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9xdC9RdERvd25sb2FkTWFu YWdlci5jcHAKaW5kZXggNjg4M2NjNDI5MjNlZjVlZmU1MThlZTFmNzExNjc3YzMyYzQzM2UxOC4u ODU2NjdiYTg0Mzg4NTNhNTA0YjEwZTBlNWMzYmE3M2FjZGJkZjUyZCAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYktpdDIvVUlQcm9jZXNzL3F0L1F0RG93bmxvYWRNYW5hZ2VyLmNwcAorKysgYi9Tb3Vy Y2UvV2ViS2l0Mi9VSVByb2Nlc3MvcXQvUXREb3dubG9hZE1hbmFnZXIuY3BwCkBAIC04OSw2ICs4 OSwxOCBAQCB2b2lkIFF0RG93bmxvYWRNYW5hZ2VyOjpkb3dubG9hZEZhaWxlZChEb3dubG9hZFBy b3h5KiBkb3dubG9hZCwgY29uc3QgUXRXZWJFcnJvcgogICAgIC8vIFdpbGwgYmUgY2FsbGVkIHdo ZW4gZG93bmxvYWQgZmFpbHMgb3IgaXMgYWJvcnRlZC4KICAgICBRV2ViRG93bmxvYWRJdGVtKiBk b3dubG9hZEl0ZW0gPSBtX2Rvd25sb2Fkcy50YWtlKGRvd25sb2FkLT5kb3dubG9hZElEKCkpOwog ICAgIEFTU0VSVChkb3dubG9hZEl0ZW0pOworCisgICAgLy8gSWYgdGhpcyBmdW5jdGlvbiBpcyBl dmVyIHJlYWNoZWQgd2l0aCBhbiAiZW1wdHkiIGRvd25sb2FkSXRlbQorICAgIC8vIHRoZW4gaXQg bWVhbnMgV2ViUHJvY2VzcyByYWlzZWQgYSBkb3dubG9hZCBmYWlsdXJlIGV2ZW4gYmVmb3JlCisg ICAgLy8gd2UgcmVhY2hlZCBkb3dubG9hZFJlY2VpdmVkUmVzcG9uc2UoKS4gQXQgdGhpcyBwb2lu dAorICAgIC8vIFFRdWlja1dlYlZpZXc6OmRvd25sb2FkUmVxdWVzdGVkKCkgc2lnbmFsIGhhc24n dCBiZWVuIGVtaXR0ZWQgYW5kCisgICAgLy8gdGhlIGRvd25sb2FkSXRlbSB3aWxsIGhhdmUgbnVs bCBwYXJlbnQuIFRoZXJlZm9yZSwgaXQgd2lsbCBsZWFrCisgICAgLy8gdW5sZXNzIHdlIGRlbGV0 ZSBpdC4KKyAgICBpZiAoZG93bmxvYWRJdGVtLT51cmwoKS5pc0VtcHR5KCkpIHsKKyAgICAgICAg ZGVsZXRlIGRvd25sb2FkSXRlbTsKKyAgICAgICAgcmV0dXJuOworICAgIH0KKwogICAgIGVtaXQg ZG93bmxvYWRJdGVtLT5mYWlsZWQoZXJyb3IuZXJyb3JDb2RlQXNEb3dubG9hZEVycm9yKCksIGVy cm9yLnVybCgpLCBlcnJvci5kZXNjcmlwdGlvbigpKTsKIH0KIAo= 119920 2011-12-19 14:43:29 -0800 2011-12-19 16:12:56 -0800 Patch bug-74618-20111219194317.patch text/plain 2348 jesus U3VidmVyc2lvbiBSZXZpc2lvbjogMTAzMjY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMTdjMTZjNzk2ZjNkYzg1 YTA0NzgzNzdhZDE0MzY0MTA5NDZlMTc0Ni4uODQ0NjIwYzcxNzcxODhjMmMyNzRmMDBmNDY3ZjJk YThhMWQ4NTg5NyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDExLTEyLTE5ICBKZXN1 cyBTYW5jaGV6LVBhbGVuY2lhICA8amVzdXMucGFsZW5jaWFAb3BlbmJvc3NhLm9yZz4KKworICAg ICAgICBbUXRdW1dLMl0gUVdlYkRvd25sb2FkSXRlbXMgYXJlIGxlYWtpbmcKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTc0NjE4CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUVdlYkRvd25sb2FkSXRlbXMgYXJl IGxlYWtpbmcgd2hlbiBXZWJQcm9jZXNzIHJhaXNlcyBhCisgICAgICAgIGRvd25sb2FkIGZhaWx1 cmUgYmVmb3JlIHNlbmRpbmcgZGlkUmVjZWl2ZVJlc3BvbnNlIGJhY2sgdG8gVUlQcm9jZXNzLgor ICAgICAgICBUaGlzIGNhbiBoYXBwZW4gd2hlbiBRdEZpbGVEb3dubG9hZGVyIGZhaWxzIGluIGRl dGVybWluZUZpbGVuYW1lKCksIGZvciBpbnN0YW5jZS4KKyAgICAgICAgSXQgaGFwcGVucyB3aGVu IFF0RG93bmxvYWRNYW5hZ2VyOjpkb3dubG9hZEZhaWxlZCgpIGlzIHJlYWNoZWQKKyAgICAgICAg YW5kIGhhcyBhbiAiZW1wdHkiIGRvd25sb2FkSXRlbSwgd2hpY2ggbWVhbnMgd2UgbmV2ZXIgZ290 IHRvCisgICAgICAgIFF0RG93bmxvYWRNYW5hZ2VyOjpkb3dubG9hZFJlY2VpdmVkUmVzcG9uc2Uo KS4gQXQgdGhpcyBwb2ludAorICAgICAgICBRUXVpY2tXZWJWaWV3Ojpkb3dubG9hZFJlcXVlc3Rl ZCgpIHNpZ25hbCBoYXNuJ3QgYmVlbiBlbWl0dGVkIGFuZAorICAgICAgICB0aGUgZG93bmxvYWRJ dGVtIHdpbGwgaGF2ZSBudWxsIHBhcmVudC4gVGhlcmVmb3JlLCBpdCB3aWxsIGxlYWsKKyAgICAg ICAgdW5sZXNzIHdlIGRlbGV0ZSBpdCBvdXJzZWx2ZXMuCisKKyAgICAgICAgKiBVSVByb2Nlc3Mv cXQvUXREb3dubG9hZE1hbmFnZXIuY3BwOgorICAgICAgICAoV2ViS2l0OjpRdERvd25sb2FkTWFu YWdlcjo6ZG93bmxvYWRGYWlsZWQpOgorCiAyMDExLTEyLTE5ICBFcmljIENhcmxzb24gIDxlcmlj LmNhcmxzb25AYXBwbGUuY29tPgogCiAgICAgICAgIEVuYWJsZSA8dHJhY2s+IGZvciBNYWMgYnVp bGQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9xdC9RdERvd25sb2FkTWFu YWdlci5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvcXQvUXREb3dubG9hZE1hbmFnZXIu Y3BwCmluZGV4IDY4ODNjYzQyOTIzZWY1ZWZlNTE4ZWUxZjcxMTY3N2MzMmM0MzNlMTguLjk5YzRk YzU4ZThmY2FkNmQ4YjJhM2QzYzU2YTMyNGNlMTczNTk1YmIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJLaXQyL1VJUHJvY2Vzcy9xdC9RdERvd25sb2FkTWFuYWdlci5jcHAKKysrIGIvU291cmNlL1dl YktpdDIvVUlQcm9jZXNzL3F0L1F0RG93bmxvYWRNYW5hZ2VyLmNwcApAQCAtODksNiArODksMTYg QEAgdm9pZCBRdERvd25sb2FkTWFuYWdlcjo6ZG93bmxvYWRGYWlsZWQoRG93bmxvYWRQcm94eSog ZG93bmxvYWQsIGNvbnN0IFF0V2ViRXJyb3IKICAgICAvLyBXaWxsIGJlIGNhbGxlZCB3aGVuIGRv d25sb2FkIGZhaWxzIG9yIGlzIGFib3J0ZWQuCiAgICAgUVdlYkRvd25sb2FkSXRlbSogZG93bmxv YWRJdGVtID0gbV9kb3dubG9hZHMudGFrZShkb3dubG9hZC0+ZG93bmxvYWRJRCgpKTsKICAgICBB U1NFUlQoZG93bmxvYWRJdGVtKTsKKworICAgIC8vIElmIHRoZSBwYXJlbnQgaXMgbnVsbCBhdCB0 aGlzIHBvaW50LCB0aGUgZG93bmxvYWQgZmFpbGVkIGJlZm9yZSBpdAorICAgIC8vIHJlY2VpdmVk IGEgcmVzcG9uc2UgYW5kIGRvd25sb2FkUmVxdWVzdGVkIHdhcyBlbWl0dGVkLgorICAgIC8vIER1 ZSB0byB0aGlzIHRoZSBpdGVtIHdpbGwgbmV2ZXIgYmUgcGFyZW50ZWQgYW5kIHdlIGhhdmUgdG8g ZGVsZXRlIGl0CisgICAgLy8gbWFudWFsbHkgYXQgdGhpcyBwb2ludC4KKyAgICBpZiAoIWRvd25s b2FkSXRlbS0+cGFyZW50KCkpIHsKKyAgICAgICAgZGVsZXRlIGRvd25sb2FkSXRlbTsKKyAgICAg ICAgcmV0dXJuOworICAgIH0KKwogICAgIGVtaXQgZG93bmxvYWRJdGVtLT5mYWlsZWQoZXJyb3Iu ZXJyb3JDb2RlQXNEb3dubG9hZEVycm9yKCksIGVycm9yLnVybCgpLCBlcnJvci5kZXNjcmlwdGlv bigpKTsKIH0KIAo=