Don't read message-body for HEAD responses.

Don't read message-body for HEAD responses.

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

[eroman, 2009-03-05 11:57:19]

Based on the timestamp of the bug, I expect this repros for winhttp stack as
well. (I have only investigated relative to newhttp so can't say for sure.)

[wtc, 2009-03-05 18:12:54]

LGTM!  Thanks for fixing this.

http://codereview.chromium.org/40161/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/40161/diff/1/2#newcode1026
Line 1026: // For certain responses, we know the content length is always 0.
Good catch.  You may want to quote the relevant text from
this paragraph from RFC 2616 Section 4.3 Message Body:

   For response messages, whether or not a message-body is included with
   a message is dependent on both the request method and the response
   status code (section 6.1.1). All responses to the HEAD request method
   MUST NOT include a message-body, even though the presence of entity-
   header fields might lead one to believe they do. All 1xx
   (informational), 204 (no content), and 304 (not modified) responses
   MUST NOT include a message-body. All other responses do include a
   message-body, although it MAY be of zero length.

At least, we should find out why we have 205 here but don't
have 1xx.  Section 10.2.6 205 Reset Content says:

   ... The response MUST NOT include an entity.

So I believe it is correct to have 205 here (no entity
seems to be stronger than no message-body).  So we just
need to find out if we should add 1xx here.

http://codereview.chromium.org/40161/diff/1/2#newcode1034
Line 1034: if (request_->method == "HEAD")
Perhaps the content_length_ header should be renamed
message_body_length_ to avoid confusion, because it is
legal for a HEAD response to have a Content-Length header
(one of the entity-headers that RFC 2616 Section 4.3 refers
to).

http://codereview.chromium.org/40161/diff/1/3
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/40161/diff/1/3#newcode428
Line 428: MockRead("HTTP/1.1 404 OK\r\n"),
Nit: "404 OK" => "404 Not Found" or "200 OK"

[eroman, 2009-03-05 22:37:28]

http://codereview.chromium.org/40161/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/40161/diff/1/2#newcode1026
Line 1026: // For certain responses, we know the content length is always 0.
On 2009/03/05 18:12:54, wtc wrote:
> Good catch.  You may want to quote the relevant text from
> this paragraph from RFC 2616 Section 4.3 Message Body:
> 
>    For response messages, whether or not a message-body is included with
>    a message is dependent on both the request method and the response
>    status code (section 6.1.1). All responses to the HEAD request method
>    MUST NOT include a message-body, even though the presence of entity-
>    header fields might lead one to believe they do. All 1xx
>    (informational), 204 (no content), and 304 (not modified) responses
>    MUST NOT include a message-body. All other responses do include a
>    message-body, although it MAY be of zero length.
> 
> At least, we should find out why we have 205 here but don't
> have 1xx.  Section 10.2.6 205 Reset Content says:
> 
>    ... The response MUST NOT include an entity.
> 
> So I believe it is correct to have 205 here (no entity
> seems to be stronger than no message-body).  So we just
> need to find out if we should add 1xx here.

Done.

Pasted in that quote from RFC2616

I haven't thought about the 205/1xx cases yet -- I will file a bug and do this
as a post-step.

http://codereview.chromium.org/40161/diff/1/2#newcode1034
Line 1034: if (request_->method == "HEAD")
On 2009/03/05 18:12:54, wtc wrote:
> Perhaps the content_length_ header should be renamed
> message_body_length_ to avoid confusion, because it is
> legal for a HEAD response to have a Content-Length header
> (one of the entity-headers that RFC 2616 Section 4.3 refers
> to).

Done.

Renamed |content_length_| to |message_body_length_|.

Should I also rename |content_read_| to |message_body_read_| for consistency?

http://codereview.chromium.org/40161/diff/1/3
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/40161/diff/1/3#newcode428
Line 428: MockRead("HTTP/1.1 404 OK\r\n"),
On 2009/03/05 18:12:54, wtc wrote:
> Nit: "404 OK" => "404 Not Found" or "200 OK"

Ok, Done.

[wtc, 2009-03-05 22:53:06]

On 2009/03/05 22:37:28, eroman wrote:
> 
> Renamed |content_length_| to |message_body_length_|.
> 
> Should I also rename |content_read_| to |message_body_read_| for consistency?

Yes.  On second thought, let's name these members
response_body_length_ and response_body_read_, because
a request may also have a message-body.

You can check in after the renaming without another
review by me.  LGTM!

Re: 1xx: it's possible that we won't reach that place
on a 1xx response.  Good idea to look at that separately.

[eroman, 2009-03-05 23:10:31]

> Yes.  On second thought, let's name these members
> response_body_length_ and response_body_read_, because
> a request may also have a message-body.

Done. Excellent name suggestions.

[wtc, 2009-03-05 23:27:39]

LGTM.