Do not abort redirect responses with unadvertised encoding.

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/+/14d9d00fcdd545dc47387443e17c3d9fbb41e5fb

[eustas, 2017-04-25 21:14:38]

eustas@chromium.org changed reviewers:
+ mmenke@chromium.org, rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2017-04-25 21:21:58]

Relying on Matt for validity of usage of IsRedirect() in this case; beyond that
LGTM.

[mmenke, 2017-04-25 21:24:29]

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, 2017-04-25 21:59:22]

eustas@chromium.org changed reviewers:
+ holte@google.com

[eustas, 2017-04-25 21:59:22]

Steven, take a look at histograms.xml, please.

[eustas, 2017-04-25 22:20:41]

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.

[Steven Holte, 2017-04-26 02:20:22]

holte@chromium.org changed reviewers:
+ holte@chromium.org

[Steven Holte, 2017-04-26 02:20:23]

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.

[Randy Smith (Not in Mondays), 2017-04-26 03:26:50]

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.)

[mmenke, 2017-04-26 14:23:02]

LGTM

[eustas, 2017-04-28 13:53:10]

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.

[eustas, 2017-04-28 13:53:51]

The CQ bit was checked by eustas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 13:54:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Randy Smith (Not in Mondays), 2017-04-28 15:06:09]

LGTM.

[commit-bot: I haz the power, 2017-04-28 15:54:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 15:54:04]

Dry run: This issue passed the CQ dry run.

[eustas, 2017-04-28 16:57:34]

The CQ bit was checked by eustas@chromium.org

[eustas, 2017-04-28 16:57:34]

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")

[commit-bot: I haz the power, 2017-04-28 16:58:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 17:04:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493398654029390,
"parent_rev": "509a04187b65fc65c561d81af3ee3ede268584f3", "commit_rev":
"14d9d00fcdd545dc47387443e17c3d9fbb41e5fb"}

[commit-bot: I haz the power, 2017-04-28 17:04:51]

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...
==========

[commit-bot: I haz the power, 2017-04-28 17:04:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/14d9d00fcdd545dc47387443e17c...

[sky, 2017-05-01 18:03:26]

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.

[mmenke, 2017-05-01 18:06:45]

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.

[sky, 2017-05-01 21:10:51]

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.