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

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Created attachment 211780 [details]
Patch

It is the originally proposed fix, because there wasn't a better suggestion.

[Csaba Osztrogonác]

Could you suggest any better fix for it or is it OK after the comment of Kwang Yul Seo?

[Darin Adler]

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]

This thing was introduced in http://trac.webkit.org/changeset/144216.
I'm going to check this change.

[Brian Holt]

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

Alexey, do you mind giving some insight into this particular line of code?

[Alexey Proskuryakov]

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]

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 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]

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

Not sure, are you saying that delegateProvidedRequest only has an NSURLRequest in it, and platform request fields are not updated?

[Carlos Garcia Campos]

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

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]

(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 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 on attachment 211780 [details]
Patch

Looks like we landed something like this a long time ago.

[Csaba Osztrogonác]

*** This bug has been marked as a duplicate of bug 125422 ***