126006 2013-12-19 12:02:39 -0800 [SOUP] ResourceHandleSoup should use async client callbacks when client uses async callbacks 2013-12-21 00:47:55 -0800 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Soup P2 Normal --- 126002 108832 1 cgarcia webkit-unassigned berto cdumez commit-queue danw gustavo mrobinson rakuco oldest_to_newest 961487 0 cgarcia 2013-12-19 12:02:39 -0800 The client used by the network process uses async callbacks, we need to call those. 961490 1 219671 cgarcia 2013-12-19 12:10:41 -0800 Created attachment 219671 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 961501 2 berto 2013-12-19 12:26:00 -0800 I had the impression that the mac port didn't have to do this. Did you check the reason? 961534 3 219671 mrobinson 2013-12-19 13:57:29 -0800 Comment on attachment 219671 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? 961782 4 cgarcia 2013-12-20 01:25:50 -0800 (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 961783 5 cgarcia 2013-12-20 01:26:41 -0800 (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 961786 6 cgarcia 2013-12-20 01:34:44 -0800 (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. 961856 7 mrobinson 2013-12-20 06:58:55 -0800 (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? 961883 8 cgarcia 2013-12-20 08:31:09 -0800 (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. 961924 9 219671 mrobinson 2013-12-20 11:15:33 -0800 Comment on attachment 219671 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. 962218 10 cgarcia 2013-12-21 00:47:55 -0800 Committed r160961: <http://trac.webkit.org/changeset/160961> 219671 2013-12-19 12:10:41 -0800 2013-12-20 11:15:33 -0800 Patch soup-async-client.diff text/plain 4856 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA2ZGU5YmYwLi4yOTkzY2YwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjgg QEAKIDIwMTMtMTItMTkgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgogCisgICAgICAgIFtTT1VQXSBSZXNvdXJjZUhhbmRsZVNvdXAgc2hvdWxkIHVzZSBhc3luYyBj bGllbnQgY2FsbGJhY2tzIHdoZW4gY2xpZW50IHVzZXMgYXN5bmMgY2FsbGJhY2tzCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjYwMDYKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIGZpeGVzIFdlYktp dDIgbG9hZGVyIGNsaWVudCB1bml0IHRlc3RzIHdoZW4gdXNpbmcgdGhlIG5ldHdvcmsKKyAgICAg ICAgcHJvY2Vzcy4KKworICAgICAgICAqIHBsYXRmb3JtL25ldHdvcmsvUmVzb3VyY2VIYW5kbGUu Y3BwOgorICAgICAgICAqIHBsYXRmb3JtL25ldHdvcmsvc291cC9SZXNvdXJjZUhhbmRsZVNvdXAu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6ZG9SZWRpcmVjdCk6IENhbGwgd2lsbFNlbmRSZXF1ZXN0 QXN5bmMgb24gdGhlIGNsaWVudAorICAgICAgICB3aGVuIHVzZXNBc3luY0NhbGxiYWNrcyByZXR1 cm5zIHRydWUuCisgICAgICAgIChXZWJDb3JlOjpuZXh0TXVsdGlwYXJ0UmVzcG9uc2VQYXJ0Q2Fs bGJhY2spOiBDYWxsCisgICAgICAgIGRpZFJlY2VpdmVSZXNwb25zZUFzeW5jIG9uIHRoZSBjbGll bnQgd2hlbiB1c2VzQXN5bmNDYWxsYmFja3MKKyAgICAgICAgcmV0dXJucyB0cnVlLgorICAgICAg ICAoV2ViQ29yZTo6c2VuZFJlcXVlc3RDYWxsYmFjayk6IERpdHRvLgorICAgICAgICAoV2ViQ29y ZTo6UmVzb3VyY2VIYW5kbGU6OmNvbnRpbnVlV2lsbFNlbmRSZXF1ZXN0KTogRW1wdHkKKyAgICAg ICAgaW1wbGVtZW50YXRpb24gZm9yIG5vdyBiZWNhdXNlIHRoZSBkZWZhdWx0IG9uZSBhc3NlcnRz LgorICAgICAgICAoV2ViQ29yZTo6UmVzb3VyY2VIYW5kbGU6OmNvbnRpbnVlRGlkUmVjZWl2ZVJl c3BvbnNlKTogRGl0dG8uCisgICAgICAgIChXZWJDb3JlOjpSZXNvdXJjZUhhbmRsZTo6Y29udGlu dWVTaG91bGRVc2VDcmVkZW50aWFsU3RvcmFnZSk6IERpdHRvLgorCisyMDEzLTEyLTE5ICBDYXJs b3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlhLmNvbT4KKwogICAgICAgICBCdWlsZGZp eCBmb3IgTmV0d29ya1Jlc291cmNlTG9hZGVyOjpjb250aW51ZVdpbGxTZW5kUmVxdWVzdCgpIG9u ICFQTEFURk9STShNQUMpIHBsYXRmb3JtcwogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MTIxNDMwCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL25ldHdvcmsvUmVzb3VyY2VIYW5kbGUuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZv cm0vbmV0d29yay9SZXNvdXJjZUhhbmRsZS5jcHAKaW5kZXggY2E1MzFiMi4uMGM4NTJkYyAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9SZXNvdXJjZUhhbmRsZS5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9SZXNvdXJjZUhhbmRsZS5j cHAKQEAgLTE0Nyw3ICsxNDcsNyBAQCB2b2lkIFJlc291cmNlSGFuZGxlOjpzZXRDbGllbnQoUmVz b3VyY2VIYW5kbGVDbGllbnQqIGNsaWVudCkKICAgICBkLT5tX2NsaWVudCA9IGNsaWVudDsKIH0K IAotI2lmICFQTEFURk9STShNQUMpICYmICFVU0UoQ0ZORVRXT1JLKQorI2lmICFQTEFURk9STShN QUMpICYmICFVU0UoQ0ZORVRXT1JLKSAmJiAhVVNFKFNPVVApCiAvLyBSZXNvdXJjZUhhbmRsZSBu ZXZlciB1c2VzIGFzeW5jIGNsaWVudCBjYWxscyBvbiB0aGVzZSBwbGF0Zm9ybXMgeWV0Lgogdm9p ZCBSZXNvdXJjZUhhbmRsZTo6Y29udGludWVXaWxsU2VuZFJlcXVlc3QoY29uc3QgUmVzb3VyY2VS ZXF1ZXN0JikKIHsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsv c291cC9SZXNvdXJjZUhhbmRsZVNvdXAuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0 d29yay9zb3VwL1Jlc291cmNlSGFuZGxlU291cC5jcHAKaW5kZXggNjJlNTQyNi4uZTlmZTc3NiAx MDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNl SGFuZGxlU291cC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3Vw L1Jlc291cmNlSGFuZGxlU291cC5jcHAKQEAgLTUxMSw3ICs1MTEsMTAgQEAgc3RhdGljIHZvaWQg ZG9SZWRpcmVjdChSZXNvdXJjZUhhbmRsZSogaGFuZGxlKQogICAgIC8vIHRoZSBXZWJLaXQgbGF5 ZXIuIFRoZXkgd2VyZSBvbmx5IHBsYWNlZCBpbiB0aGUgVVJMIGZvciB0aGUgYmVuZWZpdCBvZiBs aWJzb3VwLgogICAgIG5ld1JlcXVlc3QucmVtb3ZlQ3JlZGVudGlhbHMoKTsKIAotICAgIGQtPmNs aWVudCgpLT53aWxsU2VuZFJlcXVlc3QoaGFuZGxlLCBuZXdSZXF1ZXN0LCBkLT5tX3Jlc3BvbnNl KTsKKyAgICBpZiAoZC0+Y2xpZW50KCktPnVzZXNBc3luY0NhbGxiYWNrcygpKQorICAgICAgICBk LT5jbGllbnQoKS0+d2lsbFNlbmRSZXF1ZXN0QXN5bmMoaGFuZGxlLCBuZXdSZXF1ZXN0LCBkLT5t X3Jlc3BvbnNlKTsKKyAgICBlbHNlCisgICAgICAgIGQtPmNsaWVudCgpLT53aWxsU2VuZFJlcXVl c3QoaGFuZGxlLCBuZXdSZXF1ZXN0LCBkLT5tX3Jlc3BvbnNlKTsKICAgICBoYW5kbGUtPnNlbmRQ ZW5kaW5nUmVxdWVzdCgpOwogfQogCkBAIC02NDksNyArNjUyLDEwIEBAIHN0YXRpYyB2b2lkIG5l eHRNdWx0aXBhcnRSZXNwb25zZVBhcnRDYWxsYmFjayhHT2JqZWN0KiAvKnNvdXJjZSovLCBHQXN5 bmNSZXN1bHQqCiAgICAgZC0+bV9yZXNwb25zZS5zZXRVUkwoaGFuZGxlLT5maXJzdFJlcXVlc3Qo KS51cmwoKSk7CiAgICAgZC0+bV9yZXNwb25zZS51cGRhdGVGcm9tU291cE1lc3NhZ2VIZWFkZXJz KHNvdXBfbXVsdGlwYXJ0X2lucHV0X3N0cmVhbV9nZXRfaGVhZGVycyhkLT5tX211bHRpcGFydElu cHV0U3RyZWFtLmdldCgpKSk7CiAKLSAgICBoYW5kbGUtPmNsaWVudCgpLT5kaWRSZWNlaXZlUmVz cG9uc2UoaGFuZGxlLmdldCgpLCBkLT5tX3Jlc3BvbnNlKTsKKyAgICBpZiAoaGFuZGxlLT5jbGll bnQoKS0+dXNlc0FzeW5jQ2FsbGJhY2tzKCkpCisgICAgICAgIGhhbmRsZS0+Y2xpZW50KCktPmRp ZFJlY2VpdmVSZXNwb25zZUFzeW5jKGhhbmRsZS5nZXQoKSwgZC0+bV9yZXNwb25zZSk7CisgICAg ZWxzZQorICAgICAgICBoYW5kbGUtPmNsaWVudCgpLT5kaWRSZWNlaXZlUmVzcG9uc2UoaGFuZGxl LmdldCgpLCBkLT5tX3Jlc3BvbnNlKTsKIAogICAgIGlmIChoYW5kbGUtPmNhbmNlbGxlZE9yQ2xp ZW50bGVzcygpKSB7CiAgICAgICAgIGNsZWFudXBTb3VwUmVxdWVzdE9wZXJhdGlvbihoYW5kbGUu Z2V0KCkpOwpAQCAtNzE2LDcgKzcyMiwxMCBAQCBzdGF0aWMgdm9pZCBzZW5kUmVxdWVzdENhbGxi YWNrKEdPYmplY3QqLCBHQXN5bmNSZXN1bHQqIHJlc3VsdCwgZ3BvaW50ZXIgZGF0YSkKICAgICAg ICAgZC0+bV9yZXNwb25zZS5zZXRFeHBlY3RlZENvbnRlbnRMZW5ndGgoc291cF9yZXF1ZXN0X2dl dF9jb250ZW50X2xlbmd0aChkLT5tX3NvdXBSZXF1ZXN0LmdldCgpKSk7CiAgICAgfQogCi0gICAg aGFuZGxlLT5jbGllbnQoKS0+ZGlkUmVjZWl2ZVJlc3BvbnNlKGhhbmRsZS5nZXQoKSwgZC0+bV9y ZXNwb25zZSk7CisgICAgaWYgKGQtPmNsaWVudCgpLT51c2VzQXN5bmNDYWxsYmFja3MoKSkKKyAg ICAgICAgaGFuZGxlLT5jbGllbnQoKS0+ZGlkUmVjZWl2ZVJlc3BvbnNlQXN5bmMoaGFuZGxlLmdl dCgpLCBkLT5tX3Jlc3BvbnNlKTsKKyAgICBlbHNlCisgICAgICAgIGhhbmRsZS0+Y2xpZW50KCkt PmRpZFJlY2VpdmVSZXNwb25zZShoYW5kbGUuZ2V0KCksIGQtPm1fcmVzcG9uc2UpOwogCiAgICAg aWYgKGhhbmRsZS0+Y2FuY2VsbGVkT3JDbGllbnRsZXNzKCkpIHsKICAgICAgICAgY2xlYW51cFNv dXBSZXF1ZXN0T3BlcmF0aW9uKGhhbmRsZS5nZXQoKSk7CkBAIC0xMzcwLDYgKzEzNzksMjQgQEAg c3RhdGljIHZvaWQgcmVhZENhbGxiYWNrKEdPYmplY3QqLCBHQXN5bmNSZXN1bHQqIGFzeW5jUmVz dWx0LCBncG9pbnRlciBkYXRhKQogICAgICAgICBkLT5tX2NhbmNlbGxhYmxlLmdldCgpLCByZWFk Q2FsbGJhY2ssIGhhbmRsZS5nZXQoKSk7CiB9CiAKK3ZvaWQgUmVzb3VyY2VIYW5kbGU6OmNvbnRp bnVlV2lsbFNlbmRSZXF1ZXN0KGNvbnN0IFJlc291cmNlUmVxdWVzdCYgcmVxdWVzdCkKK3sKKyAg ICBBU1NFUlQoY2xpZW50KCkpOworICAgIEFTU0VSVChjbGllbnQoKS0+dXNlc0FzeW5jQ2FsbGJh Y2tzKCkpOworfQorCit2b2lkIFJlc291cmNlSGFuZGxlOjpjb250aW51ZURpZFJlY2VpdmVSZXNw b25zZSgpCit7CisgICAgQVNTRVJUKGNsaWVudCgpKTsKKyAgICBBU1NFUlQoY2xpZW50KCktPnVz ZXNBc3luY0NhbGxiYWNrcygpKTsKK30KKwordm9pZCBSZXNvdXJjZUhhbmRsZTo6Y29udGludWVT aG91bGRVc2VDcmVkZW50aWFsU3RvcmFnZShib29sKQoreworICAgIEFTU0VSVChjbGllbnQoKSk7 CisgICAgQVNTRVJUKGNsaWVudCgpLT51c2VzQXN5bmNDYWxsYmFja3MoKSk7Cit9CisKIHN0YXRp YyBnYm9vbGVhbiByZXF1ZXN0VGltZW91dENhbGxiYWNrKGdwb2ludGVyIGRhdGEpCiB7CiAgICAg UmVmUHRyPFJlc291cmNlSGFuZGxlPiBoYW5kbGUgPSBzdGF0aWNfY2FzdDxSZXNvdXJjZUhhbmRs ZSo+KGRhdGEpOwo=