Bug 121430 - Buildfix for NetworkResourceLoader::continueWillSendRequest() on !PLATFORM(MAC) platforms
Summary: Buildfix for NetworkResourceLoader::continueWillSendRequest() on !PLATFORM(MA...
Status: RESOLVED DUPLICATE of bug 125422
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 110141
  Show dependency treegraph
 
Reported: 2013-09-16 07:04 PDT by Csaba Osztrogonác
Modified: 2014-12-03 07:58 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 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?"
Comment 1 Csaba Osztrogonác 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.
Comment 2 Csaba Osztrogonác 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?
Comment 3 Darin Adler 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.
Comment 4 Csaba Osztrogonác 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.
Comment 5 Brian Holt 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.
Comment 6 Martin Robinson 2013-12-09 06:21:00 PST
Alexey, do you mind giving some insight into this particular line of code?
Comment 7 Alexey Proskuryakov 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.
Comment 8 Carlos Garcia Campos 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.
Comment 9 Alexey Proskuryakov 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.
Comment 10 Carlos Garcia Campos 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?
Comment 11 Alexey Proskuryakov 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?
Comment 12 Carlos Garcia Campos 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));
Comment 13 Alexey Proskuryakov 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.
Comment 14 Carlos Garcia Campos 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.
Comment 15 Darin Adler 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.
Comment 16 Darin Adler 2014-07-12 17:01:48 PDT
Comment on attachment 211780 [details]
Patch

Looks like we landed something like this a long time ago.
Comment 17 Csaba Osztrogonác 2014-12-03 07:58:15 PST

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