121430 2013-09-16 07:04:19 -0700 Buildfix for NetworkResourceLoader::continueWillSendRequest() on !PLATFORM(MAC) platforms 2014-12-03 07:58:15 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED DUPLICATE 125422 P2 Normal --- 110141 1 ossy ossy ap beidson brian.holt cgarcia darin kbalazs mrobinson ossy skyul oldest_to_newest 929688 0 ossy 2013-09-16 07:04:19 -0700 It is a part of https://bugs.webkit.org/show_bug.cgi?id=110141 The proposed change was the following: diff --git a/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp b/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp index ac3008b6911d6840122b2c64b4d122f6791d0aa0..de2fc839577156eb312e339c5294144e0e118b78 100644 --- a/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp +++ b/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp @@ -214,7 +214,11 @@ void NetworkResourceLoader::willSendRequestAsync(ResourceHandle* handle, const R void NetworkResourceLoader::continueWillSendRequest(const ResourceRequest& newRequest) { +#if PLATFORM(MAC) m_suggestedRequestForWillSendRequest.updateFromDelegatePreservingOldHTTPBody(newRequest.nsURLRequest(DoNotUpdateHTTPBody)); +#else + m_suggestedRequestForWillSendRequest.updateFromDelegatePreservingOldHTTPBody(newRequest); +#endif RunLoop::main()->dispatch(bind(&NetworkResourceLoadScheduler::receivedRedirect, &NetworkProcess::shared().networkResourceLoadScheduler(), this, m_suggestedRequestForWillSendRequest.url())); m_handle->continueWillSendRequest(m_suggestedRequestForWillSendRequest); comments: ---------- https://bugs.webkit.org/show_bug.cgi?id=110141#c11 Brady Eidson: "I wish we could find a sensible way to avoid this." https://bugs.webkit.org/show_bug.cgi?id=110141#c17 Kwang Yul Seo: "I am not sure how to avoid this as only the Mac port has a platform specific URL request object (m_nsRequest) and doUpdatePlatformRequest, doUpdateResourceRequest, doUpdatePlatformHTTPBody and doUpdateResourceHTTPBody do nothing in soup. Any suggestion?" 929692 1 211780 ossy 2013-09-16 07:08:23 -0700 Created attachment 211780 Patch It is the originally proposed fix, because there wasn't a better suggestion. 930225 2 ossy 2013-09-17 08:18:14 -0700 Could you suggest any better fix for it or is it OK after the comment of Kwang Yul Seo? 930300 3 darin 2013-09-17 10:48:24 -0700 It seems obvious that the issue here is that there is not a function that does what newRequest.nsURLRequest(DoNotUpdateHTTPBody) does without involving a platform-specific request object. Putting in an #ifdef is just a way to avoid doing that. I understand why this code doesn’t compile, but I don’t understand why there is no need to do the operation on non-Mac platforms. I haven’t had time to look more deeply myself and find the answer. 930619 4 ossy 2013-09-18 07:58:30 -0700 This thing was introduced in http://trac.webkit.org/changeset/144216. I'm going to check this change. 957844 5 brian.holt 2013-12-09 03:56:13 -0800 (In reply to comment #3) > It seems obvious that the issue here is that there is not a function that does what newRequest.nsURLRequest(DoNotUpdateHTTPBody) does without involving a platform-specific request object. Putting in an #ifdef is just a way to avoid doing that. I understand why this code doesn’t compile, but I don’t understand why there is no need to do the operation on non-Mac platforms. > > I haven’t had time to look more deeply myself and find the answer. It turns out we don't even need m_suggestedRequestForWillSendRequest.updateFromDelegatePreservingOldHTTPBody(newRequest); for the Network Process to work on GTK, and the patch in https://bugs.webkit.org/show_bug.cgi?id=125422 is sufficient (I didn't know about this bug when I filed and uploaded that patch). What is still unclear is why we don't need this, but Mac does. 957871 6 mrobinson 2013-12-09 06:21:00 -0800 Alexey, do you mind giving some insight into this particular line of code? 957939 7 ap 2013-12-09 09:46:43 -0800 HTTP request body is not sent over IPC, for two reasons: - it's impossible serialize when it's a stream, - it can be huge, and clients don't need it anyway for policy decisions. So I think that all ports need this. 961357 8 219642 cgarcia 2013-12-19 05:55:03 -0800 Created attachment 219642 Another patch I wonder if we can avoid the platform ifdef entirely by moving the NSURLRequest to the platform specific file. 961481 9 219642 ap 2013-12-19 11:54:42 -0800 Comment on attachment 219642 Another patch View in context: https://bugs.webkit.org/attachment.cgi?id=219642&action=review > Source/WebCore/platform/network/mac/ResourceRequestMac.mm:229 > - *this = delegateProvidedRequest; > + *this = delegateProvidedRequest.nsURLRequest(DoNotUpdateHTTPBody); This looks like a much slower way to do the same, as we'll need to move all fields into NSURLRequest first, and then back into WebCore types. 961484 10 cgarcia 2013-12-19 11:58:37 -0800 (In reply to comment #9) > (From update of attachment 219642 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=219642&action=review > > > Source/WebCore/platform/network/mac/ResourceRequestMac.mm:229 > > - *this = delegateProvidedRequest; > > + *this = delegateProvidedRequest.nsURLRequest(DoNotUpdateHTTPBody); > > This looks like a much slower way to do the same, as we'll need to move all fields into NSURLRequest first, and then back into WebCore types. Right, but it's already done by the caller anyway no? 961486 11 ap 2013-12-19 12:02:27 -0800 Not sure, are you saying that delegateProvidedRequest only has an NSURLRequest in it, and platform request fields are not updated? 961494 12 cgarcia 2013-12-19 12:16:39 -0800 (In reply to comment #11) > Not sure, are you saying that delegateProvidedRequest only has an NSURLRequest in it, and platform request fields are not updated? What I mean is that the caller already creates a ResourceRequest from a NSURLRequest, so I guess it's equivalent, I'm just moving the request.nsURLRequest(DoNotUpdateHTTPBody) from the caller to the implementation, or am I missing something? m_suggestedRequestForWillSendRequest.updateFromDelegatePreservingOldHTTPBody(newRequest.nsURLRequest(DoNotUpdateHTTPBody)); 961504 13 ap 2013-12-19 12:34:24 -0800 There are two callers of updateFromDelegatePreservingOldHTTPBody(). The other one is in Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageResourceLoadClient.cpp, and it passes an existing ResourceRequest, not one created from NSURLRequest just for this purpose. It seems that this patch adds another round-trip in this case. 961791 14 cgarcia 2013-12-20 01:41:19 -0800 (In reply to comment #13) > There are two callers of updateFromDelegatePreservingOldHTTPBody(). The other one is in Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageResourceLoadClient.cpp, and it passes an existing ResourceRequest, not one created from NSURLRequest just for this purpose. > > It seems that this patch adds another round-trip in this case. Oh, fair enough, I missed the other caller. Thanks for clarifying. 1022026 15 219642 darin 2014-07-12 17:01:17 -0700 Comment on attachment 219642 Another patch Issue might still exist, but this has changed quite a bit since December, and the patch as-is won’t be needed. 1022027 16 211780 darin 2014-07-12 17:01:48 -0700 Comment on attachment 211780 Patch Looks like we landed something like this a long time ago. 1052158 17 ossy 2014-12-03 07:58:15 -0800 *** This bug has been marked as a duplicate of bug 125422 *** 211780 2013-09-16 07:08:23 -0700 2014-07-12 17:01:48 -0700 Patch bug-121430-20130916160822.patch text/plain 1871 ossy U3VidmVyc2lvbiBSZXZpc2lvbjogMTU1ODQ1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNDI5NDljOTBhNjU5ZjI3 ZDRkNjNkYzZjNzVkYTRkMGZhYTkwYWFiNi4uZjRjNmI4ZjQ2M2IzMmVjZGI3ZDIzNTU1ZWI2NTIy ZDhkNmQzOWRkNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEzLTA5LTE2ICBDc2Fi YSBPc3p0cm9nb27DoWMgIDxvc3N5QHdlYmtpdC5vcmc+CisKKyAgICAgICAgQnVpbGRmaXggZm9y IE5ldHdvcmtSZXNvdXJjZUxvYWRlcjo6Y29udGludWVXaWxsU2VuZFJlcXVlc3QoKSBvbiAhUExB VEZPUk0oTUFDKSBwbGF0Zm9ybXMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTEyMTQzMAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIE9yaWdpbmFsIHBhdGNoIGJ5IEt3YW5nIFl1bCBTZW8gPHNreXVsQGNvbXBh bnkxMDAuY29tPi4KKworICAgICAgICAqIE5ldHdvcmtQcm9jZXNzL05ldHdvcmtSZXNvdXJjZUxv YWRlci5jcHA6CisgICAgICAgIChXZWJLaXQ6Ok5ldHdvcmtSZXNvdXJjZUxvYWRlcjo6Y29udGlu dWVXaWxsU2VuZFJlcXVlc3QpOgorICAgICAgICBEb24ndCB1c2UgbmV3UmVxdWVzdC5uc1VSTFJl cXVlc3RhIGFzIGl0IGlzIE1hYyBwb3J0IHNwZWNpZmljLgorCiAyMDEzLTA5LTE1ICBHdXN0YXZv IE5vcm9uaGEgU2lsdmEgIDxnbnNAZ25vbWUub3JnPgogCiAgICAgICAgIFVucmV2aWV3ZWQgbWFr ZSBkaXN0Y2hlY2sgZml4LgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvTmV0d29ya1Byb2Nl c3MvTmV0d29ya1Jlc291cmNlTG9hZGVyLmNwcCBiL1NvdXJjZS9XZWJLaXQyL05ldHdvcmtQcm9j ZXNzL05ldHdvcmtSZXNvdXJjZUxvYWRlci5jcHAKaW5kZXggOTFlNDRkZmQ0OGEzZDA1OTk2Yjg1 ZWI2NjVkNzY4ZTE5MGIwZWIwOS4uZmFlNTg3YWExZDdhOWYzMDEyNWU5YjNjY2VlNzRlMzc1NjIy NGZjYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvTmV0d29ya1Byb2Nlc3MvTmV0d29ya1Jl c291cmNlTG9hZGVyLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9OZXR3b3JrUHJvY2Vzcy9OZXR3 b3JrUmVzb3VyY2VMb2FkZXIuY3BwCkBAIC0yNDQsNyArMjQ0LDExIEBAIHZvaWQgTmV0d29ya1Jl c291cmNlTG9hZGVyOjp3aWxsU2VuZFJlcXVlc3RBc3luYyhSZXNvdXJjZUhhbmRsZSogaGFuZGxl LCBjb25zdCBSCiAKIHZvaWQgTmV0d29ya1Jlc291cmNlTG9hZGVyOjpjb250aW51ZVdpbGxTZW5k UmVxdWVzdChjb25zdCBSZXNvdXJjZVJlcXVlc3QmIG5ld1JlcXVlc3QpCiB7CisjaWYgUExBVEZP Uk0oTUFDKQogICAgIG1fc3VnZ2VzdGVkUmVxdWVzdEZvcldpbGxTZW5kUmVxdWVzdC51cGRhdGVG cm9tRGVsZWdhdGVQcmVzZXJ2aW5nT2xkSFRUUEJvZHkobmV3UmVxdWVzdC5uc1VSTFJlcXVlc3Qo RG9Ob3RVcGRhdGVIVFRQQm9keSkpOworI2Vsc2UKKyAgICBtX3N1Z2dlc3RlZFJlcXVlc3RGb3JX aWxsU2VuZFJlcXVlc3QudXBkYXRlRnJvbURlbGVnYXRlUHJlc2VydmluZ09sZEhUVFBCb2R5KG5l d1JlcXVlc3QpOworI2VuZGlmCiAKICAgICBSdW5Mb29wOjptYWluKCktPmRpc3BhdGNoKGJpbmQo Jk5ldHdvcmtSZXNvdXJjZUxvYWRTY2hlZHVsZXI6OnJlY2VpdmVkUmVkaXJlY3QsICZOZXR3b3Jr UHJvY2Vzczo6c2hhcmVkKCkubmV0d29ya1Jlc291cmNlTG9hZFNjaGVkdWxlcigpLCB0aGlzLCBt X3N1Z2dlc3RlZFJlcXVlc3RGb3JXaWxsU2VuZFJlcXVlc3QudXJsKCkpKTsKIAo= 219642 2013-12-19 05:55:03 -0800 2014-07-12 17:01:16 -0700 Another patch wk2-network-process-buildifx.diff text/plain 3076 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBlNmQ0YmY4Li42ZGU5YmYwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUg QEAKKzIwMTMtMTItMTkgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIEJ1aWxkZml4IGZvciBOZXR3b3JrUmVzb3VyY2VMb2FkZXI6OmNvbnRpbnVl V2lsbFNlbmRSZXF1ZXN0KCkgb24gIVBMQVRGT1JNKE1BQykgcGxhdGZvcm1zCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjE0MzAKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBsYXRmb3JtL25ldHdvcmsv bWFjL1Jlc291cmNlUmVxdWVzdE1hYy5tbToKKyAgICAgICAgKFdlYkNvcmU6OlJlc291cmNlUmVx dWVzdDo6dXBkYXRlRnJvbURlbGVnYXRlUHJlc2VydmluZ09sZEhUVFBCb2R5KToKKyAgICAgICAg VXBkYXRlIHRoZSBSZXNvdXJjZVJlcXVlc3QgZnJvbSBhIG5ldyBvbmUgY3JlYXRlZCB3aXRoCisg ICAgICAgIE5TVVJMUmVxdWVzdCB1c2luZyBEb05vdFVwZGF0ZUhUVFBCb2R5IGZsYWcuCisKIDIw MTMtMTItMTggIEplciBOb2JsZSAgPGplci5ub2JsZUBhcHBsZS5jb20+CiAKICAgICAgICAgW01T RV1bTWFjXSBBZGQgQVZTYW1wbGVCdWZmZXJSZW5kZXJlclN5bmNocm9uaXplciBzdXBwb3J0Lgpk aWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9tYWMvUmVzb3VyY2VS ZXF1ZXN0TWFjLm1tIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9tYWMvUmVzb3Vy Y2VSZXF1ZXN0TWFjLm1tCmluZGV4IDg2OTE0MDUuLjAxNWJlYTkgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvbWFjL1Jlc291cmNlUmVxdWVzdE1hYy5tbQorKysg Yi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL21hYy9SZXNvdXJjZVJlcXVlc3RNYWMu bW0KQEAgLTIyNiw3ICsyMjYsNyBAQCB2b2lkIFJlc291cmNlUmVxdWVzdDo6dXBkYXRlRnJvbURl bGVnYXRlUHJlc2VydmluZ09sZEhUVFBCb2R5KGNvbnN0IFJlc291cmNlUmVxdQogewogICAgIFJl ZlB0cjxGb3JtRGF0YT4gb2xkSFRUUEJvZHkgPSBodHRwQm9keSgpOwogCi0gICAgKnRoaXMgPSBk ZWxlZ2F0ZVByb3ZpZGVkUmVxdWVzdDsKKyAgICAqdGhpcyA9IGRlbGVnYXRlUHJvdmlkZWRSZXF1 ZXN0Lm5zVVJMUmVxdWVzdChEb05vdFVwZGF0ZUhUVFBCb2R5KTsKICAgICBzZXRIVFRQQm9keShv bGRIVFRQQm9keS5yZWxlYXNlKCkpOwogfQogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNTdkZDM1My4uNjc1OTRh YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJL aXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEzLTEyLTE5ICBDYXJsb3MgR2FyY2lh IENhbXBvcyAgPGNnYXJjaWFAaWdhbGlhLmNvbT4KKworICAgICAgICBCdWlsZGZpeCBmb3IgTmV0 d29ya1Jlc291cmNlTG9hZGVyOjpjb250aW51ZVdpbGxTZW5kUmVxdWVzdCgpIG9uICFQTEFURk9S TShNQUMpIHBsYXRmb3JtcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTIxNDMwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBOZXR3b3JrUHJvY2Vzcy9OZXR3b3JrUmVzb3VyY2VMb2FkZXIuY3BwOgorICAg ICAgICAoV2ViS2l0OjpOZXR3b3JrUmVzb3VyY2VMb2FkZXI6OmNvbnRpbnVlV2lsbFNlbmRSZXF1 ZXN0KTogUGFzcyB0aGUKKyAgICAgICAgZ2l2ZW4gUmVzb3VyY2VSZXF1ZXN0IHRvIHVwZGF0ZUZy b21EZWxlZ2F0ZVByZXNlcnZpbmdPbGRIVFRQQm9keSgpLgorCiAyMDEzLTEyLTE4ICBUaW0gSG9y dG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUuY29tPgogCiAgICAgICAgIFdlYktpdDIgVmlldyBH ZXN0dXJlczogTW92ZSBXZWJQcm9jZXNzLXNpZGUgZ2VvbWV0cnkgY29sbGVjdGlvbiBpbnRvIGl0 cyBvd24gY2xhc3MKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL05ldHdvcmtQcm9jZXNzL05l dHdvcmtSZXNvdXJjZUxvYWRlci5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9OZXR3b3JrUHJvY2Vzcy9O ZXR3b3JrUmVzb3VyY2VMb2FkZXIuY3BwCmluZGV4IDJmOGE0OWYuLjE0YWVjNDMgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9XZWJLaXQyL05ldHdvcmtQcm9jZXNzL05ldHdvcmtSZXNvdXJjZUxvYWRlci5j cHAKKysrIGIvU291cmNlL1dlYktpdDIvTmV0d29ya1Byb2Nlc3MvTmV0d29ya1Jlc291cmNlTG9h ZGVyLmNwcApAQCAtMjQzLDcgKzI0Myw3IEBAIHZvaWQgTmV0d29ya1Jlc291cmNlTG9hZGVyOjp3 aWxsU2VuZFJlcXVlc3RBc3luYyhSZXNvdXJjZUhhbmRsZSogaGFuZGxlLCBjb25zdCBSCiAKIHZv aWQgTmV0d29ya1Jlc291cmNlTG9hZGVyOjpjb250aW51ZVdpbGxTZW5kUmVxdWVzdChjb25zdCBS ZXNvdXJjZVJlcXVlc3QmIG5ld1JlcXVlc3QpCiB7Ci0gICAgbV9zdWdnZXN0ZWRSZXF1ZXN0Rm9y V2lsbFNlbmRSZXF1ZXN0LnVwZGF0ZUZyb21EZWxlZ2F0ZVByZXNlcnZpbmdPbGRIVFRQQm9keShu ZXdSZXF1ZXN0Lm5zVVJMUmVxdWVzdChEb05vdFVwZGF0ZUhUVFBCb2R5KSk7CisgICAgbV9zdWdn ZXN0ZWRSZXF1ZXN0Rm9yV2lsbFNlbmRSZXF1ZXN0LnVwZGF0ZUZyb21EZWxlZ2F0ZVByZXNlcnZp bmdPbGRIVFRQQm9keShuZXdSZXF1ZXN0KTsKIAogICAgIFJ1bkxvb3A6Om1haW4oKS0+ZGlzcGF0 Y2goYmluZCgmTmV0d29ya1Jlc291cmNlTG9hZFNjaGVkdWxlcjo6cmVjZWl2ZWRSZWRpcmVjdCwg Jk5ldHdvcmtQcm9jZXNzOjpzaGFyZWQoKS5uZXR3b3JrUmVzb3VyY2VMb2FkU2NoZWR1bGVyKCks IHRoaXMsIG1fc3VnZ2VzdGVkUmVxdWVzdEZvcldpbGxTZW5kUmVxdWVzdC51cmwoKSkpOwogCg==