Bug 82036 – Soup HTTP backend does not send Content-Length in certain cases

• Component: Platform
• Status: RESOLVED FIXED

[Dominik Röttsches (drott)]

The soup http backend does not send Content-Length on outgoing requests in certain cases. Especially striking for POST and PUT requests.

As far as I interpret the HTTP specification, 
http://tools.ietf.org/html/rfc2616#section-14.13
the content-length should be send whenever the message-body size can be determined at send time and if the particular type of method usually has a body / has body semantics defined - e.g. no Content-Length required for GET and HEAD requests.

Looking at the http/tests/xmlhttprequest/methods*.html cases,
it can be consistently found in the non-soup ports that on the POST cases, they all send a content-length.
They all don't send Content-Length on GET request.

We should make the libsoup backend behave more in line with spec and other browser implementations here.

[Dominik Röttsches (drott)]

Created attachment 133462 [details]
Explicitly tell soup to send Content-Length

[WebKit Review Bot]

Attachment 133462 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:507:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/ChangeLog:12:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominik Röttsches (drott)]

Created attachment 133468 [details]
Explicitly tell soup to send Content-Length, v2

Style issues fixed.

[Dan Winship]

Comment on attachment 133468 [details]
Explicitly tell soup to send Content-Length, v2

>+    // Make outgoing requests more strictly adhere to HTTP RFC 2616

This is not about adherence to RFC 2616. HTTP has a distinction between "a zero-length body" and "no body", and each is useful/desired in some situations.

In the particular case of XMLHttpRequest, the spec says: (http://www.w3.org/TR/XMLHttpRequest/#the-send-method)

    3. If the request method is a case-sensitive match for GET or HEAD act
       as if data is null.

       If the data argument has been omitted or is null, do not include a
       request entity body and go to the next step.

So by those rules, the soup backend is correct (at least for Content-Length; the spec then goes on to give rules for Content-Type that no WebKit browsers obey...)

Bug 3812 comment 6 suggests that Firefox behaves the same way as libsoup.

So... I'm not convinced there's an actual bug here. What is the exact problem you were running into?

[Dominik Röttsches (drott)]

(In reply to comment #4)
> (From update of attachment 133468 [details])
> >+    // Make outgoing requests more strictly adhere to HTTP RFC 2616

Dan, thanks for taking the time to look into this.

For the GET/HEAD cases, I think we agree - and there's nothing wrong with those case. Please note that the rebaselining is not touching the GET/HEAD cases for GTK.

The initial problem that I faced was that the test cases that I am (re)baselining here initially failed in EFL. Then I looked a bit closer at why there were rebaselined for GTK last time and what the other ports to. 

The issue I distilled is that in the POST cases, all other ports, and the other browsers I tested (Opera, Firefox, IE), they always send a Content-Length for POST. For "no body" as well as for "zero length" body.

So, before creating the baseline for EFL I wanted to make the POST-case behavior of libsoup more consistent with those other ports and browser. I am still inclined to quote the Content-Length section of the HTTP spec as reasoning, even though there might be a better reason that I haven't found yet.

Does that make sense?

[Dan Winship]

(In reply to comment #5)
> (In reply to comment #4)
> The initial problem that I faced was that the test cases that I am (re)baselining here initially failed in EFL. Then I looked a bit closer at why there were rebaselined for GTK last time and what the other ports to. 

Hm... I wonder if we should have LayoutTests/platform/soup/ for shared gtk/efl overrides. (Especially since if the clutter port ever lands it would share them as well.)

> The issue I distilled is that in the POST cases, all other ports, and the other browsers I tested (Opera, Firefox, IE), they always send a Content-Length for POST. For "no body" as well as for "zero length" body.

Ah, I didn't actually test Firefox, but you're right, and the earlier bug I mentioned before is wrong. (Or possibly, Firefox changed its behavior between then and now.)

> I am still inclined to quote the Content-Length section of the HTTP spec as reasoning, even though there might be a better reason that I haven't found yet.

No, really, it's not relevant at all. Trust me. :)

The reasoning here seems to be "web page authors expect that 'req.send()' or 'req.send(null)' means the same thing as 'req.send("")' for a PUT/POST, even though according to the spec it doesn't."

(And if that's the case, then really, this should be at a higher level, in the xmlhttprequest code, not in the soup backend...)

> +    if (request.httpMethod() != "GET" && request.httpMethod() != "HEAD") {

Again, I think we should special case PUT and POST here, not everything except GET and HEAD. (This matches Firefox and Chrome better.)

> +        soup_message_headers_set_encoding(soupMessage->request_headers, SOUP_ENCODING_CONTENT_LENGTH);

That breaks anyone trying to use chunked encoding in a request. You'd want to put that in the if(empty) as well. But it turns out you don't need that line anyway; if you call soup_message_headers_set_content_length(), it sets the encoding for you as well.

[Gustavo Noronha (kov)]

I have 3 comments about this:

It looks like the new expected results for EFL and the updated ones for GTK+ match the non-platform-specific ones now, so they platform-specific ones should not be required, see: http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/xmlhttprequest/workers/shared-worker-methods-expected.txt#L1

I agree with Dan this should go up, to the XMLHTTPRequest code, would be good to grab ap's input on this point.

If ap thinks this code should live in the http backend, then the patch with the changes suggested by Dan (including the comment one) and dropping the baselines (unless I'm missing something and they're actually required) would be fine to go in, in my opinion, so I'd r+.

[Dan Winship]

The results would match the baselines if we did (!GET && !HEAD && !DELETE) [note that the current version of the patch does not special-case DELETE], but not if we did (PUT || POST).

With (PUT || POST), we match Firefox's behavior, and match Chrome's behavior except for a really obvious blatant bug (Chrome sends a body with HEAD requests).

[Alexey Proskuryakov]

So if this is moved to XMLHttpRequest code, will there be a need to make a separate fix for empty forms?

<form method=post action="http://127.0.0.1:8000/">
<input type=submit>
</form>

This is also sent with Content-Length in Safari and Firefox.

[Dan Winship]

(In reply to comment #9)
> So if this is moved to XMLHttpRequest code, will there be a need to make a separate fix for empty forms?

no, because WebKit sets a Content-Type in that case, so we already know there must be a body

[Dominik Röttsches (drott)]

Opened new bug 82202.

[Eric Seidel (no email)]

Comment on attachment 133468 [details]
Explicitly tell soup to send Content-Length, v2

Cleared review? from attachment 133468 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Dominik Röttsches (drott)]

Bug 82202 showed that because of CORS checking in XHR leve, it's tricky to add headers there. Will do that at soup backend level. Patch follows.

[Dominik Röttsches (drott)]

Created attachment 135047 [details]
Special case for POST & PUT, comment edited

I hope with this one we can reach consensus and the comment fits - at least I am not mentioning the HTTP RFC anymore ;-).

[Gustavo Noronha (kov)]

Comment on attachment 135047 [details]
Special case for POST & PUT, comment edited

OK

[WebKit Review Bot]

Comment on attachment 135047 [details]
Special case for POST & PUT, comment edited

Clearing flags on attachment: 135047

Committed r113019: <http://trac.webkit.org/changeset/113019>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.