Bug 126006 – [SOUP] ResourceHandleSoup should use async client callbacks when client uses async callbacks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

The client used by the network process uses async callbacks, we need to call those.

[Carlos Garcia Campos]

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

[Alberto Garcia]

I had the impression that the mac port didn't have to do this. Did you check the reason?

[Martin Robinson]

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?

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Martin Robinson]

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.

[Carlos Garcia Campos]

Committed r160961: <http://trac.webkit.org/changeset/160961>