Bug 111574 – [WK2][GTK] Invalid request returned by webkit_download_get_request if called before download starts

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

The following code will return an invalid request:

    WebKitDownload* download = webkit_web_context_download_uri(webkit_web_context_get_default(), "http://www.google.com/");
    WebKitURIRequest* request = webkit_download_get_request(download);
    // Request is invalid here, for example next method will return an empty string
    webkit_uri_request_get_uri(request); // Empty instead of "http://www.google.com/"

However, if you wait till download stats, the request is valid and the method webkit_uri_request_get_uri() works properly.

Taking a look to the related code:

* WebKitDownload.cpp:

WebKitURIRequest* webkit_download_get_request(WebKitDownload* download)
{
    g_return_val_if_fail(WEBKIT_IS_DOWNLOAD(download), 0);

    WebKitDownloadPrivate* priv = download->priv;
    if (!priv->request)
        priv->request = adoptGRef(webkitURIRequestCreateForResourceRequest(priv->download->request()));
    return priv->request.get();
}

* DownloadProxy.h:

    const WebCore::ResourceRequest& request() const { return m_request; }

* DownloadProxy.cpp:

void DownloadProxy::didStart(const ResourceRequest& request)
{
    m_request = request;
    
    if (!m_webContext)
        return;
        
    m_webContext->downloadClient().didStart(m_webContext.get(), this);
}

In WebKitDownload priv->download is a DownloadProxy. So, if it's called before DownloadProxy::didStart the request is not initialized yet.

There's even a bigger problem in webkit_download_get_request, as it's caching the request in priv->request. If you call webkit_download_get_request before didStart (like in the example above) and you call it again after didStart, it's going to return always an invalid request.

[Carlos Garcia Campos]

The problem only happens for downloads started manually with webkit_web_context_download or webkit_web_view_download, because for downloads started implicitly, you get the download object from the download-started signal. 
So, I think that instead of creating the uri request on demand, we should set it from download-started and document that until download-started is emitted, the request will be NULL. 
Or for the cases where the download is started manually, we can create a first request for the requested URI and overwrite it with the download proxy request when download-started is emitted. This will ensure that webkit_download_get_request will always return a valid object, but it might be weird that the request changes.

[Carlos Garcia Campos]

In case of a download started manually, the request used by the web process is the one sent from the ui process, so we can just use that always. I'll make a patch

[Carlos Garcia Campos]

Created attachment 191938 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Adding WebKit2 owners to CC

[Benjamin Poulain]

Comment on attachment 191938 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        Create download objects with a request URI for downloads started
> +        manually.

Your ChangeLog should explain why the bug happened and why this fixes this.

It is good for ChangeLog to describe both the Why? and the How?

[Benjamin Poulain]

I don't know enough about the domain to sign off on this. Maybe Alexey?

[Carlos Garcia Campos]

(In reply to comment #6)
> (From update of attachment 191938 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191938&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        Create download objects with a request URI for downloads started
> > +        manually.
> 
> Your ChangeLog should explain why the bug happened and why this fixes this.
> 
> It is good for ChangeLog to describe both the Why? and the How?

You are right, I wrote this too fast, I'll explain the issue and how it's solved.

[Carlos Garcia Campos]

Created attachment 192009 [details]
Updated patch

Explained the issue and the solution in the changelog.

[Alexey Proskuryakov]

Comment on attachment 192009 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitDownloadPrivate.h:30
> +WebKitDownload* webkitDownloadCreateForRequest(WebKit::DownloadProxy*, const WebCore::ResourceRequest&);

Exposing WebCore types in WebKit API seems surprising. Is this something the Gtk port routinely does?

[Martin Robinson]

Comment on attachment 192009 [details]
Updated patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitDownloadPrivate.h:30
>> +WebKitDownload* webkitDownloadCreateForRequest(WebKit::DownloadProxy*, const WebCore::ResourceRequest&);
> 
> Exposing WebCore types in WebKit API seems surprising. Is this something the Gtk port routinely does?

In contrast to the Mac port private headers, we do not ship ours or make the symbols in them visible in the shared object.

[Carlos Garcia Campos]

(In reply to comment #10)
> (From update of attachment 192009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192009&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitDownloadPrivate.h:30
> > +WebKitDownload* webkitDownloadCreateForRequest(WebKit::DownloadProxy*, const WebCore::ResourceRequest&);
> 
> Exposing WebCore types in WebKit API seems surprising. Is this something the Gtk port routinely does?

This is a private header, it's not installed and the symbols are not exported.

[Martin Robinson]

Comment on attachment 192009 [details]
Updated patch

Looks good to me! Nice cleanup in the unit tests too.

[Carlos Garcia Campos]

Comment on attachment 192009 [details]
Updated patch

Thanks for the review

[WebKit Review Bot]

Comment on attachment 192009 [details]
Updated patch

Clearing flags on attachment: 192009

Committed r145244: <http://trac.webkit.org/changeset/145244>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.