|
|
DescriptionDo not abort redirect responses with unadvertised encoding.
It is actually a server misbehavior, but currently it is unknown,
how widespread it is.
Added UMA to track such events.
Drive-by: fix reported encoding in case of unadvertised encoding
to be "REJECTED".
BUG=714514
Review-Url: https://codereview.chromium.org/2835403003
Cr-Commit-Position: refs/heads/master@{#468032}
Committed: https://chromium.googlesource.com/chromium/src/+/14d9d00fcdd545dc47387443e17c3d9fbb41e5fb
Patch Set 1 #Patch Set 2 : Add unittest #
Total comments: 3
Patch Set 3 : Pulled the check out of the loop and report "false" for valid responses #
Messages
Total messages: 27 (11 generated)
eustas@chromium.org changed reviewers: + mmenke@chromium.org, rdsmith@chromium.org
Relying on Matt for validity of usage of IsRedirect() in this case; beyond that LGTM.
On 2017/04/25 21:21:58, Randy Smith (Not in Mondays) wrote: > Relying on Matt for validity of usage of IsRedirect() in this case; beyond that > LGTM. Use looks fine to me, but two things: 1) Should we have a test for this? 2) Is a histogram that just uses the true bucket OK? I'll defer on the histograms.xml owners on this.
eustas@chromium.org changed reviewers: + holte@google.com
Steven, take a look at histograms.xml, please.
On 2017/04/25 21:24:29, mmenke wrote: > On 2017/04/25 21:21:58, Randy Smith (Not in Mondays) wrote: > > Relying on Matt for validity of usage of IsRedirect() in this case; beyond > that > > LGTM. > > Use looks fine to me, but two things: > > 1) Should we have a test for this? > 2) Is a histogram that just uses the true bucket OK? I'll defer on the > histograms.xml owners on this. Sure. Added unittest and histograms reviewer.
holte@chromium.org changed reviewers: + holte@chromium.org
lgtm, but consider adding the false case. https://codereview.chromium.org/2835403003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2835403003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:1814: true); Recording only true values will technically work, and will give you absolute counts of how often this occurs, but it won't give you any kind of baseline. If you want to know "How often is this a redirect" or "When this is a redirect, how often are there no encodings, I'd suggest recording the false case.
https://codereview.chromium.org/2835403003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2835403003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:1814: true); On 2017/04/26 02:20:22, Steven Holte wrote: > Recording only true values will technically work, and will give you absolute > counts of how often this occurs, but it won't give you any kind of baseline. > > If you want to know "How often is this a redirect" or "When this is a redirect, > how often are there no encodings, I'd suggest recording the false case. If you do this I'd recommend the denominator be the number of redirects that we get, not the number of bad encodings. I think the fraction of redirects that are bad encodings has some interest, but the fraction of bad encodings that are redirects does not. (But I'm uncertain as to whether we want that percentage or just an absolutely number, or maybe the denominator being the total number of requests we get. Which makes me not want to worry about it too much--worst case, we can always do the comparisons by hand against another statistic.)
LGTM
PTAL https://codereview.chromium.org/2835403003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2835403003/diff/20001/net/http/http_network_t... net/http/http_network_transaction.cc:1814: true); On 2017/04/26 03:26:50, Randy Smith (Not in Mondays) wrote: > On 2017/04/26 02:20:22, Steven Holte wrote: > > Recording only true values will technically work, and will give you absolute > > counts of how often this occurs, but it won't give you any kind of baseline. > > > > If you want to know "How often is this a redirect" or "When this is a > redirect, > > how often are there no encodings, I'd suggest recording the false case. > > If you do this I'd recommend the denominator be the number of redirects that we > get, not the number of bad encodings. I think the fraction of redirects that > are bad encodings has some interest, but the fraction of bad encodings that are > redirects does not. > > (But I'm uncertain as to whether we want that percentage or just an absolutely > number, or maybe the denominator being the total number of requests we get. > Which makes me not want to worry about it too much--worst case, we can always do > the comparisons by hand against another statistic.) Done.
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...
LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eustas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2835403003/#ps40001 (title: "Pulled the check out of the loop and report "false" for valid responses")
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": 40001, "attempt_start_ts": 1493398654029390, "parent_rev": "509a04187b65fc65c561d81af3ee3ede268584f3", "commit_rev": "14d9d00fcdd545dc47387443e17c3d9fbb41e5fb"}
Message was sent while issue was closed.
Description was changed from ========== Do not abort redirect responses with unadvertised encoding. It is actually a server misbehavior, but currently it is unknown, how widespread it is. Added UMA to track such events. Drive-by: fix reported encoding in case of unadvertised encoding to be "REJECTED". BUG=714514 ========== to ========== Do not abort redirect responses with unadvertised encoding. It is actually a server misbehavior, but currently it is unknown, how widespread it is. Added UMA to track such events. Drive-by: fix reported encoding in case of unadvertised encoding to be "REJECTED". BUG=714514 Review-Url: https://codereview.chromium.org/2835403003 Cr-Commit-Position: refs/heads/master@{#468032} Committed: https://chromium.googlesource.com/chromium/src/+/14d9d00fcdd545dc47387443e17c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/14d9d00fcdd545dc47387443e17c...
Message was sent while issue was closed.
On 2017/04/28 17:04:52, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) as > https://chromium.googlesource.com/chromium/src/+/14d9d00fcdd545dc47387443e17c... This is the current culprit for net_unittests flake, so I'm reverting. I had to do the revert manually, here: https://codereview.chromium.org/2849193002/ . See 716594 for details.
Message was sent while issue was closed.
On 2017/05/01 18:03:26, sky wrote: > On 2017/04/28 17:04:52, commit-bot: I haz the power wrote: > > Committed patchset #3 (id:40001) as > > > https://chromium.googlesource.com/chromium/src/+/14d9d00fcdd545dc47387443e17c... > > This is the current culprit for net_unittests flake, so I'm reverting. I had to > do the revert manually, here: https://codereview.chromium.org/2849193002/ . See > 716594 for details. What makes you so sure this is the culprit? That test doesn't even look to make HttpNetworkTransactions.
Message was sent while issue was closed.
On 2017/05/01 18:06:45, mmenke wrote: > On 2017/05/01 18:03:26, sky wrote: > > On 2017/04/28 17:04:52, commit-bot: I haz the power wrote: > > > Committed patchset #3 (id:40001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/14d9d00fcdd545dc47387443e17c... > > > > This is the current culprit for net_unittests flake, so I'm reverting. I had > to > > do the revert manually, here: https://codereview.chromium.org/2849193002/ . > See > > 716594 for details. > > What makes you so sure this is the culprit? That test doesn't even look to make > HttpNetworkTransactions. This was one of the patches that touched net. As you say though, this wasn't the culprit and I have reverted the revert here: https://codereview.chromium.org/2852123002/. Sorry for the noise. |