Bug 126006

Summary: [SOUP] ResourceHandleSoup should use async client callbacks when client uses async callbacks
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cdumez, commit-queue, danw, gns, mrobinson, rakuco
Priority: P2 Keywords: Soup
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 126002    
Bug Blocks: 108832    
Attachments:
Description Flags
Patch mrobinson: review+

Description Carlos Garcia Campos 2013-12-19 12:02:39 PST
The client used by the network process uses async callbacks, we need to call those.
Comment 1 Carlos Garcia Campos 2013-12-19 12:10:41 PST
Created attachment 219671 [details]
Patch

Fixes the loader client unit tests when using the network process

Programs/WebKit2APITests/TestLoaderClient 
/webkit2/WebKitWebView/loading-status: OK
/webkit2/WebKitWebView/loading-error: OK
/webkit2/WebKitWebView/load-html: OK
/webkit2/WebKitWebView/load-alternate-html: OK
/webkit2/WebKitWebView/load-plain-text: OK
/webkit2/WebKitWebView/load-request: OK
/webkit2/WebKitWebView/stop-loading: OK
/webkit2/WebKitWebView/title: OK
/webkit2/WebKitWebView/progress: OK
/webkit2/WebKitWebView/reload: OK
/webkit2/WebKitWebView/history-load: OK
/webkit2/WebKitWebView/active-uri: OK
/webkit2/WebKitWebView/is-loading: OK
/webkit2/WebKitWebPage/get-uri: OK
/webkit2/WebKitURIRequest/http-headers: OK
Comment 2 Alberto Garcia 2013-12-19 12:26:00 PST
I had the impression that the mac port didn't have to do this. Did you check the reason?
Comment 3 Martin Robinson 2013-12-19 13:57:29 PST
Comment on attachment 219671 [details]
Patch

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

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:518
> -    d->client()->willSendRequest(handle, newRequest, d->m_response);
> +    if (d->client()->usesAsyncCallbacks())
> +        d->client()->willSendRequestAsync(handle, newRequest, d->m_response);
> +    else
> +        d->client()->willSendRequest(handle, newRequest, d->m_response);
>      handle->sendPendingRequest();

I think I don't totally understand the purpose of the asynchronous versions of these methods. We send the request immediately (as does the CFNet ResourceHandle) instead of waiting until the asynchronous callback. Do you have any insight into why these asynchronous versions exist?
Comment 4 Carlos Garcia Campos 2013-12-20 01:25:50 PST
(In reply to comment #2)
> I had the impression that the mac port didn't have to do this. Did you check the reason?

Mac does the same, see for example:

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/mac/ResourceHandleMac.mm
Comment 5 Carlos Garcia Campos 2013-12-20 01:26:41 PST
(In reply to comment #4)
> (In reply to comment #2)
> > I had the impression that the mac port didn't have to do this. Did you check the reason?
> 
> Mac does the same, see for example:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/mac/ResourceHandleMac.mm

I meant http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/mac/ResourceHandleMac.mm#L390
Comment 6 Carlos Garcia Campos 2013-12-20 01:34:44 PST
(In reply to comment #3)
> (From update of attachment 219671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219671&action=review
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:518
> > -    d->client()->willSendRequest(handle, newRequest, d->m_response);
> > +    if (d->client()->usesAsyncCallbacks())
> > +        d->client()->willSendRequestAsync(handle, newRequest, d->m_response);
> > +    else
> > +        d->client()->willSendRequest(handle, newRequest, d->m_response);
> >      handle->sendPendingRequest();
> 
> I think I don't totally understand the purpose of the asynchronous versions of these methods. We send the request immediately (as does the CFNet ResourceHandle) instead of waiting until the asynchronous callback. Do you have any insight into why these asynchronous versions exist?

This is because in the network process, the ResourceHandleClient (implemented by NetworkResourceLoader) only uses the async versions of the callbacks (usesAsyncCallbacks returns true). So, if the client uses async callbacks, calling, for example, didReceiveResponse has no effect, because it's not handled, the version didReceiveResponseAsync is the one implemented in NetworkResourceLoader. I think mac uses a different thread to implement the network callback asynchronously, but our implementation has always been async using the main loop instead. I think the only continueFoo that we need to implement is continueWillSendRequest, because the request might have changed (note that willSendRequest, the one that allows the user to change the request happens in the web process so it still works), in the other cases we don't need them. Note that loader client pass now, and also most of the resource load client test as well.
Comment 7 Martin Robinson 2013-12-20 06:58:55 PST
(In reply to comment #6)

> This is because in the network process, the ResourceHandleClient (implemented by NetworkResourceLoader) only uses the async versions of the callbacks (usesAsyncCallbacks returns true). So, if the client uses async callbacks, calling, for example, didReceiveResponse has no effect, because it's not handled, the version didReceiveResponseAsync is the one implemented in NetworkResourceLoader. I think mac uses a different thread to implement the network callback asynchronously, but our implementation has always been async using the main loop instead. I think the only continueFoo that we need to implement is continueWillSendRequest, because the request might have changed (note that willSendRequest, the one that allows the user to change the request happens in the web process so it still works), in the other cases we don't need them. Note that loader client pass now, and also most of the resource load client test as well.

What sort of thing would happen in continueWillSendRequest?
Comment 8 Carlos Garcia Campos 2013-12-20 08:31:09 PST
(In reply to comment #7)
> (In reply to comment #6)
> 
> > This is because in the network process, the ResourceHandleClient (implemented by NetworkResourceLoader) only uses the async versions of the callbacks (usesAsyncCallbacks returns true). So, if the client uses async callbacks, calling, for example, didReceiveResponse has no effect, because it's not handled, the version didReceiveResponseAsync is the one implemented in NetworkResourceLoader. I think mac uses a different thread to implement the network callback asynchronously, but our implementation has always been async using the main loop instead. I think the only continueFoo that we need to implement is continueWillSendRequest, because the request might have changed (note that willSendRequest, the one that allows the user to change the request happens in the web process so it still works), in the other cases we don't need them. Note that loader client pass now, and also most of the resource load client test as well.
> 
> What sort of thing would happen in continueWillSendRequest?

We would not call sendPendingRequest() in doRedirect and in continueWillSendRequest we would replace the current request by the passed in one, then we would call sendPendingRequest() to actually send the request. I left that part for a following up patch, because I haven't been able to find a test case that calls this willSendRequest in a redirect. This patch fixes a lot of problems we currently have, basically everything that depends on having a response in the UI process.
Comment 9 Martin Robinson 2013-12-20 11:15:33 PST
Comment on attachment 219671 [details]
Patch

Looks good, but as you suggested I think we should add FIXMEs to actually implement the asynchronous versions of these methods. It would also be good to open a bug for this and include the bug URL in the FIXME. That way this won't get lost in the cracks.
Comment 10 Carlos Garcia Campos 2013-12-21 00:47:55 PST
Committed r160961: <http://trac.webkit.org/changeset/160961>