RESOLVED DUPLICATE of bug 125422 121430
Buildfix for NetworkResourceLoader::continueWillSendRequest() on !PLATFORM(MAC) platforms 
https://bugs.webkit.org/show_bug.cgi?id=121430
Summary Buildfix for NetworkResourceLoader::continueWillSendRequest() on !PLATFORM(MA...
Csaba Osztrogonác
Reported 2013-09-16 07:04:19 PDT
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?"
Attachments
Patch (1.83 KB, patch)
2013-09-16 07:08 PDT, Csaba Osztrogonác 		no flags
DetailsFormatted DiffDiff
Another patch (3.00 KB, patch)
2013-12-19 05:55 PST, Carlos Garcia Campos 		darin: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Csaba Osztrogonác
Comment 1 2013-09-16 07:08:23 PDT
Created attachment 211780 [details] Patch It is the originally proposed fix, because there wasn't a better suggestion.
Csaba Osztrogonác
Comment 2 2013-09-17 08:18:14 PDT
Could you suggest any better fix for it or is it OK after the comment of Kwang Yul Seo?
Darin Adler
Comment 3 2013-09-17 10:48:24 PDT
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.
Csaba Osztrogonác
Comment 4 2013-09-18 07:58:30 PDT
This thing was introduced in http://trac.webkit.org/changeset/144216. I'm going to check this change.
Brian Holt
Comment 5 2013-12-09 03:56:13 PST
(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.
Martin Robinson
Comment 6 2013-12-09 06:21:00 PST
Alexey, do you mind giving some insight into this particular line of code?
Alexey Proskuryakov
Comment 7 2013-12-09 09:46:43 PST
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.
Carlos Garcia Campos
Comment 8 2013-12-19 05:55:03 PST
Created attachment 219642 [details] Another patch I wonder if we can avoid the platform ifdef entirely by moving the NSURLRequest to the platform specific file.
Alexey Proskuryakov
Comment 9 2013-12-19 11:54:42 PST
Comment on attachment 219642 [details] 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.
Carlos Garcia Campos
Comment 10 2013-12-19 11:58:37 PST
(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?
Alexey Proskuryakov
Comment 11 2013-12-19 12:02:27 PST
Not sure, are you saying that delegateProvidedRequest only has an NSURLRequest in it, and platform request fields are not updated?
Carlos Garcia Campos
Comment 12 2013-12-19 12:16:39 PST
(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));
Alexey Proskuryakov
Comment 13 2013-12-19 12:34:24 PST
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.
Carlos Garcia Campos
Comment 14 2013-12-20 01:41:19 PST
(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.
Darin Adler
Comment 15 2014-07-12 17:01:17 PDT
Comment on attachment 219642 [details] Another patch Issue might still exist, but this has changed quite a bit since December, and the patch as-is won’t be needed.
Darin Adler
Comment 16 2014-07-12 17:01:48 PDT
Comment on attachment 211780 [details] Patch Looks like we landed something like this a long time ago.
Csaba Osztrogonác
Comment 17 2014-12-03 07:58:15 PST
*** This bug has been marked as a duplicate of bug 125422 ***
Note You need to log in before you can comment on or make changes to this bug.