-[WKWebView loadRequest:] ignores HTTPBody in POST requests

• Component: WebKit2
• Status: RESOLVED DUPLICATE

[Eugene But]

Created attachment 253793 [details]
Test App

Steps to Reproduce:
1. Run "nc -l 8000" in terminal
2. Unzip WKWebView.zip, build and run attached project
3. Observe terminal output from nc tool

Expected Results:
nc should log TestBody

Actual Results:
nc logs 0 content length:
Content-Length: 0


Not reproducible with UIWebView
rdar://21124107

[Stuart Morgan]

Note that unlike bug 140188 it doesn't seem this could be argued to be a performance optimization; I can't imagine any case where a developer would construct a request with a body and pass that to loadRequest: and *not* want the body to be included.

[Stuart Morgan]

Is there anything helpful we could do to get traction on this? Being unable to do any programmatic action involving a POST is a pretty serious limitation.

[Brady Eidson]

Created attachment 258634 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 258634 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=258634&action=review

> Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm:99
> -    // We don't send HTTP body over IPC for better performance.
> -    // Also, it's not always possible to do, as streams can only be created in process that does networking.
> -    if ([requestToSerialize HTTPBody] || [requestToSerialize HTTPBodyStream]) {
> +    // We don't send HTTP body over IPC if streams are involved, as streams can only be created in process that does networking.
> +    if ([requestToSerialize HTTPBodyStream]) {

On the surface, this looks like it would kill performance, why won't it?

[Brady Eidson]

(In reply to comment #4)
> Comment on attachment 258634 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258634&action=review
> 
> > Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm:99
> > -    // We don't send HTTP body over IPC for better performance.
> > -    // Also, it's not always possible to do, as streams can only be created in process that does networking.
> > -    if ([requestToSerialize HTTPBody] || [requestToSerialize HTTPBodyStream]) {
> > +    // We don't send HTTP body over IPC if streams are involved, as streams can only be created in process that does networking.
> > +    if ([requestToSerialize HTTPBodyStream]) {
> 
> On the surface, this looks like it would kill performance, why won't it?

In discussion on IRC, I've been given the impression we actually have hard data that not having this restriction killed performance.

I was not aware of this data, and had just thought through how rare body posts are when compared to normal subresource loads, figuring this was unlikely to be relevant.

Since there's more complexity than previously known, I won't have time to pursue this further.

[Build Bot]

Comment on attachment 258634 [details]
Patch v1

Attachment 258634 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/39064

New failing tests:
http/tests/navigation/postredirect-goback1.html
http/tests/navigation/post-goback-same-url.html
http/tests/navigation/postredirect-basic.html
http/tests/navigation/forward-and-cancel.html
http/tests/navigation/post-goback2.html
http/tests/navigation/postredirect-frames.html
http/tests/navigation/postredirect-goback2.html
http/tests/navigation/postredirect-reload.html

[Build Bot]

Created attachment 258641 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Stuart Morgan]

I'm confused by the performance argument. I understand the callback case (bug 140188), where the body may not be used, but in the case of a request being explicitly given to WKWebView it's always necessary; if it weren't the caller wouldn't have added it.

What's the point of making this case more performant by making it completely wrong, and thus useless to call? This seems analogous to making pageload faster by simply not rendering the page.

The only alternative we've found is a tortured (and flaky) workaround involving encoding the entire body to construct and then inject JavaScript that does the POST, and I can't imagine that needlessly loading a blank page and then round-tripping through JS is more performant than having POST work natively.

[Brady Eidson]

(In reply to comment #8)
> I'm confused by the performance argument. I understand the callback case
> (bug 140188), where the body may not be used, but in the case of a request
> being explicitly given to WKWebView it's always necessary; if it weren't the
> caller wouldn't have added it.
> 
> What's the point of making this case more performant by making it completely
> wrong, and thus useless to call? This seems analogous to making pageload
> faster by simply not rendering the page.
> 
> The only alternative we've found is a tortured (and flaky) workaround
> involving encoding the entire body to construct and then inject JavaScript
> that does the POST, and I can't imagine that needlessly loading a blank page
> and then round-tripping through JS is more performant than having POST work
> natively.

While I share your frustration, your characterization is overblown - The performance concern with always shipping the body across the wire is about a *huge* body being encoded/decoded multiple times (redirects, etc).

That comes up in normal page loading in a browser application, which is way more common than embedded WebKit apps.

Nobody here is arguing this is not currently broken - It, of course, is.

Fixing it "the Right Way™" will take some brainstorming.

[Stuart Morgan]

(In reply to comment #9)
> While I share your frustration, your characterization is overblown - The
> performance concern with always shipping the body across the wire is about a
> *huge* body being encoded/decoded multiple times (redirects, etc).

How is -[WKWebView loadRequest:] involved in a redirect?

The reason we filed this bug separately from bug 140188 is because that bug is about a case that would affect performance all the time, whereas this is is just about the specific case where an embedder is calling the embedding API and providing a POST body. The "always" in this case is "in all cases where the embedder has explicitly tried to make a POST request with a body". I genuinely don't see how it's overblown to say that discarding the body makes that call useless.

> That comes up in normal page loading in a browser application, which is way
> more common than embedded WebKit apps.

Again, this bug is just about the embedding API. (Although, given that we filed this bug on behalf of Chromium, I don't actually see a distinction between a browser application and an embedding app.)

> Nobody here is arguing this is not currently broken

Sure, I'm just trying to understand why for this case the argument is that 'working but slow' is worse than 'fast but completely broken', given that we in practice the current situation forces embedders to go with a totally different option that is 'slow, convoluted, kind of broken, and can't be made transparently faster by later under-the-hood improvements'.

Is the issue that it's hard to fix this case without also affecting cases like bug 140188.

[Brady Eidson]

(In reply to comment #10)
> (In reply to comment #9)
> > While I share your frustration, your characterization is overblown - The
> > performance concern with always shipping the body across the wire is about a
> > *huge* body being encoded/decoded multiple times (redirects, etc).
> 
> How is -[WKWebView loadRequest:] involved in a redirect?

Re-read my comment. The performance concern is with *always* shipping the body across the wire, and that's what this patch implements.

Redirects happen in normal web browsing scenarios in browser-type apps (e.g., Safari), and they also happen with site-specific WebKit apps.

The controversy is not whether or not we should fix this bug, but whether or not it is acceptable to cause a performance regression in currently fast cases (such as browsing in Safari) when doing so.

> The reason we filed this bug separately from bug 140188 is because that bug
> is about a case that would affect performance all the time, whereas this is
> is just about the specific case where an embedder is calling the embedding
> API and providing a POST body. The "always" in this case is "in all cases
> where the embedder has explicitly tried to make a POST request with a body".
> I genuinely don't see how it's overblown to say that discarding the body
> makes that call useless.

Discarding the body makes -[WKWebView loadRequest:] useless for POST requests with bodies, yes.

Such requests are important, but not the common case.

However, that's not what you claimed. You said earlier:
> What's the point of making this case more performant by making it completely
> wrong, and thus useless to call? This seems analogous to making pageload
> faster by simply not rendering the page.

That analogy is what I'm saying was overblown. 

WebKit is a highly complex web content rendering engine that has countless stakeholders - such as Safari in addition to 3rd party apps. Saying that one particular use case in only one of the supported APIs being broken is equal to "may as well speed things up by not rendering web pages" is somewhat insulting to the contributors to the project. 

> > Nobody here is arguing this is not currently broken
> 
> Sure, I'm just trying to understand why for this case the argument is that
> 'working but slow' is worse than 'fast but completely broken', given that we
> in practice the current situation forces embedders to go with a totally
> different option that is 'slow, convoluted, kind of broken, and can't be
> made transparently faster by later under-the-hood improvements'.

High performance (including zero regressions) is one of the stated goals of the WebKit project.

Correctness is obviously also a stated goal.

Obviously these goals are occasionally in conflict, and have to be weighed against one another.

While we definitely appreciate that you have a personal stake in this one specific bug and that is why you are so passionate about it, the fact of the matter is that fixing this one specific bug *in this one specific way* is not a valid tradeoff for the project.

Please note that at *no time* did anybody state this is not a bug, at *no time* did anybody state we should not fix this, and at *no time* did anybody state that we would (for any bizarre reason) reject a patch that fixes this without introducing a performance regression across the board.

The only things stated were that the patch I decided to knock out during a spare couple of hours turns out to have too simplistic a view of the problem, and that I personally won't have time to pursue it further right now.

[Stuart Morgan]

> The performance concern is with *always* shipping the body across the wire, and
> that's what this patch implements.

Thanks, this was not at all clear to me; since the patch description only mentioned this bug, and the patch was here rather than in the more general bug, I assumed the change was specific to this case. Given that, it sounded like the issue was that this fixed the bug in a way that wasn't sufficiently performant (vs. actually regressing other behavior).

Hopefully re-reading my comments in that light will make them seem less overdramatic :)

> Saying that one particular use case in only one of the supported APIs being broken
> is equal to "may as well speed things up by not rendering web pages" is somewhat
> insulting to the contributors to the project.

I apologize for the phrasing. I didn't intend this to be a "this one bug ruins everything" but I can definitely see how it comes across that way.

Now that I understand what the performance concern actually is, I completely understand the decision. Sorry for the noise.

[David]

At the risk of also creating noise, I would like to put my vote in for finding the time to fix this bug.

I get why it cannot be fixed by always sending the HTTPBody, but just getting loadRequest to send the body the other way (to the WebKit process) would seem reasonable. I have not seen the code though so have no idea if it is.

I have also filed this as rdar://24489469

[Florent Crivello]

FYI, I've been faced with this bug too, and one workaround is to make the WKWebView execute a Javascript script to redirect with a POST body:

    NSString *javascriptPOSTRedirect = @"\
    var form = document.createElement('form');\
    form.method = 'POST';\
    form.action = '<URL>';\
    \
    var input = document.createElement('input');\
    input.type = 'text';\
    input.name = '<key>';\
    input.value = '<value>';\
    form.appendChild(input);\
    form.submit();";
    
    [webView evaluateJavaScript:javascriptPOSTRedirect completionHandler:^(id _Nullable content, NSError * _Nullable error) {
        // Your thing
    }];

[Michael Taverne]

I would also like to see a solution to this. I ran into this issue while trying to implement Apple Pay within a WKWebView, an approach that Apple appears to endorse since it provides rudimentary sample code for it. I want to post the payment token to my web server but am unable to do so using WKWebView.

[David Gatwood]

Why not just do what NSURLProtocol does?  When you write an NSURLProtocol the body data object is always nil.  However:

1.  If there was a body stream object, it is still available.
2.  If there was a body data object, the API creates a body stream object with [NSInputStream inputStreamWithData:request.body] that reads from the body object.

Unless I'm missing something, that approach should work just fine for you.  Code that needs to grab the body data can just read from the stream, and code that doesn't care about the body data won't take the performance hit introduced by serializing and deserializing a huge data blob.

[Brady Eidson]

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