Bug 121430

Summary:

Buildfix for NetworkResourceLoader::continueWillSendRequest() on !PLATFORM(MAC) platforms

Product:

WebKit

Reporter:

Csaba Osztrogonác <ossy>

Component:

New Bugs

Assignee:

Csaba Osztrogonác <ossy>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

ap, beidson, brian.holt, cgarcia, darin, kbalazs, mrobinson, ossy, skyul

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

110141

Attachments:

Description	Flags
Patch	none
Another patch	darin: review-

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	Details Formatted Diff Diff
Another patch (3.00 KB, patch) 2013-12-19 05:55 PST, Carlos Garcia Campos	darin: review-	Details Formatted Diff Diff
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.