Treat ERR_CONNECTION_CLOSED as end-of-data marker for downloads.

Treat ERR_CONNECTION_CLOSED as end-of-data marker for downloads.

BUG=79694
TEST=Download a torrent file from http://ca.isohunt.com.  It should complete.


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

[ahendrickson, 2011-04-21 17:08:24]

I thought about putting code into HttpStreamParser::DoReadBodyComplete() to deal
with this, but I don't know enough about other systems to figure out what is the
right thing to do.

Would setting the error to 0 work in general?

[darin (slow to review), 2011-04-21 17:13:38]

I'm pretty sure Mozilla handles this transparently at the HTTP layer.  I
think we should
do the same unless we have evidence that it is problematic.

I think the bug you are hitting here could easily impact other subsystems
that use
the network stack to fetch URLs.  Why should we have to fix this quirk N
times?

-Darin


On Thu, Apr 21, 2011 at 10:08 AM, <ahendrickson@chromium.org> wrote:

> I thought about putting code into HttpStreamParser::DoReadBodyComplete() to
> deal
> with this, but I don't know enough about other systems to figure out what
> is the
> right thing to do.
>
> Would setting the error to 0 work in general?
>
>
>
> http://codereview.chromium.org/6881106/
>

[cbentzel, 2011-04-21 17:16:41]

http://codereview.chromium.org/6881106/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6881106/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:547: if ((os_error == 0) ||
(os_error == net::ERR_CONNECTION_CLOSED)) {
We discussed doing this at a lower-layer [in HttpStreamRequest] only in the case
when the response has Connection: close, and not returning ERR_CONNECTION_CLOSED
in that case even if bytes read in body did not equal Content-Length. There's
likely other cases which will result in ERR_CONNECTION_CLOSED here [i.e. it's a
Keep-Alive connection but there's a TCP RST].

You should take a look at the Firefox code to see how they handle that case as
well.

[ahendrickson, 2011-05-02 18:27:20]

Moved the detection code to URLRequestJob.

[ahendrickson, 2011-05-03 20:30:53]

Ping

[Randy Smith (Not in Mondays), 2011-05-03 21:30:41]

The quick summary from me is that the substantive changes look good, but there
strikes me as being a fair amount in this CL that doesn't seem to belong.  Let
me know if you disagree.

http://codereview.chromium.org/6881106/diff/11001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.cc (left):

http://codereview.chromium.org/6881106/diff/11001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.cc:452: DVLOG(3) << "Get
store for list: " << list_id;
I doubt this is part of your CL, so you probably don't want to change this file?

http://codereview.chromium.org/6881106/diff/11001/content/browser/renderer_ho...
File content/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/6881106/diff/11001/content/browser/renderer_ho...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:71: LOG(ERROR) <<
"NoSocketId in OnSentData";
Ditto.  This is a fine fix, but I don't think it belongs as part of this CL.

http://codereview.chromium.org/6881106/diff/11001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6881106/diff/11001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:74: << " headers = \"" << headers.ToString() <<
"\"";
I'd be inclined to separate out pure logging changes if they're the only changes
in a file, and I think the only changes in this file are for logging.

http://codereview.chromium.org/6881106/diff/11001/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6881106/diff/11001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:291: // See bug: 79694.
I think the proper format for this last is "See http://crbug.com/79694".

http://codereview.chromium.org/6881106/diff/11001/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/6881106/diff/11001/net/url_request/url_request...
net/url_request/url_request_job.cc:333: << " total = " <<
postfilter_bytes_read_;
This seems way too chatty; if someone does --v=1, every time a URLRequest read
completes we'll get a log entry?

[ahendrickson, 2011-05-03 21:41:19]

+shess for the removal of a (massively outputting) log in the safe browsing
code.

[Scott Hess - ex-Googler, 2011-05-03 21:50:29]

http://codereview.chromium.org/6881106/diff/11001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.cc (left):

http://codereview.chromium.org/6881106/diff/11001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.cc:452: DVLOG(3) << "Get
store for list: " << list_id;
If this is the log line you refer to in the CC to me, then LGTM to remove.

BTW, I'd generally un-support adding new log lines unless you're convinced that
they would be totally useful for cases where you can't just re-compile Chromium.
 They may-or-may-not be cheap in execution time, but often enough the log line
which was obvious at point of insertion isn't obvious six months later when
someone is debugging things, so it's just a bunch of extra noise to read through
in the code.

[ahendrickson, 2011-05-04 15:08:24]

http://codereview.chromium.org/6881106/diff/11001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.cc (left):

http://codereview.chromium.org/6881106/diff/11001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.cc:452: DVLOG(3) << "Get
store for list: " << list_id;
On 2011/05/03 21:30:41, rdsmith wrote:
> I doubt this is part of your CL, so you probably don't want to change this
file?

Since all our debugging happens at VLOG(20), this outputs a massive amount of
logging (making it look like Chrome is hung) when I try to debug.

I'd be happy to leave it in at VLOG(30) or something like that, if people think
it's necessary.

Oh, and list_id is always 1.

http://codereview.chromium.org/6881106/diff/11001/content/browser/renderer_ho...
File content/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/6881106/diff/11001/content/browser/renderer_ho...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:71: LOG(ERROR) <<
"NoSocketId in OnSentData";
On 2011/05/03 21:30:41, rdsmith wrote:
> Ditto.  This is a fine fix, but I don't think it belongs as part of this CL.

If I don't do it in this CL, I'll forget about it.  If someone else wants to do
it, I'm fine with that.

http://codereview.chromium.org/6881106/diff/11001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6881106/diff/11001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:74: << " headers = \"" << headers.ToString() <<
"\"";
On 2011/05/03 21:30:41, rdsmith wrote:
> I'd be inclined to separate out pure logging changes if they're the only
changes
> in a file, and I think the only changes in this file are for logging.

I find myself putting these things in every time I want to debug something in
the downloads!  It's much easier than searching in the about:net-internals for
the matching section (and fewer steps to get the info).

There are only 2 logging statements, one for the request and one for the
response.

http://codereview.chromium.org/6881106/diff/11001/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6881106/diff/11001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:291: // See bug: 79694.
On 2011/05/03 21:30:41, rdsmith wrote:
> I think the proper format for this last is "See http://crbug.com/79694%22.

Done.

http://codereview.chromium.org/6881106/diff/11001/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/6881106/diff/11001/net/url_request/url_request...
net/url_request/url_request_job.cc:333: << " total = " <<
postfilter_bytes_read_;
On 2011/05/03 21:30:41, rdsmith wrote:
> This seems way too chatty; if someone does --v=1, every time a URLRequest read
> completes we'll get a log entry?

Oops, that was an oversight.  Changed it to VLOG(20).

[Randy Smith (Not in Mondays), 2011-05-04 20:31:37]

LGTM.

[ahendrickson, 2011-05-06 17:11:21]

Ping

[ahendrickson, 2011-05-06 20:47:54]

Adding Will Chan.

[ahendrickson, 2011-05-16 14:52:56]

Ping

[darin (slow to review), 2011-05-16 17:47:16]

http://codereview.chromium.org/6881106/diff/18001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6881106/diff/18001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:562: static std::string
GetResponseHeaderLines(const HttpResponseHeaders& headers) {
it is usually bad form to stick a non-class function in the middle of a series
of class function definitions.  it breaks up the organization of the file.

i think it is cleaner to uplift helper functions to a section at the top of the
file.  this could also be a method on HttpResponseHeaders.

http://codereview.chromium.org/6881106/diff/18001/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/6881106/diff/18001/net/url_request/url_request...
net/url_request/url_request_job.cc:330: VLOG(20) << __FUNCTION__ << "() "
would it be more helpful if this were output to the net log instead?  same goes
for other VLOGs.

[ahendrickson, 2011-05-16 19:37:40]

Darin, could you take a look at the code in url_request_http_job.cc?

Thanks,

Andy

http://codereview.chromium.org/6881106/diff/18001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6881106/diff/18001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:562: static std::string
GetResponseHeaderLines(const HttpResponseHeaders& headers) {
On 2011/05/16 17:47:16, darin wrote:
> it is usually bad form to stick a non-class function in the middle of a series
> of class function definitions.  it breaks up the organization of the file.
> 
> i think it is cleaner to uplift helper functions to a section at the top of
the
> file.  this could also be a method on HttpResponseHeaders.

Moved to the top of the file.

http://codereview.chromium.org/6881106/diff/18001/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/6881106/diff/18001/net/url_request/url_request...
net/url_request/url_request_job.cc:330: VLOG(20) << __FUNCTION__ << "() "
On 2011/05/16 17:47:16, darin wrote:
> would it be more helpful if this were output to the net log instead?  same
goes
> for other VLOGs.

I find it useful to use VLOG, as I have a lot of other debugging information
that uses it (which is not appropriate for net-internals).  This way I don't
have to search in two different locations for the information I want, and it's
serial in time).

I can add a net-internals log statement if you think it would be useful.

[ahendrickson, 2011-05-16 19:38:49]

I believe that FireFox handles this case in:

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpCon...

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

On Mon, May 16, 2011 at 12:37 PM, <ahendrickson@chromium.org> wrote:

> Darin, could you take a look at the code in url_request_http_job.cc?
>
> Thanks,
>
> Andy
>
>
>
>
>
>
http://codereview.chromium.org/6881106/diff/18001/net/http/http_stream_parser.cc
> File net/http/http_stream_parser.cc (right):
>
>
>
http://codereview.chromium.org/6881106/diff/18001/net/http/http_stream_parser...
> net/http/http_stream_parser.cc:562: static std::string
> GetResponseHeaderLines(const HttpResponseHeaders& headers) {
> On 2011/05/16 17:47:16, darin wrote:
>
>> it is usually bad form to stick a non-class function in the middle of
>>
> a series
>
>> of class function definitions.  it breaks up the organization of the
>>
> file.
>
>  i think it is cleaner to uplift helper functions to a section at the
>>
> top of the
>
>> file.  this could also be a method on HttpResponseHeaders.
>>
>
> Moved to the top of the file.


thanks


>
>
>
>
http://codereview.chromium.org/6881106/diff/18001/net/url_request/url_request...
> File net/url_request/url_request_job.cc (right):
>
>
>
http://codereview.chromium.org/6881106/diff/18001/net/url_request/url_request...
> net/url_request/url_request_job.cc:330: VLOG(20) << __FUNCTION__ << "()
> "
> On 2011/05/16 17:47:16, darin wrote:
>
>> would it be more helpful if this were output to the net log instead?
>>
> same goes
>
>> for other VLOGs.
>>
>
> I find it useful to use VLOG, as I have a lot of other debugging
> information that uses it (which is not appropriate for net-internals).
> This way I don't have to search in two different locations for the
> information I want, and it's serial in time).
>

i'm not at all against using base/logging.h... sorry for not being more
clear.  my
comment was more motivated by observing that are using the non-debug version
of VLOG.  if you want this to appear in a release build, then presumably you
want
to be able to debug end-user systems using this logging.  but that makes me
think about net-internals logging.

it seems to me that we should prefer debug-only logging unless we have
reason to
make something available in release mode.

-darin



>
> I can add a net-internals log statement if you think it would be useful.
>
>
> http://codereview.chromium.org/6881106/
>

[Paweł Hajdan Jr., 2011-05-17 20:05:18]

I'm not familiar enough with this code to review it, removing myself from the
list.

[ahendrickson, 2011-05-18 16:10:35]

Fixed issues with the accounting for pre and post filter bytes.

[ahendrickson, 2011-05-18 23:05:50]

rvargas, avi, and willchan, could you take a look at this?

Thanks,

Andy

[rvargas (doing something else), 2011-05-19 19:30:36]

We need a unit test at the URLRequestJob layer.

http://codereview.chromium.org/6881106/diff/35002/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6881106/diff/35002/net/http/http_stream_parser...
net/http/http_stream_parser.cc:22: static std::string GetResponseHeaderLines(
nit: can you use an anonymous namespace instead?

http://codereview.chromium.org/6881106/diff/35002/net/http/http_stream_parser...
net/http/http_stream_parser.cc:31: header_line += strlen(header_line) + 1;
It probably doesn't matter too much, but it looks like you are going twice over
header_line looking for nulls, and building a temp string anyway. I'd leave less
work for the optimizer.

http://codereview.chromium.org/6881106/diff/35002/net/http/http_stream_parser...
net/http/http_stream_parser.cc:86: VLOG(20) << __FUNCTION__ << "()"
This looks odd. Could you use DVLOG instead?. Having random numbers like 20
makes it pretty hard to know what to look for unless one finds this line in the
code first. Also, this is pretty much the same thing that goes to the
net_internals log, and there is a command line option to direct the
net_internals log to the regular log output.

http://codereview.chromium.org/6881106/diff/35002/net/http/http_stream_parser...
net/http/http_stream_parser.cc:639: VLOG(20) << __FUNCTION__ << "()"
same here (and other places)

http://codereview.chromium.org/6881106/diff/35002/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6881106/diff/35002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:302: prefilter_bytes_read() ==
expected_length) {
if expected_length is 0 or prefilter_bytes_read we should not be clearing the
error, because those conditions should not be generating the error in the first
place (or at least not the one that we want to mask).

http://codereview.chromium.org/6881106/diff/35002/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/6881106/diff/35002/net/url_request/url_request...
net/url_request/url_request_job.cc:591: if (*bytes_read > 0) {
You cannot evaluate bytes_read without first looking at the return value of
ReadRawData. Why not do this on OnRawReadComplete/RecordBytesRead?

[ahendrickson, 2011-05-19 22:29:33]

Still looking at unit tests (url_request_unittest.cc).

http://codereview.chromium.org/6881106/diff/35002/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6881106/diff/35002/net/http/http_stream_parser...
net/http/http_stream_parser.cc:22: static std::string GetResponseHeaderLines(
On 2011/05/19 19:30:36, rvargas wrote:
> nit: can you use an anonymous namespace instead?

Done.

http://codereview.chromium.org/6881106/diff/35002/net/http/http_stream_parser...
net/http/http_stream_parser.cc:86: VLOG(20) << __FUNCTION__ << "()"
On 2011/05/19 19:30:36, rvargas wrote:
> This looks odd. Could you use DVLOG instead?. Having random numbers like 20
> makes it pretty hard to know what to look for unless one finds this line in
the
> code first. Also, this is pretty much the same thing that goes to the
> net_internals log, and there is a command line option to direct the
> net_internals log to the regular log output.

Switched to DVLOG.

20 is the number we use for debugging downloads.  We figured it wouldn't
interfere with others' debugging $-).

I did not know about the command-line option (log-net-log).  Just tried it, and
it has a LOT of output, so it's harder for me to read and not as helpful as this
code.

http://codereview.chromium.org/6881106/diff/35002/net/http/http_stream_parser...
net/http/http_stream_parser.cc:639: VLOG(20) << __FUNCTION__ << "()"
On 2011/05/19 19:30:36, rvargas wrote:
> same here (and other places)

Done.

http://codereview.chromium.org/6881106/diff/35002/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6881106/diff/35002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:302: prefilter_bytes_read() ==
expected_length) {
On 2011/05/19 19:30:36, rvargas wrote:
> if expected_length is 0 or prefilter_bytes_read we should not be clearing the
> error, because those conditions should not be generating the error in the
first
> place (or at least not the one that we want to mask).

Done.

http://codereview.chromium.org/6881106/diff/35002/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/6881106/diff/35002/net/url_request/url_request...
net/url_request/url_request_job.cc:591: if (*bytes_read > 0) {
On 2011/05/19 19:30:36, rvargas wrote:
> You cannot evaluate bytes_read without first looking at the return value of
> ReadRawData. Why not do this on OnRawReadComplete/RecordBytesRead?

Done.

[rvargas (doing something else), 2011-05-21 01:24:49]

http://codereview.chromium.org/6881106/diff/40001/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/6881106/diff/40001/net/url_request/url_request...
net/url_request/url_request_job.cc:327: VLOG(21) << __FUNCTION__ << "() "
DVLOG. I'm still not too happy about the 21... it's not like these files have
DVLOG at any level so it goes from 0 to level 21.

Correct me if I'm wrong, but the idea behind VLOG is to filter features based on
file name, not that a given feature will have a log level. In other words,
activating log at level 21 without setting a filter on file names will just give
you every possible log from everybody (unless somebody decided to use 40 or
something like that), so I just don't get your comment about not interfering
with somebody else.

Even if the downloads code uses level 20/21, it looks inappropriate to expect
other random files to follow the same convention. To me, level 21 means that I
should expect (at some point) 20 progressive levels of more and more logging
(from the same file) below the current level.

[ahendrickson, 2011-05-22 06:43:34]

Now have a unit test.

http://codereview.chromium.org/6881106/diff/40001/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/6881106/diff/40001/net/url_request/url_request...
net/url_request/url_request_job.cc:327: VLOG(21) << __FUNCTION__ << "() "
On 2011/05/21 01:24:50, rvargas wrote:
> DVLOG. I'm still not too happy about the 21... it's not like these files have
> DVLOG at any level so it goes from 0 to level 21.
> 
> Correct me if I'm wrong, but the idea behind VLOG is to filter features based
on
> file name, not that a given feature will have a log level. In other words,
> activating log at level 21 without setting a filter on file names will just
give
> you every possible log from everybody (unless somebody decided to use 40 or
> something like that), so I just don't get your comment about not interfering
> with somebody else.
> 
> Even if the downloads code uses level 20/21, it looks inappropriate to expect
> other random files to follow the same convention. To me, level 21 means that I
> should expect (at some point) 20 progressive levels of more and more logging
> (from the same file) below the current level.

Reduced the number to 2 (in case anyone adds VLOGs and doesn't want to be
spammed with this.

http://codereview.chromium.org/6881106/diff/42001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/6881106/diff/42001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:806: #    self.send_header('Content-type',
'gzip')
Removing these commented-out lines . . .

[ahendrickson, 2011-05-23 21:23:23]

Ping . . .

[ahendrickson, 2011-05-24 04:58:37]

http://codereview.chromium.org/6881106/diff/50001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6881106/diff/50001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:646: << " headers = \"" <<
GetResponseHeaderLines(*response_->headers)
On 2011/05/23 22:12:26, willchan wrote:
> Nit: Why not just use response_->headers.raw_headers()? Is the output not
clean
> enough? It'd save adding this extra function that's only used in debug mode.

Because the headers are separated by NULL characters, so you would only display
the first one.

http://codereview.chromium.org/6881106/diff/50001/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6881106/diff/50001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:338: status = URLRequestStatus();
On 2011/05/23 22:12:26, willchan wrote:
> I'd prefer if you explicitly set the fields to OK. It's not obvious that the
> default constructor will do this.

OK, but this is the way it's done in the rest of the file.

http://codereview.chromium.org/6881106/diff/50001/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/6881106/diff/50001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:442: (sizeof(test_parameters) /
sizeof(test_parameters[0])) - 1;  // Skip NULL.
On 2011/05/23 22:12:26, willchan wrote:
> Use arraysize() instead.

Done.

[Avi (use Gerrit), 2011-05-24 18:00:40]

LGTM

[rvargas (doing something else), 2011-05-24 18:25:34]

LGTM

http://codereview.chromium.org/6881106/diff/53003/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/6881106/diff/53003/net/tools/testserver/testse...
net/tools/testserver/testserver.py:811: self.send_header('ETag', '\'' +
file_path + '\'')
nit: make sure the protocol version is 1.1 or don't use 1.1 features.

[commit-bot: I haz the power, 2011-05-24 20:14:43]

Change committed as 86472