Ignore Content-Length mismatches when Content-Length is too large.

If fewer bytes are transferred in an HTTP response body than are advertised by the Content-Length header when the connection is closed, treat it as a valid response.

This used to be reported as ERR_CONNECTION_CLOSED. Unfortunately, there are a number of misconfigured servers on the web which report a Content-Length completely divorced from reality. Other browsers display whatever data was received without reporting errors in this case, and Chrome now does the same.

An earlier CL reported this as an error (ERR_CONTENT_LENGTH_MISMATCH), and had the webkit glue layer rewrite this case as a successful response. Unfortunately, this led to incorrect state machine logic (and crashes) when the connection was closed before the MIME type sniffing in BufferedResourceHandler completed.

If certain requests want to be alerted about this issue (such as URLFetcher or downloads) in the future, we could add a LOAD_FLAG to specify whether to return an error or treat the result as OK. Alternately, we could rewrite the URLRequestStatus in ResourceDispatcherHostImpl.

BUG=52847
TEST=Manually, go to www.goldwarez.org and confirm that we do not crash. Added some browser tests as well to test, but I may remove these or need to find a better location. Also, net_unittests have been modified.

[cbentzel, 2012-03-23 18:40:07]

I'm trying to find a better place to put the browser_tests than
prerender_browsertest - they were just convenient for now.

My previous attempt at this caused a crash, and the CL description describes why
it happened. Fundamentally, rewriting an error from the URLRequest layer into a
success violates a number of issues. If we want to be selective about ignoring
content length mismatches, we'll probably want to add a load flag which controls
who HttpStreamParser reacts to this issue.

[rvargas (doing something else), 2012-03-23 20:50:29]

I may be missing something.

https://chromiumcodereview.appspot.com/9811006/diff/9008/net/http/http_stream...
File net/http/http_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/9811006/diff/9008/net/http/http_stream...
net/http/http_stream_parser.cc:694: result = ERR_CONNECTION_CLOSED;
So we still return an error code at this layer in any case (contrary to the
comment)... and now there's not "fixup" for content length at the job layer...
so we're back to always fail.

[cbentzel, 2012-03-23 21:19:20]

https://chromiumcodereview.appspot.com/9811006/diff/9008/net/http/http_stream...
File net/http/http_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/9811006/diff/9008/net/http/http_stream...
net/http/http_stream_parser.cc:694: result = ERR_CONNECTION_CLOSED;
On 2012/03/23 20:50:29, rvargas wrote:
> So we still return an error code at this layer in any case (contrary to the
> comment)... and now there's not "fixup" for content length at the job layer...
> so we're back to always fail.

Oops, this was a bad local patch that I did to make sure that the tests failed
when it was not returning OK.

[cbentzel, 2012-03-23 21:21:50]

Wow, thanks for that catch. It would have been caught by trybots, but I
shouldn't have had it in there in the first place.

I also plan on landing this after branch point.

[rvargas (doing something else), 2012-03-24 01:06:50]

I would actually remove the browser test. We are removing high level "hacks",
and the test is not adding anything over the net_unittests.

LGTM

https://chromiumcodereview.appspot.com/9811006/diff/12027/net/http/http_netwo...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

https://chromiumcodereview.appspot.com/9811006/diff/12027/net/http/http_netwo...
net/http/http_network_transaction_spdy21_unittest.cc:6168: EXPECT_EQ(OK, rv);
This is an interesting case. SPDY is really at a different layer, so it makes
sense that it should not care about any content-length mismatch... however,
there are some security implications with content-length mismatches in SSL
connections... and it sort of makes sense to be able to spec SPDY to enforce
something here.

In other words, seems OK to land this as is, but explore if we should do
something about it at least for SPDY (being a new implementation for everyone).

[cbentzel, 2012-03-24 01:24:35]

https://chromiumcodereview.appspot.com/9811006/diff/12027/net/http/http_netwo...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

https://chromiumcodereview.appspot.com/9811006/diff/12027/net/http/http_netwo...
net/http/http_network_transaction_spdy21_unittest.cc:6168: EXPECT_EQ(OK, rv);
On 2012/03/24 01:06:50, rvargas wrote:
> This is an interesting case. SPDY is really at a different layer, so it makes
> sense that it should not care about any content-length mismatch... however,
> there are some security implications with content-length mismatches in SSL
> connections... and it sort of makes sense to be able to spec SPDY to enforce
> something here.
> 
> In other words, seems OK to land this as is, but explore if we should do
> something about it at least for SPDY (being a new implementation for
everyone).

Yes, this is interesting. I am concerned about web compatibility issues with
SPDY proxies forwarding data along from incorrect HTTP backends - in that case
we may want this behavior to be permitted.

[wtc1, 2012-03-26 21:37:01]

Drive-by review comments on patch set 11:

Please feel free to ignore my comments.

https://chromiumcodereview.appspot.com/9811006/diff/9010/net/http/http_stream...
File net/http/http_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/9811006/diff/9010/net/http/http_stream...
net/http/http_stream_parser.cc:675: //    tolerant of it, so this is reported as
OK.

If we report OK only for non-HTTPS, will that provide enough
compatibility?

https://chromiumcodereview.appspot.com/9811006/diff/9010/net/http/http_stream...
net/http/http_stream_parser.cc:692: result = ERR_INVALID_CHUNKED_ENCODING;

Perhaps ERR_INCOMPLETE_CHUNKED_ENCODING so we can tell it's
this case?

[cbentzel, 2012-03-27 17:57:42]

https://chromiumcodereview.appspot.com/9811006/diff/9010/net/http/http_stream...
File net/http/http_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/9811006/diff/9010/net/http/http_stream...
net/http/http_stream_parser.cc:675: //    tolerant of it, so this is reported as
OK.
On 2012/03/26 21:37:02, wtc1 wrote:
> 
> If we report OK only for non-HTTPS, will that provide enough
> compatibility?

We'd have to measure this. As discussed off of this code review, I'm going to
land another change which improves the measurement for how frequently this event
happens.