Bug 111574 - [WK2][GTK] Invalid request returned by webkit_download_get_request if called before download starts
Summary: [WK2][GTK] Invalid request returned by webkit_download_get_request if called ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-06 06:32 PST by Manuel Rego Casasnovas (Holidays - back 1st Sep)
Modified: 2013-03-08 10:44 PST (History)
8 users (show)

See Also:


Attachments
Patch (11.78 KB, patch)
2013-03-07 00:35 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (12.57 KB, patch)
2013-03-07 08:42 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-03-06 06:32:37 PST
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.
Comment 1 Carlos Garcia Campos 2013-03-06 06:41:53 PST
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.
Comment 2 Carlos Garcia Campos 2013-03-06 06:53:48 PST
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
Comment 3 Carlos Garcia Campos 2013-03-07 00:35:45 PST
Created attachment 191938 [details]
Patch
Comment 4 WebKit Review Bot 2013-03-07 01:08:58 PST
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
Comment 5 Carlos Garcia Campos 2013-03-07 01:10:20 PST
Adding WebKit2 owners to CC
Comment 6 Benjamin Poulain 2013-03-07 01:23:32 PST
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?
Comment 7 Benjamin Poulain 2013-03-07 01:24:18 PST
I don't know enough about the domain to sign off on this. Maybe Alexey?
Comment 8 Carlos Garcia Campos 2013-03-07 01:26:51 PST
(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.
Comment 9 Carlos Garcia Campos 2013-03-07 08:42:38 PST
Created attachment 192009 [details]
Updated patch

Explained the issue and the solution in the changelog.
Comment 10 Alexey Proskuryakov 2013-03-07 09:48:29 PST
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?
Comment 11 Martin Robinson 2013-03-07 09:55:07 PST
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.
Comment 12 Carlos Garcia Campos 2013-03-07 23:29:31 PST
(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.
Comment 13 Martin Robinson 2013-03-08 10:14:16 PST
Comment on attachment 192009 [details]
Updated patch

Looks good to me! Nice cleanup in the unit tests too.
Comment 14 Carlos Garcia Campos 2013-03-08 10:38:44 PST
Comment on attachment 192009 [details]
Updated patch

Thanks for the review
Comment 15 WebKit Review Bot 2013-03-08 10:44:37 PST
Comment on attachment 192009 [details]
Updated patch

Clearing flags on attachment: 192009

Committed r145244: <http://trac.webkit.org/changeset/145244>
Comment 16 WebKit Review Bot 2013-03-08 10:44:42 PST
All reviewed patches have been landed.  Closing bug.