Bug 137299 – POST form data is missing with a custom NSURLProtocol

• Component: WebKit API
• Status: RESOLVED INVALID

[Daniel]

If a custom NSURLProtocol is handling an HTTP POST request, the request HTTPBody is nil.
This only happens if [WKBrowsingContextController registerSchemeForCustomProtocol:] is called for that protocol. If this method is not called, then the POST request is simply bypassing the custom protocol.

This happens on iOS 8.1 [12B401].
It's also reproducible on the master branch ( 809db5f8edac632ec3332b43dc90795d96914440 ).

Test case:
https://gist.github.com/anonymous/e4ef8586780b2194a6fa
Run it with ./TestWebKitAPI --gtest_filter=WebKit2CustomProtocolsTest.FormPostBodyExists

[Daniel]

Created attachment 239019 [details]
Automatic test case

[Daniel]

Manual test case:
1. go to http://world-backgammon.rhcloud.com/login.php
2. click "Login" button
Expected results: a message "You did not fill in a required field."
Actual results: the page is reloaded, and you see the form again, because POST parameters are not sent (and Content-Length = 0).

For this to happen you need to register a custom NSURLProtocol handling HTTP.

[Andy Estes]

It looks like we don't serialize HTTP body when sending the request from the networking process to the UI process. See http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm#L60.

[Alexey Proskuryakov]

I think that custom protocol callbacks would need to serialize the body separately. However, the comment still holds - we can't possibly always make it work, because the body may be a stream, and thus not meaningfully serializable.

[Daniel]

A comment in the serialization code is saying:
"We don't send HTTP body over IPC for better performance"
What was the performance problem?

Consider a use case of having a custom HTTP implementation using NSURLProtocol. Opera Coast and Google Chrome on iOS are using this approach. If the body is not sent to UIProcess, there's no way a correct request can be made from UIProcess.

If HTTPBodyStream is present one would need to organize proper streaming between processes, where the sending process reads the stream block by block, and sends it through IPC as in series of calls, while in the UIProcess it makes a proxy stream on that request that accumulates data.

I would say that allowing the HTTPBody to be serialized will fix most of the simple FORM POST requests, which might be good enough already. What are the use cases when HTTPBodyStream is not nil? Can it happen with XmlHttpRequest? Maybe some <input> for file uploads?

[Alexey Proskuryakov]

> I would say that allowing the HTTPBody to be serialized will fix most of the simple FORM POST requests, which might be good enough already.

I expect that file upload won't work in this case, which is probably not good enough for a shipping browser.

Beefing up custom protocols so that they could work to fully replace http stack does not seem like a worthy goal to me personally (Sam, Anders and Andy would be the authority on this, not me). Perhaps we should explicitly block anything but GET, to avoid giving an impression that this may sort of work?

HTTP resource fetching is just too low level to meaningfully delegate.

[Sam Weinig]

(In reply to comment #6)
> > I would say that allowing the HTTPBody to be serialized will fix most of the simple FORM POST requests, which might be good enough already.
> 
> I expect that file upload won't work in this case, which is probably not good enough for a shipping browser.
> 
> Beefing up custom protocols so that they could work to fully replace http stack does not seem like a worthy goal to me personally (Sam, Anders and Andy would be the authority on this, not me). Perhaps we should explicitly block anything but GET, to avoid giving an impression that this may sort of work?
> 
> HTTP resource fetching is just too low level to meaningfully delegate.

Please note that using custom protocol handlers with the Modern WebKit API is not supported at all at this time (it would require use of SPI which can change at any time).

[Daniel]

I'm aware that the Modern WebKit API doesn't support NSURLProtocol, so treat this discussion as a feature request.

I would like to share with you some use cases that Opera Coast is having with this and why is it interesting to use HTTP delegation, but I can't freely discuss it in public, so please take a look on a bug rdar://18536522 .

I'm sure that other browser product guys have something to say about this, I'll try to bring them into discussion. I just know that iOS Google Chrome, Opera Mini and Yandex Browser are all successfully utilising HTTP delegation with UIWebView by implementing some custom NSURLProtocols.

[Daniel]

Created attachment 239328 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 239328 [details]
Patch

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

I don't believe that this is acceptable for perform

> Source/WebKit2/ChangeLog:9
> +        This lets using a custom NSURLProtocol in the UI process
> +        to handle POST requests with form parameters.

I do not believe that this is acceptable for performance reasons.

[Anders Carlsson]

While I do think it could be desirable to serialize the request body (assuming it's an NSData object), I don't think we want to do this for _all_ requests, and we have to be careful to not do unnecessary copies (so we would want to do copy-on-write shared memory mapping of the data).

[Stuart Morgan]

(In reply to comment #0)
> If a custom NSURLProtocol is handling an HTTP POST request, the request HTTPBody is nil.
> This only happens if [WKBrowsingContextController registerSchemeForCustomProtocol:] is called for that protocol.

This is actually not true; the same problem exists in a stock WKWebView without calling any SPI. If you implement WKNavigationDelegate callbacks, the NSURLRequest provided in those callbacks is missing this data, so the delegate method is providing incorrect information to the client (this is filed as rdar://18399639 BTW).

Even without custom protocol handlers, Chrome needs this information in order to correctly implement our back-forward handling.

[Stuart Morgan]

(In reply to comment #6)
> Beefing up custom protocols so that they could work to fully replace http stack does not seem like a worthy goal

Please keep in mind that given that UIWebView is increasingly crippled (slow JS, no IndexedDB support, etc.), this ends up being equivalent in practice to saying that continuing to allow at least the current level of support for an ecosystem of compelling third-party browsers on iOS is not a worthy goal. Speaking for Chromium on iOS, obviously we hope that does not end up being the team's official position.

[Daniel]

Created attachment 239465 [details]
patch part 2

Please check out an update - patch part 2. Now request body is serialized only in the IPC message "CustomProtocolManagerProxy::StartLoading" (a message that's sent to initiate a resource loading in another process). In all other cases it works like before, that is the body is stripped.
That's a rough implementation I'm sure there will be some review comments, so I didn't make a proper single patch with changelogs, sorry for that.

[Alexey Proskuryakov]

Does form upload work with this patch? If it does, can you please explain how it serializes a body stream?

[Daniel]

Right, this patch only sends NSURLRequest.HTTPBody, and doesn't handle NSURLRequest.HTTPBodyStream. HTTPBodyStream is stripped as before. It means that uploading a file won't work (for example on http://imgur.com ). I think that the file upload use case should be treated in a separate bug, because it's quite a different use case, and should be prioritized separately. It needs different test cases as well. I'm going to create a separate bug report for this if that's fine.

The patch here fixes all the form types without "input type=file", including all sorts of login and registration forms.

[Daniel]

Separate bug report for handling file uploads (HTTPBodyStream):
https://bugs.webkit.org/show_bug.cgi?id=138131

[Daniel]

Created attachment 240538 [details]
Patch

[Daniel]

Submitted a combined patch with updated changelogs.

[Darin Adler]

Comment on attachment 240538 [details]
Patch

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

I don’t think the approach is quite right here. Also, the patch does not build on any of the EWS bots, so review-.

> Source/WebCore/platform/network/ResourceRequestWithBody.h:35
> +class ResourceRequestWithBody {

I don’t understand the purpose of this class. It seems to wrap a ResourceRequest and add nothing, with forwarding functions. Does not seem like a good pattern to add a class like this. I understand the desire to have clarity about whether a body is present, but this does not seem to be the right way to accomplish that. We don’t want a class that we have to keep updating every time we change the design of ResourceRequest.

> Source/WebCore/platform/network/ResourceRequestWithBody.h:36
> +    ResourceRequest m_request;

This is the wrong style for WebKit. We put private data members last, after public members, and we explicitly write "private".

> Source/WebCore/platform/network/ResourceRequestWithBody.h:49
> +#if ENABLE(CACHE_PARTITIONING)
> +    const String& cachePartition() const { return m_request.cachePartition(); }
> +#endif
> +#if ENABLE(INSPECTOR)
> +    // Whether this request should be hidden from the Inspector.
> +    bool hiddenFromInspector() const { return m_request.hiddenFromInspector(); }
> +#endif

These should have blank lines around them, so they make their own paragraphs.

> Source/WebCore/platform/network/ResourceRequestWithBody.h:52
> +    template<class Encoder> void encodeWithoutPlatformData(Encoder& e) const { m_request.encodeWithoutPlatformData(e); }

We use words for argument names, not letters. This should be "encoder".

[Daniel]

> ResourceRequestWithBody {
> I don’t understand the purpose of this class
> I understand the desire to have clarity about whether a body is present, but this does not seem to be the right way to accomplish that.

That's correct. It's only to trigger a different serialization method to work (pick a different ArgumentCoder<T>). The current IPC is implemented by using templates. When you call a send() method, it uses some magic (variadic templates) to determine an ArgumentCoder by type of the passed argument (for each argument). It's done at compile time.

Another option to fix it would be to pass the message type (class MT) through this template code path. I could add a parameter MT that you pass to send(), so that it uses a different coder method in the end if the MT matches "StartLoading" type.

Another option to fix it would be to add a flag to ResourceRequest. This sounds dirty to me, because this flag conceptually doesn't belong to ResourceRequest. It belongs to a party which is managing serialization.

Another option would be to avoid using templates at all, and just inline them in a place where send() is called, so that all argument encoding parameters are fixed there, i.e. create a message to pass by hand instead of letting the template magic do it automatically. This is dirty as well.

What do you think is best?
Do you have other ideas?

> I don’t think the approach is quite right here. Also, the patch does not build on any of the EWS bots, so review-.

I'm looking at the errors ( https://webkit-queues.appspot.com/patch/240538 ), but I don't understand them. It looks like #import statements are missing, but it's not true. I'm building using Xcode and command line (build-webkit script), and it builds well, but I'm only building in debug mode. It's some release configuration problem. I'll try to build a release and see what's wrong. If you know what's wrong, please let me know.

[Daniel]

A release build on my machine with "build-webkit --release" worked fine, what's the difference on EWS? How to test this?

[Daniel]

Created attachment 240667 [details]
Patch

[Daniel]

I reimplemented a patch so that it doesn't touch WebCore, and ResourceRequestWithBody is not needed.

[Daniel]

The windows build fails, but I can't find any error in the log:
https://webkit-queues.appspot.com/results/5319787991269376
Seems like the TestWebKitAPI project fails, but why?

[Darin Adler]

Anders is the right person to look at this.

[Maciej Stachowiak]

Two comments on this patch:

(1) Both TwestWebKitAPI and ImageDiffLauncher fail reporting this warning:      1>..\Tests\WTF\MediaTime.cpp(174): warning C4056: overflow in floating-point constant arithmetic

I didn't think warnings would ever cause a build failure on Windows but maybe this is the reason. Perhaps the patch should just be re-uploaded.

(2) If serializing the body is right, I don't think it's correct to do it only for the custom protocol case. As Stuart Morgan mentioned, Chrome for iOS would have a use for it for purposes of the back-forward list even without custom protocols. Since custom protocols are not even supported in the Modern WebKit API at present, I am not sure how the patch posted here would even do anything. Can you explain in what cases it has an effect, and how you tested it?

(3) The patch lacks tests. It should probably have an API test.

(4) If the patch is changed to be global, it probably needs to be performance tested. If the body serialization is a performance hit, we may need to be clever about how to optimize, such as by using shared memory. I think if the patch could be done with no measurable slowdown and worked for even the non-custom-protocol case, likely no one would have a strong objection.

(5) I don't think covering only HTTPBody but not HTTPBodyStream is sufficient for use in a browser, either for custom protocols or for the back/forward case. Any time a form contains a file upload, it will have a body stream. If a client relies on presence of the body, it will fail for this case. While there's no obvious way to serialize an arbitrary HTTPBodyStream, we might be able to special-case detect the one produced by WebKit and convert it back to an array of data chunks and file references.

If we do support passing the body to the client, and possibly eventually support custom protocol handlers for HTTP, I think the feature should be done fully correctly instead of just trying to hit the 90% case.

[Daniel]

Thanks for taking a look Maciej.

(1) I'll rebase and reupload to "unred" the windows.
(2) You've asked 2 questions here:
The first is "why it's not general, but only for startLoading case?". It's because it's a minimum that's enough for our use cases. Having this the Chrome team can adjust their code to take a body from on the protocol level, and then use it in the delegate callbacks. Another reason is that having it global is a vague performance problem (in some conditions, on some platforms, hard to test), and that's why it was stripped in the first place. Having it for a single case makes performance implications clear.

My first attempt was to enable it to be passed globally, you can see it in obsolete patches:
https://bugs.webkit.org/attachment.cgi?id=239328&action=prettypatch
And it was rejected for performance reasons, which you mention as well.

The 2nd question is: "how did I test it on iOS?". The answer is in the bug description:
You have to call [WKBrowsingContextController registerSchemeForCustomProtocol:], then it works as expected.

(3) The patch doesn't lack tests. The test case was uploaded in the first place (you can see it in obsolete patches), and it is still present in the last version of the patch. It's in the file Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm . See:
https://bugs.webkit.org/attachment.cgi?id=240667&action=diff#a/Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm_sec1

(5) I agree about that having file upload support is important. I just want the work to be split in separate tasks, that's why I made a separate bug 138131 for supporting uploads. I don't think that the work done here code-wise depends on 138131 or vice-versa, because:
a) separate fields are used for normal body and stream
b) serialization is done separately
c) different methods should be applied when passing this stream through IPC (and that can be a long discussion for streams)
d) separate test case needs to be made
For me this is enough to split it as a separate issue, and work on it separately.

The second reason for splitting it is that some clients might be already happy by only having simple POST forms.

The third reason is that it's the first experience, and I wanted to start gently with small and targeted changes, where it's easy for us to argue about the solution and if it makes sense or not. By no means I wanted to start with some huge architectural changes that affect performance in unknown ways, and require a lot of experience and testing.

If this work is accepted, and a related 137302 is accepted (which is already reviewed, but blocked by this bug), I was planning to start working on uploads shortly, because we definitely need them as well. So don't worry, the streams case won't be forgotten.

[Daniel]

Created attachment 243971 [details]
Patch

[Daniel]

Rebased to master. No conflicts, no changes. Waiting for reviewers to respond to the last comments.

[Sam Weinig]

After discussing this a bit, we have decided that since we do not support custom NSURLProtocols as API, there is no reason to complicate the code further by adding this functionality that no one will be allowed to use. As such, I don't think this bug is valid.

I appreciate your work on it, and apologize for taking so long to come to a conclusive answer here.  I am interested in the larger types of problems you are having that make you feel using a custom protocol would solve. Please feel free to file those.

[Stuart Morgan]

> we have decided that since we do not support custom NSURLProtocols as API, there
> is no reason to complicate the code further by adding this functionality that no
> one will be allowed to use.

As noted in comment 12 and comment 27 this bug actually is not specific to NSURLProtocols and is a serious problem for Chromium on iOS without any SPI involved.

Would retitling this bug to replace "with a custom NSURLProtocol" with "in navigation delegate callbacks" help change the resolution?

[Sam Weinig]

(In reply to comment #32)
> > we have decided that since we do not support custom NSURLProtocols as API, there
> > is no reason to complicate the code further by adding this functionality that no
> > one will be allowed to use.
> 
> As noted in comment 12 and comment 27 this bug actually is not specific to
> NSURLProtocols and is a serious problem for Chromium on iOS without any SPI
> involved.
> 
> Would retitling this bug to replace "with a custom NSURLProtocol" with "in
> navigation delegate callbacks" help change the resolution?

Absolutely, though at this point, I think filing a new bug about an issue with the navigation delegate callbacks would be ideal, so we can have a cleaner conversation about that there.

[Sam Weinig]

(In reply to comment #32)
> > we have decided that since we do not support custom NSURLProtocols as API, there
> > is no reason to complicate the code further by adding this functionality that no
> > one will be allowed to use.
> 
> As noted in comment 12 and comment 27 this bug actually is not specific to
> NSURLProtocols and is a serious problem for Chromium on iOS without any SPI
> involved.
> 
> Would retitling this bug to replace "with a custom NSURLProtocol" with "in
> navigation delegate callbacks" help change the resolution?

Absolutely, though at this point, I think filing a new bug about an issue with the navigation delegate callbacks would be ideal, so we can have a cleaner conversation about that there.

[Stuart Morgan]

(In reply to comment #34)
> I think filing a new bug about an issue with the navigation delegate callbacks
> would be ideal, so we can have a cleaner conversation about that there.

Makes sense, thanks! Filed bug 140188.