Bug 74618 – [Qt][WK2] QWebDownloadItems are leaking

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

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.

[Jesus Sanchez-Palencia]

Created attachment 119457 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 119457 [details]
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.

[Jesus Sanchez-Palencia]

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

[Simon Hausmann]

Comment on attachment 119457 [details]
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;
}

[Jesus Sanchez-Palencia]

Created attachment 119920 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119920 [details]
Patch

Clearing flags on attachment: 119920

Committed r103278: <http://trac.webkit.org/changeset/103278>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.