Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25312002:
net: don't preserve 1xx responses in parser buffer. (Closed)
|
Created:
7 years, 2 months ago by agl Modified:
7 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionnet: don't preserve 1xx responses in parser buffer.
It's unclear why 1xx responses were kept in the parser's buffer previously so
this change aligns their processing with all other response types.
BUG=299892
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226539
Patch Set 1 #
Total comments: 16
Patch Set 2 : ... #
Total comments: 4
Patch Set 3 : g try #
Total comments: 12
Patch Set 4 : Address wtc's comments. #
Total comments: 2
Patch Set 5 : .. #
Messages
Total messages: 14 (0 generated)
LGTM, with a few remarks. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:307: DCHECK_EQ(0, read_buf_unused_offset_); Should we move this DCHECK to the start of the function? https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:584: DCHECK_EQ(0, read_buf_unused_offset_); Should we move this DCHECK to the start of the function? https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:591: read_buf_unused_offset_ = end_of_header_offset; This is only needed if the response contains a body. Otherwise the value gets set to 0. It should therefore probably be moved down to just above line 620, before "io_state_ = STATE_BODY_PENDING;". https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:761: DCHECK_EQ(0, read_buf_unused_offset_); Should we move this DCHECK to the start of the function? https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:767: DCHECK_EQ(0, read_buf_unused_offset_); Should we move this DCHECK to the start of the function? https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:783: DCHECK_EQ(0, read_buf_unused_offset_); Should we move this DCHECK to the start of the function? https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:791: read_buf_->StartOfBuffer() + read_buf_unused_offset_, end_offset)); "read_buf_unused_offset_" should be removed here too. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.h File net/http/http_stream_parser.h (right): https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.h... net/http/http_stream_parser.h:183: int read_buf_unused_offset_; This may be a stretch, but since this is only used to refer to the start of the body in the buffer, could we: - rename this "read_buf_body_offset_", - set it to some invalid constant (eg. -1) while reading headers, - set it to a non-zero positive offset only once headers have been read and the response is expected to contain a body? I think that would make the code easier to understand and the DCHECKS more obvious.
I've checked that tunnel establishment still fails with a 1xx response and have added a unittest to check that. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:307: DCHECK_EQ(0, read_buf_unused_offset_); On 2013/09/30 18:27:47, SkyLined wrote: > Should we move this DCHECK to the start of the function? Done. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:584: DCHECK_EQ(0, read_buf_unused_offset_); On 2013/09/30 18:27:47, SkyLined wrote: > Should we move this DCHECK to the start of the function? Done. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:591: read_buf_unused_offset_ = end_of_header_offset; On 2013/09/30 18:27:47, SkyLined wrote: > This is only needed if the response contains a body. Otherwise the value gets > set to 0. It should therefore probably be moved down to just above line 620, > before "io_state_ = STATE_BODY_PENDING;". Done. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:761: DCHECK_EQ(0, read_buf_unused_offset_); On 2013/09/30 18:27:47, SkyLined wrote: > Should we move this DCHECK to the start of the function? Done. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:767: DCHECK_EQ(0, read_buf_unused_offset_); On 2013/09/30 18:27:47, SkyLined wrote: > Should we move this DCHECK to the start of the function? Done. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:783: DCHECK_EQ(0, read_buf_unused_offset_); On 2013/09/30 18:27:47, SkyLined wrote: > Should we move this DCHECK to the start of the function? Done. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.c... net/http/http_stream_parser.cc:791: read_buf_->StartOfBuffer() + read_buf_unused_offset_, end_offset)); On 2013/09/30 18:27:47, SkyLined wrote: > "read_buf_unused_offset_" should be removed here too. Done. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.h File net/http/http_stream_parser.h (right): https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.h... net/http/http_stream_parser.h:183: int read_buf_unused_offset_; On 2013/09/30 18:27:47, SkyLined wrote: > This may be a stretch, but since this is only used to refer to the start of the > body in the buffer, could we: > - rename this "read_buf_body_offset_", > - set it to some invalid constant (eg. -1) while reading headers, > - set it to a non-zero positive offset only once headers have been read and the > response is expected to contain a body? > I think that would make the code easier to understand and the DCHECKS more > obvious. I haven't done this for the following reasons: 1) It's not the offset of the body, it's the current offset: it's moved as we read the body in DoReadBody. I would expect the proposed name to be used for a variable that's constant. 2) I haven't switched the magic value to -1 because DoReadHeadersComplete can move to STATE_READ_BODY_COMPLETE in the event of truncated headers and DoReadBodyComplete does arithmetic with read_buf_unused_offset_ which is easier if it's zero in the unused case.
LGTM, with minor remarks https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parse... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parse... net/http/http_stream_parser.cc:619: } else { Nit: coding style says don't use else after return https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parse... net/http/http_stream_parser.cc:784: DCHECK_EQ(0, read_buf_unused_offset_); Since this check is already done at the start of this function and the value isn't changed in the function, do we need another check here?
https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parse... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parse... net/http/http_stream_parser.cc:619: } else { On 2013/09/30 22:02:55, SkyLined wrote: > Nit: coding style says don't use else after return Done. https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parse... net/http/http_stream_parser.cc:784: DCHECK_EQ(0, read_buf_unused_offset_); On 2013/09/30 22:02:55, SkyLined wrote: > Since this check is already done at the start of this function and the value > isn't changed in the function, do we need another check here? Done.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Patch set 3 LGTM. I can't read the bug report, so I only read the CL. https://codereview.chromium.org/25312002/diff/13001/net/http/http_proxy_clien... File net/http/http_proxy_client_socket_pool_unittest.cc (right): https://codereview.chromium.org/25312002/diff/13001/net/http/http_proxy_clien... net/http/http_proxy_client_socket_pool_unittest.cc:579: "Proxy-Authorization: Basic Zm9vOmJhcg==\r\n\r\n"), Nit: Is the Proxy-Authorization header (and the AddAuthToCache() call) important to this unit test? If not, we can omit it. https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:592: // If the body is 0, the caller may not call ReadResponseBody, which 1. Nit: "If the body is 0" isn't clear. I think it means "If the body is 0 bytes" or "If the body is of zero length". (This comment comes from the original code.) 2. It seems that this bug could also affect the other response codes that have a zero-length response body, such as 204 and 304. I think it's the fact that we transition to STATE_DONE for those response codes that make them immune to this bug. Correct? https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:599: memmove(read_buf_->StartOfBuffer(), IMPORTANT: is it correct to move the extra data for 1xx responses? The original code doesn't do that. https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:604: read_buf_unused_offset_ = 0; This is not necessary in the new code. (It was necessary in the original code.) https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:607: // header, so reset state to support that. We don't completely ignore a Nit: delete "a" at the end of this line. https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:613: response_body_length_ = -1; Why do we need to change response_body_length_ to -1 here?
Another nit: the CL's description says "don't preserve 1xx responses in parser buffer". Since 1xx responses do not have a message-body, "1xx responses" is a little confusing. Are you referring to the final response after the 1xx response?
https://codereview.chromium.org/25312002/diff/13001/net/http/http_proxy_clien... File net/http/http_proxy_client_socket_pool_unittest.cc (right): https://codereview.chromium.org/25312002/diff/13001/net/http/http_proxy_clien... net/http/http_proxy_client_socket_pool_unittest.cc:579: "Proxy-Authorization: Basic Zm9vOmJhcg==\r\n\r\n"), On 2013/10/01 18:12:57, wtc wrote: > > Nit: Is the Proxy-Authorization header (and the AddAuthToCache() call) important > to this unit test? If not, we can omit it. Done. https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:592: // If the body is 0, the caller may not call ReadResponseBody, which On 2013/10/01 18:12:57, wtc wrote: > 1. Nit: "If the body is 0" isn't clear. I think it means "If the body is 0 > bytes" or "If the body is of zero length". (This comment comes from the original > code.) Fixed. > 2. It seems that this bug could also affect the other response codes that have a > zero-length response body, such as 204 and 304. I think it's the fact that we > transition to STATE_DONE for those response codes that make them immune to this > bug. Correct? I don't think that's the case. (You should be able to see the bug now.) https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:599: memmove(read_buf_->StartOfBuffer(), On 2013/10/01 18:12:57, wtc wrote: > > IMPORTANT: is it correct to move the extra data for 1xx responses? The original > code doesn't do that. (see bug.) https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:604: read_buf_unused_offset_ = 0; On 2013/10/01 18:12:57, wtc wrote: > > This is not necessary in the new code. (It was necessary in the original code.) Done. https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:607: // header, so reset state to support that. We don't completely ignore a On 2013/10/01 18:12:57, wtc wrote: > > Nit: delete "a" at the end of this line. Done. https://codereview.chromium.org/25312002/diff/13001/net/http/http_stream_pars... net/http/http_stream_parser.cc:613: response_body_length_ = -1; On 2013/10/01 18:12:57, wtc wrote: > > Why do we need to change response_body_length_ to -1 here? It will have been set to 0 by CalculateResponseBodySize() and 0 is a valid value so, for the next response, it won't be updated again unless it's set to -1 here.
Patch set 4 LGTM. https://codereview.chromium.org/25312002/diff/44001/net/http/http_stream_pars... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/44001/net/http/http_stream_pars... net/http/http_stream_parser.cc:607: // 1xx response because they cannot be returned in reply to a CONNECT Nit: they => it ?
https://codereview.chromium.org/25312002/diff/44001/net/http/http_stream_pars... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/44001/net/http/http_stream_pars... net/http/http_stream_parser.cc:607: // 1xx response because they cannot be returned in reply to a CONNECT On 2013/10/01 21:49:28, wtc wrote: > > Nit: they => it ? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/25312002/64001
Message was sent while issue was closed.
Change committed as 226539 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
