Report ERR_CONTENT_LENGTH_MISMATCH when the count of bytes received doesn't match Content-Length.

Report ERR_CONTENT_LENGTH_MISMATCH when the count of bytes received doesn't match Content-Length.

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 simply removed error reporting for this case. However, some uses may want to still treat this like an error (such as URLFetcher), or could eventually treat the case as an error and recover (such as downloads).

Data received will be populated in the disk cache as a truncated entry, if the server advertises byte range support.

WebURLLoaderImpl ignores this for all async and sync resource loads. At some point it may want to selectively do this based on content type.

Downloads ignore the error, but no longer ignore ERR_CONNECTION_CLOSED.

Other uses will treat this like an error.

BUG=52847
TEST=Manually, go to a site that advertises a large content-length and closes the connection before writing so many bytes, and notice that the page now displays.

Automatic: HttpNetworkTransactionTest.LargeContentLengthThenClose, URLRequestTestHttp.GetZippedTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110505

[cbentzel, 2011-11-08 22:29:51]

ahendrickson: review the download change, and url_request_http_job.

darin: review weburlloader_impl.cc

rvargas: The whole thing.

I will look at implementing a system test. 

The biggest question for me is the weburlloader_impl side. I considered
implementing the "ignore error" logic downstream in the ResourceLoader
derivative classes, but that would leak into WebKit and it would be at the time
didFail is called.

The other thought was to move the logic upstream to the ResourceDispatcher or
AsyncResourceHandler.

[ahendrickson, 2011-11-08 23:13:21]

LGTM.

[rvargas (doing something else), 2011-11-08 23:27:46]

http://codereview.chromium.org/8496016/diff/3009/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/8496016/diff/3009/net/http/http_stream_parser....
net/http/http_stream_parser.cc:537: if (result == 0 && !IsResponseBodyComplete()
&& CanFindEndOfResponse())
This not only includes content-length mismatch, but also problems detected by
the chunked_decoder. We should probably return ERR_CONNECTION_CLOSED for chunk
mismatches, or add a new error code.

[darin (slow to review), 2011-11-08 23:30:53]

http://codereview.chromium.org/8496016/diff/3009/webkit/glue/weburlloader_imp...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/8496016/diff/3009/webkit/glue/weburlloader_imp...
webkit/glue/weburlloader_impl.cc:606: net::URLRequestStatus status =
original_status;
what's the justification for doing this fixup at this level instead of closer to
the network stack?

[cbentzel, 2011-11-09 01:26:09]

http://codereview.chromium.org/8496016/diff/3009/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/8496016/diff/3009/net/http/http_stream_parser....
net/http/http_stream_parser.cc:537: if (result == 0 && !IsResponseBodyComplete()
&& CanFindEndOfResponse())
On 2011/11/08 23:27:47, rvargas wrote:
> This not only includes content-length mismatch, but also problems detected by
> the chunked_decoder. We should probably return ERR_CONNECTION_CLOSED for chunk
> mismatches, or add a new error code.

Good point. I could imagine a new error code. I'll adjust my test server to do
bad chunked encoding and see what the behavior is on other browsers before
making a decision here.

http://codereview.chromium.org/8496016/diff/3009/webkit/glue/weburlloader_imp...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/8496016/diff/3009/webkit/glue/weburlloader_imp...
webkit/glue/weburlloader_impl.cc:606: net::URLRequestStatus status =
original_status;
On 2011/11/08 23:30:53, darin wrote:
> what's the justification for doing this fixup at this level instead of closer
to
> the network stack?

We could potentially move this up to AsyncResourceHandler or ResourceDispatcher.
I wanted to move this as far down as possible to minimize the places which
ignore the error, but could generalize more.

The change needs to be done below URLRequest since we don't want to suppress the
error for some URLRequest::Delegate's such as URLFetcher.

It also needs to done below ResourceDispatcherHost because we'll eventually want
to pay attention to this error in DownloadResourceHandler.

[cbentzel, 2011-11-09 18:17:36]

http://codereview.chromium.org/8496016/diff/3009/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/8496016/diff/3009/net/http/http_stream_parser....
net/http/http_stream_parser.cc:537: if (result == 0 && !IsResponseBodyComplete()
&& CanFindEndOfResponse())
On 2011/11/09 01:26:09, cbentzel wrote:
> On 2011/11/08 23:27:47, rvargas wrote:
> > This not only includes content-length mismatch, but also problems detected
by
> > the chunked_decoder. We should probably return ERR_CONNECTION_CLOSED for
chunk
> > mismatches, or add a new error code.
> 
> Good point. I could imagine a new error code. I'll adjust my test server to do
> bad chunked encoding and see what the behavior is on other browsers before
> making a decision here.

I tested with FF4 and FF8. Need to test with other browsers.

If chunked-encoding advertises too few bytes in the chunk [i.e. advertises 8\r\n
but actually sends a 16 byte chunk] FF4 will not show the page but FF8 will.
Similar behavior exists for octet-streams.

If chunked-encoding advertised more bytes than are actually sent before the
connection closes, both FF4 and FF8 show whatever content was received.

I think it makes sense to report ERR_INVALID_CHUNKED_ENCODING here, and treat
like that an error. If we end up getting some pushback that this is hurting
users, we can ignore it for certain resource fetches as well.

[cbentzel, 2011-11-09 18:48:35]

http://codereview.chromium.org/8496016/diff/3009/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/8496016/diff/3009/net/http/http_stream_parser....
net/http/http_stream_parser.cc:537: if (result == 0 && !IsResponseBodyComplete()
&& CanFindEndOfResponse())
On 2011/11/09 18:17:36, cbentzel wrote:
> On 2011/11/09 01:26:09, cbentzel wrote:
> > On 2011/11/08 23:27:47, rvargas wrote:
> > > This not only includes content-length mismatch, but also problems detected
> by
> > > the chunked_decoder. We should probably return ERR_CONNECTION_CLOSED for
> chunk
> > > mismatches, or add a new error code.
> > 
> > Good point. I could imagine a new error code. I'll adjust my test server to
do
> > bad chunked encoding and see what the behavior is on other browsers before
> > making a decision here.
> 
> I tested with FF4 and FF8. Need to test with other browsers.

Safari 5.1.1 on Windows will fail if the Chunked-Encoding advertises too few or
too many bytes. However, it will not fail if the Content-Length is higher than
the number of bytes received. 

Now that this is reporting ERR_INVALID_CHUNKED_ENCODING for one case and
ERR_CONTENT_LENGTH_MISMATCH the behavior will match that.

> 
> If chunked-encoding advertises too few bytes in the chunk [i.e. advertises
8\r\n
> but actually sends a 16 byte chunk] FF4 will not show the page but FF8 will.
> Similar behavior exists for octet-streams.
> 
> If chunked-encoding advertised more bytes than are actually sent before the
> connection closes, both FF4 and FF8 show whatever content was received.
> 
> I think it makes sense to report ERR_INVALID_CHUNKED_ENCODING here, and treat
> like that an error. If we end up getting some pushback that this is hurting
> users, we can ignore it for certain resource fetches as well.

[rvargas (doing something else), 2011-11-09 19:55:26]

LGTM as long as Darin is happy with the location of the fixup.

http://codereview.chromium.org/8496016/diff/6002/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/8496016/diff/6002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:539: result = ERR_INVALID_CHUNKED_ENCODING;
Note that this means we'll be using the same error code for some random stuff in
the middle of the format, and a dropped connection. That's probably OK, I just
wanted to point that out because there's a difference between "all I have
received so far may be crap" vs "I may lack the end of this doc" (maybe add a
comment about it).

[cbentzel, 2011-11-09 20:40:53]

http://codereview.chromium.org/8496016/diff/6002/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/8496016/diff/6002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:539: result = ERR_INVALID_CHUNKED_ENCODING;
On 2011/11/09 19:55:26, rvargas wrote:
> Note that this means we'll be using the same error code for some random stuff
in
> the middle of the format, and a dropped connection. That's probably OK, I just
> wanted to point that out because there's a difference between "all I have
> received so far may be crap" vs "I may lack the end of this doc" (maybe add a
> comment about it).

I think it's OK to lump the chunked encoding error causes together. Also updated
the comment above this block of code to cover the rationale behind the different
return values.

[cbentzel, 2011-11-14 15:41:38]

darin: ping

[darin (slow to review), 2011-11-16 21:59:57]

http://codereview.chromium.org/8496016/diff/8002/webkit/glue/weburlloader_imp...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/8496016/diff/8002/webkit/glue/weburlloader_imp...
webkit/glue/weburlloader_impl.cc:612:
status.set_status(net::URLRequestStatus::SUCCESS);
should you also clear the error field here for completeness?

[cbentzel, 2011-11-16 22:19:28]

http://codereview.chromium.org/8496016/diff/8002/webkit/glue/weburlloader_imp...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/8496016/diff/8002/webkit/glue/weburlloader_imp...
webkit/glue/weburlloader_impl.cc:612:
status.set_status(net::URLRequestStatus::SUCCESS);
On 2011/11/16 21:59:57, darin wrote:
> should you also clear the error field here for completeness?

Done. The code below doesn't investigate error if status is SUCCESS, but that
may drift over time. This almost seems like it should be an invariant for
URLRequestStatus.

[darin (slow to review), 2011-11-17 05:05:03]

On Wed, Nov 16, 2011 at 2:19 PM, <cbentzel@chromium.org> wrote:

>
> http://codereview.chromium.**org/8496016/diff/8002/webkit/**
>
glue/weburlloader_impl.cc<http://codereview.chromium.org/8496016/diff/8002/webkit/glue/weburlloader_impl.cc>
> File webkit/glue/weburlloader_impl.**cc (right):
>
> http://codereview.chromium.**org/8496016/diff/8002/webkit/**
>
glue/weburlloader_impl.cc#**newcode612<http://codereview.chromium.org/8496016/diff/8002/webkit/glue/weburlloader_impl.cc#newcode612>
> webkit/glue/weburlloader_impl.**cc:612:
> status.set_status(net::**URLRequestStatus::SUCCESS);
> On 2011/11/16 21:59:57, darin wrote:
>
>> should you also clear the error field here for completeness?
>>
>
> Done. The code below doesn't investigate error if status is SUCCESS, but
> that may drift over time. This almost seems like it should be an
> invariant for URLRequestStatus.
>

agreed

anyways, lgtm for the weburlloader_impl.cc change.


>
>
http://codereview.chromium.**org/8496016/<http://codereview.chromium.org/8496...
>

[commit-bot: I haz the power, 2011-11-17 14:43:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/8496016/12002

[commit-bot: I haz the power, 2011-11-17 16:24:50]

Change committed as 110505