Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2753453003:
Reject unadvertised encodings (Closed)
DescriptionReject unadvertised encodings in HttpNetworkTransaction.
Only recognized encodings are affected. Currently these are:
* br
* deflate
* gzip / x-gzip
* sdch
FilterSourceStream::ParseEncodingType is used to check if encoding is "recognized".
"Unrecognized" encodings go through HttpNetworkTransaction check and URLRequestHttpJob is responsible for them.
When non-advertised recognised encoding is detected, it is recorded in UMA as "REJECTED", and transaction is finished with CONTENT_DECODING_FAILED error code.
BUG=579606
Review-Url: https://codereview.chromium.org/2753453003
Cr-Commit-Position: refs/heads/master@{#462414}
Committed: https://chromium.googlesource.com/chromium/src/+/c7d27da6abdf2bb847539a83e485e7d0389ccce0
Patch Set 1 #Patch Set 2 : Reject unadvertised encodings. #
Total comments: 16
Patch Set 3 : Reject unadvertised encodings. #
Total comments: 13
Patch Set 4 : Rewrite parser and move it to HttpUtil #Patch Set 5 : Split parsing and matching #Patch Set 6 : Extracted code for UMA reporting #
Total comments: 10
Patch Set 7 : Address comments #Patch Set 8 : Reject unadvertised encodings. #Patch Set 9 : Reject unadvertised encodings. #Patch Set 10 : Reject unadvertised encodings. #
Total comments: 7
Patch Set 11 : Address comments #
Total comments: 1
Patch Set 12 : Fix nit. #
Total comments: 2
Patch Set 13 : Fix histo value comments #
Messages
Total messages: 45 (21 generated)
eustas@chromium.org changed reviewers: + rdsmith@chromium.org
rdsmith@chromium.org changed reviewers: + mmenke@chromium.org
Quick first pass--didn't review the tests since I'm on bug triage today and trying to focus on that. Matt, specific consult: Should we do this check on communications with the proxy? I'm inclined yes, but don't have confidence in my understanding of the proxy arch. https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list... net/base/net_error_list.h:706: NET_ERROR(INVALID_CONTENT_ENCODING, -373) I"m inclined instead to make this CONTENT_DECODING_FAILURE and update the Net.ContentDecodingFailed2.FilterType histogram to include "unknown" (may need to be at a different site than the current one). https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:97: bool IsAcceptableEncoding(const std::string& accept_encoding, Confirming: This is here because there's no existing code in chrome to parse client headers, specifically Accept-Encoding, because we generally create that field? This feels like the wrong place, but maybe there isn't a better one :-{. https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:669: // TODO(eustas): Should content encoding be checked here? Hmmm. I'm inclined to think yes. I'll ask Matt for an opinion, though; I don't know the proxy code well. https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:1373: // TODO(eustas): report in UMA? See comment about which error. https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:1374: // TODO(eustas): ignore, based on Finch flag? I'm inclined to think we can just watch the metrics and the screaming as this rolls out, and roll it back if we've done something horrible. https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:1848: // missing. It is a widely used setup in unit-tests. If this is specifically for unit tests, it's not very performance critical. If so, I'd suggest pulling the reading of the AcceptEncoding header up to here and having an immediate bail in the while loop if it wasn't found; I think that'd be easier to read code.
On 2017/03/15 16:35:32, Randy Smith (Not in Mondays) wrote: > Quick first pass--didn't review the tests since I'm on bug triage today and > trying to focus on that. > > Matt, specific consult: Should we do this check on communications with the > proxy? I'm inclined yes, but don't have confidence in my understanding of the > proxy arch. Yes, we should. If the proxy sends us a response body with a format we didn't advertise, we should fail the request. That's the page the user will get if they just click "cancel" rather than entering credentials. > > https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list.h > File net/base/net_error_list.h (right): > > https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list... > net/base/net_error_list.h:706: NET_ERROR(INVALID_CONTENT_ENCODING, -373) > I"m inclined instead to make this CONTENT_DECODING_FAILURE and update the > Net.ContentDecodingFailed2.FilterType histogram to include "unknown" (may need > to be at a different site than the current one). > > https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... > File net/http/http_network_transaction.cc (right): > > https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... > net/http/http_network_transaction.cc:97: bool IsAcceptableEncoding(const > std::string& accept_encoding, > Confirming: This is here because there's no existing code in chrome to parse > client headers, specifically Accept-Encoding, because we generally create that > field? This feels like the wrong place, but maybe there isn't a better one :-{. > > https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... > net/http/http_network_transaction.cc:669: // TODO(eustas): Should content > encoding be checked here? > Hmmm. I'm inclined to think yes. I'll ask Matt for an opinion, though; I don't > know the proxy code well. > > https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... > net/http/http_network_transaction.cc:1373: // TODO(eustas): report in UMA? > See comment about which error. > > https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... > net/http/http_network_transaction.cc:1374: // TODO(eustas): ignore, based on > Finch flag? > I'm inclined to think we can just watch the metrics and the screaming as this > rolls out, and roll it back if we've done something horrible. > > https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... > net/http/http_network_transaction.cc:1848: // missing. It is a widely used setup > in unit-tests. > If this is specifically for unit tests, it's not very performance critical. If > so, I'd suggest pulling the reading of the AcceptEncoding header up to here and > having an immediate bail in the while loop if it wasn't found; I think that'd be > easier to read code.
On 2017/03/15 16:55:52, mmenke wrote: > On 2017/03/15 16:35:32, Randy Smith (Not in Mondays) wrote: > > Quick first pass--didn't review the tests since I'm on bug triage today and > > trying to focus on that. > > > > Matt, specific consult: Should we do this check on communications with the > > proxy? I'm inclined yes, but don't have confidence in my understanding of the > > proxy arch. > > Yes, we should. If the proxy sends us a response body with a format we didn't > advertise, we should fail the request. That's the page the user will get if > they just click "cancel" rather than entering credentials. Note that for tunnels it doesn't matter so much, since we never display those, but for non-tunnels we should definitely be enforcing it. I'd still say we should enforce it for non-tunnel responses...Hrm...Do we even send content-encodings with those requests? If not, may be best *not* to enforce it, because I could see plenty of tunnels always sending us gzip or somesuch.
https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list... net/base/net_error_list.h:706: NET_ERROR(INVALID_CONTENT_ENCODING, -373) On 2017/03/15 16:35:31, Randy Smith (Not in Mondays) wrote: > I"m inclined instead to make this CONTENT_DECODING_FAILURE and update the > Net.ContentDecodingFailed2.FilterType histogram to include "unknown" (may need > to be at a different site than the current one). Done. Yup, net/http -> net/filter dependency doesn't look natural... https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:97: bool IsAcceptableEncoding(const std::string& accept_encoding, On 2017/03/15 16:35:31, Randy Smith (Not in Mondays) wrote: > Confirming: This is here because there's no existing code in chrome to parse > client headers, specifically Accept-Encoding, because we generally create that > field? This feels like the wrong place, but maybe there isn't a better one :-{. Yup, hasn't found parsers for "Accept-xxx" header. Perhaps it could be placed somewhere in net/base, but it might look strange, if there will be only one user of it. https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:669: // TODO(eustas): Should content encoding be checked here? On 2017/03/15 16:35:32, Randy Smith (Not in Mondays) wrote: > Hmmm. I'm inclined to think yes. I'll ask Matt for an opinion, though; I don't > know the proxy code well. Added similar check here. https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:1373: // TODO(eustas): report in UMA? On 2017/03/15 16:35:32, Randy Smith (Not in Mondays) wrote: > See comment about which error. Done. https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:1374: // TODO(eustas): ignore, based on Finch flag? On 2017/03/15 16:35:32, Randy Smith (Not in Mondays) wrote: > I'm inclined to think we can just watch the metrics and the screaming as this > rolls out, and roll it back if we've done something horrible. Done. https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:1848: // missing. It is a widely used setup in unit-tests. On 2017/03/15 16:35:32, Randy Smith (Not in Mondays) wrote: > If this is specifically for unit tests, it's not very performance critical. If > so, I'd suggest pulling the reading of the AcceptEncoding header up to here and > having an immediate bail in the while loop if it wasn't found; I think that'd be > easier to read code. Pulled GetHeader out of loop.
On 2017/03/15 17:14:44, mmenke wrote: > On 2017/03/15 16:55:52, mmenke wrote: > > On 2017/03/15 16:35:32, Randy Smith (Not in Mondays) wrote: > > > Quick first pass--didn't review the tests since I'm on bug triage today and > > > trying to focus on that. > > > > > > Matt, specific consult: Should we do this check on communications with the > > > proxy? I'm inclined yes, but don't have confidence in my understanding of > the > > > proxy arch. > > > > Yes, we should. If the proxy sends us a response body with a format we didn't > > advertise, we should fail the request. That's the page the user will get if > > they just click "cancel" rather than entering credentials. > > Note that for tunnels it doesn't matter so much, since we never display those, > but for non-tunnels we should definitely be enforcing it. I'd still say we > should enforce it for non-tunnel responses...Hrm...Do we even send > content-encodings with those requests? If not, may be best *not* to enforce it, > because I could see plenty of tunnels always sending us gzip or somesuch. Arggh, I'd like to avoid another special case. Eugene, could you figure out if we send Accept-Encoding (which is what you meant, Matt, right?) for tunnel requests? Matt, if we do, I presume it's ok to have this check there too? If we don't, would you be ok with Eugene adding a non-binding check there as well, with UMA so we can figure out if tunnels regularly do that to us?
On 2017/03/16 16:50:48, Randy Smith (Not in Mondays) wrote: > On 2017/03/15 17:14:44, mmenke wrote: > > On 2017/03/15 16:55:52, mmenke wrote: > > > On 2017/03/15 16:35:32, Randy Smith (Not in Mondays) wrote: > > > > Quick first pass--didn't review the tests since I'm on bug triage today > and > > > > trying to focus on that. > > > > > > > > Matt, specific consult: Should we do this check on communications with the > > > > proxy? I'm inclined yes, but don't have confidence in my understanding of > > the > > > > proxy arch. > > > > > > Yes, we should. If the proxy sends us a response body with a format we > didn't > > > advertise, we should fail the request. That's the page the user will get if > > > they just click "cancel" rather than entering credentials. > > > > Note that for tunnels it doesn't matter so much, since we never display those, > > but for non-tunnels we should definitely be enforcing it. I'd still say we > > should enforce it for non-tunnel responses...Hrm...Do we even send > > content-encodings with those requests? If not, may be best *not* to enforce > it, > > because I could see plenty of tunnels always sending us gzip or somesuch. > > Arggh, I'd like to avoid another special case. Eugene, could you figure out if > we send Accept-Encoding (which is what you meant, Matt, right?) for tunnel > requests? Matt, if we do, I presume it's ok to have this check there too? If > we don't, would you be ok with Eugene adding a non-binding check there as well, > with UMA so we can figure out if tunnels regularly do that to us? We should have a test for this, whichever we do. Also, I'm not sure a check in HttpNetworkTransaction would even see response headers for tunnels. They're handled in HttpProxySocketPool (Or something like that), in their own ConnectJob class, and not sure if we send the whole headers up, or just a message indicating we need auth and relevant information about the request.
https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list... net/base/net_error_list.h:706: NET_ERROR(INVALID_CONTENT_ENCODING, -373) On 2017/03/16 12:08:16, eustas wrote: > On 2017/03/15 16:35:31, Randy Smith (Not in Mondays) wrote: > > I"m inclined instead to make this CONTENT_DECODING_FAILURE and update the > > Net.ContentDecodingFailed2.FilterType histogram to include "unknown" (may need > > to be at a different site than the current one). > > Done. Yup, net/http -> net/filter dependency doesn't look natural... I've gone back and forth on whether to ask you to do this or not, but: Ryan S & I have basically agreed that a net/http -> net/filter dependency is a fine one, and I'd rather have the histogram recordings just come from a single place, so: Could you create a static function for recording decoding failures on FilterSourceStream and call it from that function and from here to unify the locations that this histogram is recorded from? I do think the dependency is ok (good, even), and I'd rather have it clear in code terms that there are multiple places that are adding to this histogram. Sorry about the extra work :-J. https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:97: bool IsAcceptableEncoding(const std::string& accept_encoding, On 2017/03/16 12:08:16, eustas wrote: > On 2017/03/15 16:35:31, Randy Smith (Not in Mondays) wrote: > > Confirming: This is here because there's no existing code in chrome to parse > > client headers, specifically Accept-Encoding, because we generally create that > > field? This feels like the wrong place, but maybe there isn't a better one > :-{. > > Yup, hasn't found parsers for "Accept-xxx" header. > Perhaps it could be placed somewhere in net/base, but it might look strange, if > there will be only one user of it. Stick in a TODO to that effect? (To move it somewhere more global if other code needs it?) I'm tempted to suggest moving it into net/filter along the lines of the earlier comment, but that's not needed for this CL. Maybe I'll do it for mine. https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:105: return false; Put a note in here about explicitly not worrying about cases like: Accept-Encoding: gzip;q=0, * Content-Encoding: brotli because we believe the higher layers of chrome will never do that to us? (Note that Accept-Encoding: identity;q=1, *;q=0 is a case we currently explicitly do have to support, but I believe this code explicitly works for that case. Might want to put a note in that we need to support it, though, since it's not the type of A-E that this code is targeted at.) https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:110: if ((c != ' ') && (c != ',')) Do we need to check other whitespace? (Tab specifically) https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:163: return false; Is the thought that q=1 will never happen? I'd sorta like to support it anyway. (It shows up in the above strange corner case, but we want a false return in that case, so that's ok. But I'd at least like to have comment documentation about the ways in which this isn't a general test function, in case it's ever made more general.) ETA: Huh, you test q=1. I'm missing how that's parsed--it looks to me like we'll get to ZERO_OR_ONE, set ones to 1, go to PERIOD, not find a '.' and return false. What am I missing? https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:193: // assert(state == PERIOD || state == DECIMALS || state == DONE Why not make this a DCHECK and remove the comments? https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:1856: // missing. It is a widely used setup in unit-tests. Suggestion: Move this comment down to above the "return false" and invert it? I.e. "It is only ok for Accept-Encoding to be empty if Content-Encoding is also empty. It is a widely used setup in unit tests." https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:1873: encoding = "compress"; Do we need to worry about these cases in the Accept-Encoding header as well? https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:16609: } Other cases to test: 1.1 +* -> false identity;q=1, *;q=0 + empty -> true identity;q=1, *;q=0 + identity -> true identity;q=1, *;q=0 + anything else -> false
Eugene: As per the discussion on http://crbug.com/579606, could you add a function in net/filter that returns all encodings known to be decodeable by that module, and only reject encodings in that list that don't match Accept-Encoding? (Matt: Trying to decouple things as much as possible here; even if other browsers don't like random encodings, we still need to deal with Android WebView, and I want minimal things blocking this CL since there's ecosystem rot that's occurring because of it's lack currently.)
On 2017/03/17 18:40:33, Randy Smith (Not in Mondays) wrote: > Eugene: As per the discussion on http://crbug.com/579606, could you add a > function in net/filter that returns all encodings known to be decodeable by that > module, and only reject encodings in that list that don't match Accept-Encoding? Sure. I've already rewritten parsers (so they look almost as you say), and moved parsers to http_util.cc Going to upload fresh version either tomorrow, or on Monday. Just one question: I hasn't understood one of four of new tests: "1.1 +* -> false"; could you explain it?
On 2017/03/17 19:03:54, eustas wrote: > On 2017/03/17 18:40:33, Randy Smith (Not in Mondays) wrote: > > Eugene: As per the discussion on http://crbug.com/579606, could you add a > > function in net/filter that returns all encodings known to be decodeable by > that > > module, and only reject encodings in that list that don't match > Accept-Encoding? > > Sure. I've already rewritten parsers (so they look almost as you say), and moved > parsers to http_util.cc > Going to upload fresh version either tomorrow, or on Monday. > > Just one question: I hasn't understood one of four of new tests: "1.1 +* -> > false"; could you explain it? My apologies; that was intended to be <encoding>;q=1.1 + * -> false, i.e. testing for the invalid quality specification of something > 1.
Description was changed from ========== Reject unadvertised encodings. BUG=579606 ========== to ========== Reject unadvertised encodings. BUG=579606 ==========
PTAL https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2753453003/diff/20001/net/base/net_error_list... net/base/net_error_list.h:706: NET_ERROR(INVALID_CONTENT_ENCODING, -373) On 2017/03/16 17:50:43, Randy Smith (Not in Mondays) wrote: > On 2017/03/16 12:08:16, eustas wrote: > > On 2017/03/15 16:35:31, Randy Smith (Not in Mondays) wrote: > > > I"m inclined instead to make this CONTENT_DECODING_FAILURE and update the > > > Net.ContentDecodingFailed2.FilterType histogram to include "unknown" (may > need > > > to be at a different site than the current one). > > > > Done. Yup, net/http -> net/filter dependency doesn't look natural... > > I've gone back and forth on whether to ask you to do this or not, but: Ryan S & > I have basically agreed that a net/http -> net/filter dependency is a fine one, > and I'd rather have the histogram recordings just come from a single place, so: > Could you create a static function for recording decoding failures on > FilterSourceStream and call it from that function and from here to unify the > locations that this histogram is recorded from? I do think the dependency is ok > (good, even), and I'd rather have it clear in code terms that there are multiple > places that are adding to this histogram. > > Sorry about the extra work :-J. > No worries =) https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:97: bool IsAcceptableEncoding(const std::string& accept_encoding, On 2017/03/16 17:50:44, Randy Smith (Not in Mondays) wrote: > On 2017/03/16 12:08:16, eustas wrote: > > On 2017/03/15 16:35:31, Randy Smith (Not in Mondays) wrote: > > > Confirming: This is here because there's no existing code in chrome to parse > > > client headers, specifically Accept-Encoding, because we generally create > that > > > field? This feels like the wrong place, but maybe there isn't a better one > > :-{. > > > > Yup, hasn't found parsers for "Accept-xxx" header. > > Perhaps it could be placed somewhere in net/base, but it might look strange, > if > > there will be only one user of it. > > Stick in a TODO to that effect? (To move it somewhere more global if other code > needs it?) > > I'm tempted to suggest moving it into net/filter along the lines of the earlier > comment, but that's not needed for this CL. Maybe I'll do it for mine. Reworked and moved to HttpUtil https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:105: return false; On 2017/03/16 17:50:44, Randy Smith (Not in Mondays) wrote: > Put a note in here about explicitly not worrying about cases like: > > Accept-Encoding: gzip;q=0, * > Content-Encoding: brotli > > because we believe the higher layers of chrome will never do that to us? > > (Note that > > Accept-Encoding: identity;q=1, *;q=0 > > is a case we currently explicitly do have to support, but I believe this code > explicitly works for that case. Might want to put a note in that we need to > support it, though, since it's not the type of A-E that this code is targeted > at.) This code is gone. https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:110: if ((c != ' ') && (c != ',')) On 2017/03/16 17:50:44, Randy Smith (Not in Mondays) wrote: > Do we need to check other whitespace? (Tab specifically) In new code whitespace is trimmed around all tokens. https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:163: return false; On 2017/03/16 17:50:44, Randy Smith (Not in Mondays) wrote: > Is the thought that q=1 will never happen? I'd sorta like to support it anyway. > (It shows up in the above strange corner case, but we want a false return in > that case, so that's ok. But I'd at least like to have comment documentation > about the ways in which this isn't a general test function, in case it's ever > made more general.) > > ETA: Huh, you test q=1. I'm missing how that's parsed--it looks to me like > we'll get to ZERO_OR_ONE, set ones to 1, go to PERIOD, not find a '.' and return > false. What am I missing? Ooops. Fixed it,... and then rewritten it to a more explicit parser. https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:193: // assert(state == PERIOD || state == DECIMALS || state == DONE On 2017/03/16 17:50:44, Randy Smith (Not in Mondays) wrote: > Why not make this a DCHECK and remove the comments? Acknowledged. https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:1856: // missing. It is a widely used setup in unit-tests. On 2017/03/16 17:50:44, Randy Smith (Not in Mondays) wrote: > Suggestion: Move this comment down to above the "return false" and invert it? > I.e. "It is only ok for Accept-Encoding to be empty if Content-Encoding is also > empty. It is a widely used setup in unit tests." Replaced with set-matching code, so now it is more explicit. https://codereview.chromium.org/2753453003/diff/40001/net/http/http_network_t... net/http/http_network_transaction.cc:1873: encoding = "compress"; On 2017/03/16 17:50:44, Randy Smith (Not in Mondays) wrote: > Do we need to worry about these cases in the Accept-Encoding header as well? Made it symmetric in new parser.
So there's one key thing missing here, which is that because of the whole compatibility risk thing (see https://bugs.chromium.org/p/chromium/issues/detail?id=579606#c26), we decided that we really should only reject *known* encodings that were used in CE but weren't in the AE list. So we need something in net/filter (or http/http_util, if you want) that lists the known types, that we can then filter on. Could you add that? (See the recent net-dev@ thread for why I suspect we're not going to be able to phase out passing through unknown content encodings real soon.) Along with that change, I'm suggesting in the comments below changing UNKNOWN to REJECTED (since we'll only be rejecting known types). Related to the above, I'm also asking you to keep UNKNOWN, just use it in a different place. This is for my benefit, not yours, and you're welcome to reject the request as not reasonably part of this CL; I'm just being opportunistic. Details below. Helen, I'd like you to be aware of the ways in which I'm playing fast-and-loose with the ContentDecodingFailed2 histogram. I've tried to be transparent about those ways in the comments below, but the very quick summary is: a) entries from above and below the cache in the same histogram, b) not revving the histogram when modifying its contents (see below for rationale), and c) adding an entry in the histogram that does not correspond to a CONTENT_DECODING_FAILED error. You're the primary other person that may have to deal with the results of my lack of rigor; please give a yell if you object. https://codereview.chromium.org/2753453003/diff/100001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/100001/net/http/http_network_... net/http/http_network_transaction.cc:1735: if (!HttpUtil::ParseAcceptEncoding(accept_encoding, &allowed_encodings)) Does this code handle the case where neither header is included in the request/response pair? (Which I think your last PS handled?) If it does, it would seem like that's reliant on ParseAcceptEncoding() succeeding with a null string as input, which seems worth documenting in the header file? (I'm presuming there's no spec difference between the lack of an Accept-Encoding header and "Accept-Encoding: "?) https://codereview.chromium.org/2753453003/diff/100001/net/http/http_network_... net/http/http_network_transaction.cc:1744: for (auto const& encoding : used_encodings) { This doesn't appear to handle "Accept-Encoding: *". I'm ok with that, but I'd like a comment to that effect. https://codereview.chromium.org/2753453003/diff/100001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2753453003/diff/100001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1080: // Unknown encoding type. Pass through raw response body. Would you be willing to put an entry in here for the unknown content encoding (in a separate comment I'm advocating for using YA new entry in the enum, REJECTED, in HttpNetworkTransaction, so this would be the only use of the unknown type)? This isn't needed for this CL, but it's simple to add, and would aid in the work I'm trying to do to figure out if we can deprecate passing through unknown encodings. https://codereview.chromium.org/2753453003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2753453003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:102611: + <int value="8" label="Unknown"/> Given that we need to only reject known encodings that aren't in Accept-Encoding (as per https://bugs.chromium.org/p/chromium/issues/detail?id=579606#c26 and https://codereview.chromium.org/2753453003#msg11) let's add yet another entry here, which is "rejected", meaning "Yeah, we've heard of it, but we didn't tell the server it could use that encoding, so we aren't going to handle it". Also, I want to call out that there's are two ways in which I'm being less than pure about this histogram (because I just don't think doing it precisely right is worth the hassle). The first is that because we're recording in this histogram from both below and above the cache, comparisons between the "Rejected" bucket and other buckets will be apples and close-to-but-not-quite apples; specifically, entries that make it into the cache will have many chances to be recorded in the histogram, as opposed to just one for the entries that are rejected below the cache. The second is that in some sense modifying the histogram should mean revving it (i.e. moving to "ContentDecodingFailed3"), but we're not modifying the meaning of any of the existing buckets, just adding new ones, so if the people evaluating the histogram remember that it shouldn't be a problem. And this histogram isn't examined that frequently anyway, so within a couple of releases it also shouldn't be a problem.
https://codereview.chromium.org/2753453003/diff/100001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/100001/net/http/http_network_... net/http/http_network_transaction.cc:1735: if (!HttpUtil::ParseAcceptEncoding(accept_encoding, &allowed_encodings)) On 2017/03/21 21:23:20, Randy Smith (Not in Mondays) wrote: > Does this code handle the case where neither header is included in the > request/response pair? (Which I think your last PS handled?) If it does, it > would seem like that's reliant on ParseAcceptEncoding() succeeding with a null > string as input, which seems worth documenting in the header file? > > (I'm presuming there's no spec difference between the lack of an Accept-Encoding > header and "Accept-Encoding: "?) Added augmentation in parser: "" -> {"*"}. Now asterisk case is checked explicitly. https://codereview.chromium.org/2753453003/diff/100001/net/http/http_network_... net/http/http_network_transaction.cc:1744: for (auto const& encoding : used_encodings) { On 2017/03/21 21:23:20, Randy Smith (Not in Mondays) wrote: > This doesn't appear to handle "Accept-Encoding: *". I'm ok with that, but I'd > like a comment to that effect. Fixed. https://codereview.chromium.org/2753453003/diff/100001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2753453003/diff/100001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1080: // Unknown encoding type. Pass through raw response body. On 2017/03/21 21:23:20, Randy Smith (Not in Mondays) wrote: > Would you be willing to put an entry in here for the unknown content encoding > (in a separate comment I'm advocating for using YA new entry in the enum, > REJECTED, in HttpNetworkTransaction, so this would be the only use of the > unknown type)? This isn't needed for this CL, but it's simple to add, and would > aid in the work I'm trying to do to figure out if we can deprecate passing > through unknown encodings. Yup, but I didn't get what item should be added... Report UNKNOWN to UMA and return nullptr? https://codereview.chromium.org/2753453003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2753453003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:102611: + <int value="8" label="Unknown"/> On 2017/03/21 21:23:20, Randy Smith (Not in Mondays) wrote: > Given that we need to only reject known encodings that aren't in Accept-Encoding > (as per https://bugs.chromium.org/p/chromium/issues/detail?id=579606#c26 and > https://codereview.chromium.org/2753453003#msg11) let's add yet another entry > here, which is "rejected", meaning "Yeah, we've heard of it, but we didn't tell > the server it could use that encoding, so we aren't going to handle it". > > Also, I want to call out that there's are two ways in which I'm being less than > pure about this histogram (because I just don't think doing it precisely right > is worth the hassle). The first is that because we're recording in this > histogram from both below and above the cache, comparisons between the > "Rejected" bucket and other buckets will be apples and close-to-but-not-quite > apples; specifically, entries that make it into the cache will have many chances > to be recorded in the histogram, as opposed to just one for the entries that are > rejected below the cache. > > The second is that in some sense modifying the histogram should mean revving it > (i.e. moving to "ContentDecodingFailed3"), but we're not modifying the meaning > of any of the existing buckets, just adding new ones, so if the people > evaluating the histogram remember that it shouldn't be a problem. And this > histogram isn't examined that frequently anyway, so within a couple of releases > it also shouldn't be a problem. > I'm fine with either solution. IMHO having less histograms is better - easier to see the picture as a whole.
Moved known endoding recognition to filters. PTAL
The CQ bit was checked by eustas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eustas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Could you expand the CL description a bit to say where the check is being added, note the lack of change around truly unknown encodings, and describe the UMA changes? I'm happy with most of this (parsing and only rejecting known encodings) but I want to get the UMA right and I want to make sure that this CL doesn't change the behavior of completely unrecognized content-encodings (I think the CL doesn't, but there's a test that seems to have been changed?) Thanks much .... https://codereview.chromium.org/2753453003/diff/100001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2753453003/diff/100001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1080: // Unknown encoding type. Pass through raw response body. On 2017/03/27 10:27:11, eustas wrote: > On 2017/03/21 21:23:20, Randy Smith (Not in Mondays) wrote: > > Would you be willing to put an entry in here for the unknown content encoding > > (in a separate comment I'm advocating for using YA new entry in the enum, > > REJECTED, in HttpNetworkTransaction, so this would be the only use of the > > unknown type)? This isn't needed for this CL, but it's simple to add, and > would > > aid in the work I'm trying to do to figure out if we can deprecate passing > > through unknown encodings. > > Yup, but I didn't get what item should be added... > Report UNKNOWN to UMA and return nullptr? Just report UNKNOWN--we can't change the behavior. https://codereview.chromium.org/2753453003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2753453003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:102611: + <int value="8" label="Unknown"/> On 2017/03/27 10:27:11, eustas wrote: > On 2017/03/21 21:23:20, Randy Smith (Not in Mondays) wrote: > > Given that we need to only reject known encodings that aren't in > Accept-Encoding > > (as per https://bugs.chromium.org/p/chromium/issues/detail?id=579606#c26 and > > https://codereview.chromium.org/2753453003#msg11) let's add yet another entry > > here, which is "rejected", meaning "Yeah, we've heard of it, but we didn't > tell > > the server it could use that encoding, so we aren't going to handle it". > > > > Also, I want to call out that there's are two ways in which I'm being less > than > > pure about this histogram (because I just don't think doing it precisely right > > is worth the hassle). The first is that because we're recording in this > > histogram from both below and above the cache, comparisons between the > > "Rejected" bucket and other buckets will be apples and close-to-but-not-quite > > apples; specifically, entries that make it into the cache will have many > chances > > to be recorded in the histogram, as opposed to just one for the entries that > are > > rejected below the cache. > > > > The second is that in some sense modifying the histogram should mean revving > it > > (i.e. moving to "ContentDecodingFailed3"), but we're not modifying the meaning > > of any of the existing buckets, just adding new ones, so if the people > > evaluating the histogram remember that it shouldn't be a problem. And this > > histogram isn't examined that frequently anyway, so within a couple of > releases > > it also shouldn't be a problem. > > > > I'm fine with either solution. IMHO having less histograms is better - easier to > see the picture as a whole. Sorry, I wasn't proposing two solutions, I was asking you to change "Unknown" to "Rejected" and add a separate "Unknown" which would be reported above the cache. The rest of the comments were documentation for other reviewers to challenge me on being imprecise, and documentation for future code archaeologists. https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... net/http/http_network_transaction.cc:1749: // successfully decoded. This test doesn't seem to be testing for the lack of an accept-encoding; am I missing how it is? If not, should the comment be moved to where that test is actually happening? https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... net/http/http_network_transaction.cc:1751: return true; Move this up above the content encoding parsing? https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:16576: CheckContentEncodingMatching(&session_deps_, "identity;q=1, *;q=0", "", true); Another that that confirms that we bounce gzip if the accept encoding looks like this? https://codereview.chromium.org/2753453003/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2753453003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:32187: +<histogram name="Net.ContentDecodingRejected.FilterType" enum="NetFilterType2"> Just to get it all in one place, my recommendation/request around histograms is: * No new histogram; keep using ContentDecodingFailed. * Two new enum entries: Unknown (optional; feel free to leave it to me) + Rejected. * Unknown is returned from URLRequestHttpJob::SetUpSourceStream, would not modify the current behavior of passing through the data, and would have a documentation note in the histograms.xml file that it did *not* correspond to an error of CONTENT_DECODING_FAILED. * Rejected would be returned from HttpNetworkTransaction for any rejected content encoding. * All entries for the histogram would be done through a static function on FilterSourceStream (as current; I'm just advocating for switching to the old histogram).
Description was changed from ========== Reject unadvertised encodings. BUG=579606 ========== to ========== Reject unadvertised encodings in HttpNetworkTransaction. Only recognized encodings are affected. Currently these are: * br * deflate * gzip / x-gzip * sdch FilterSourceStream::ParseEncodingType is used to check if encoding is "recognized". "Unrecognized" encodings go through HttpNetworkTransaction check and URLRequestHttpJob is responsible for them. When non-advertised recognised encoding is detected, it is recorded in UMA as "REJECTED", and transaction is finished with CONTENT_DECODING_FAILED error code. BUG=579606 ==========
PTAL https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... net/http/http_network_transaction.cc:1749: // successfully decoded. On 2017/03/28 19:55:36, Randy Smith (Not in Mondays) wrote: > This test doesn't seem to be testing for the lack of an accept-encoding; am I > missing how it is? If not, should the comment be moved to where that test is > actually happening? Updated comment. https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... net/http/http_network_transaction.cc:1751: return true; On 2017/03/28 19:55:36, Randy Smith (Not in Mondays) wrote: > Move this up above the content encoding parsing? If we do so, then malformed header values go through. https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2753453003/diff/180001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:16576: CheckContentEncodingMatching(&session_deps_, "identity;q=1, *;q=0", "", true); On 2017/03/28 19:55:36, Randy Smith (Not in Mondays) wrote: > Another that that confirms that we bounce gzip if the accept encoding looks like > this? Done.
LGTM with nit. Thanks very much!! https://codereview.chromium.org/2753453003/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2753453003/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:103722: + "Content-Encoding". nit: Add "... and is thus not comparable with the other values, which are potentially returned from cached entries".
The CQ bit was checked by eustas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eustas@chromium.org changed reviewers: + holte@chromium.org
Steven, take a look at histograms.xml, please.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with some minor corrections. https://codereview.chromium.org/2753453003/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2753453003/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:103806: + appears in "Content-Encoding" header. Despite of being reported in unknown -> an unknown in "Content-Encoding" header -> in a "Content-Encoding" header Despite of being -> Despite being event does not cause CONTENT_DECODING_FAILED error -> this does not cause a CONTENT_DECODING_FAILED error.
https://codereview.chromium.org/2753453003/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2753453003/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:103806: + appears in "Content-Encoding" header. Despite of being reported in On 2017/04/05 17:45:42, Steven Holte wrote: > unknown -> an unknown > in "Content-Encoding" header -> in a "Content-Encoding" > header > Despite of being -> Despite being > event does not cause CONTENT_DECODING_FAILED error -> this does not cause a > CONTENT_DECODING_FAILED error. Done. Thanks.
The CQ bit was checked by eustas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2753453003/#ps240001 (title: "Fix histo value comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491465980299050,
"parent_rev": "7773939ef302371795ef39138e10225f3d493aa1", "commit_rev":
"c7d27da6abdf2bb847539a83e485e7d0389ccce0"}
Message was sent while issue was closed.
Description was changed from ========== Reject unadvertised encodings in HttpNetworkTransaction. Only recognized encodings are affected. Currently these are: * br * deflate * gzip / x-gzip * sdch FilterSourceStream::ParseEncodingType is used to check if encoding is "recognized". "Unrecognized" encodings go through HttpNetworkTransaction check and URLRequestHttpJob is responsible for them. When non-advertised recognised encoding is detected, it is recorded in UMA as "REJECTED", and transaction is finished with CONTENT_DECODING_FAILED error code. BUG=579606 ========== to ========== Reject unadvertised encodings in HttpNetworkTransaction. Only recognized encodings are affected. Currently these are: * br * deflate * gzip / x-gzip * sdch FilterSourceStream::ParseEncodingType is used to check if encoding is "recognized". "Unrecognized" encodings go through HttpNetworkTransaction check and URLRequestHttpJob is responsible for them. When non-advertised recognised encoding is detected, it is recorded in UMA as "REJECTED", and transaction is finished with CONTENT_DECODING_FAILED error code. BUG=579606 Review-Url: https://codereview.chromium.org/2753453003 Cr-Commit-Position: refs/heads/master@{#462414} Committed: https://chromium.googlesource.com/chromium/src/+/c7d27da6abdf2bb847539a83e485... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/c7d27da6abdf2bb847539a83e485... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
