Add more BrotliFilter information to UMA.

TL;DR: adding "gzip-header-detected" and "brotli-error-code" UMA's

BrotliFilter.Status UMA's show that about 0.5% of requests finish with "In Progress" status and 0.05% with "Error" status.

The same data sliced by platform:

            | Windows | Android |
------------+---------+---------+
In progress |   0.57% |   0.29% |
Error       |   0.00% |   0.15% |
------------+---------+---------+
          Σ |   0.57% |   0.44% |

The sum for Windows and Android looks similar.

This makes me suspicious that it both results may have the same reason:
actual content has invalid (non-brotli) encoding.

To check this I plan to add additional UMA's: one will report if gzip header is detected, the other one will report brotli error code that will allow to understand at what point "corrupted" input is detected.

BUG=613113
Committed: https://crrev.com/5a2ee1339095755400f7249f6add4e97d29717c6
Cr-Commit-Position: refs/heads/master@{#395552}

[eustas, 2016-05-18 11:42:46]

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

[eustas, 2016-05-18 11:44:46]

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

[cbentzel, 2016-05-18 16:11:25]

Looks like you are trying to identify cases where gzip is sent when we expect
brotli based on content-encoding header?

If so, can you update the CL description with more information about that.

Also, could you have an associated bug with this CL?

Thanks

[eustas, 2016-05-19 09:36:59]

Description was changed from

==========
Add more BrotliFilter information to UMA.

BUG=
==========

to

==========
TL;DR: adding "gzip-header-detected" and "brotli-error-code" UMA's


BrotliFilter.Status UMA's show that about 0.5% of requests finish with "In
Progress" status and 0.05% with "Error" status.

The same data sliced by platform:

            | Windows | Android |
------------+---------+---------+
In progress |   0.57% |   0.29% |
Error       |   0.00% |   0.15% |
------------+---------+---------+
          Σ |   0.57% |   0.44% |

The sum for Windows and Android looks similar.

This makes me suspicious that it both results may have the same reason:
actual content has invalid (non-brotli) encoding.

To check this I plan to add additional UMA's: one will report if gzip header is
detected, the other one will report brotli error code that will allow to
understand at what point "corrupted" input is detected.

BUG=613113
==========

[eustas, 2016-05-19 09:37:30]

On 2016/05/18 16:11:25, cbentzel wrote:
> Looks like you are trying to identify cases where gzip is sent when we expect
> brotli based on content-encoding header?
> 
> If so, can you update the CL description with more information about that.
> 
> Also, could you have an associated bug with this CL?
> 
> Thanks

Done.

[cbentzel, 2016-05-19 11:55:33]

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc
File net/filter/brotli_filter.cc (right):

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc...
net/filter/brotli_filter.cc:17: const uint8_t gzip_header[] = {0x1f, 0x8b,
0x08};
Nit: Should be kGzipHeader or similar.
https://google.github.io/styleguide/cppguide.html#Constant_Names

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc...
net/filter/brotli_filter.cc:64: static_cast<int>(-error_code), 40);
This seems a bit fragile if additional error codes get added. Is there a way to
add BROTLI_ERROR_MAX to the enum in decode.h and reference it here? It does mean
that this would be gated on an update of the brotli library.

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc...
net/filter/brotli_filter.cc:110: for (size_t i = consumed_bytes_; i <
sizeof(gzip_header); ++i) {
Would this ever lead to a case where we read part of the first three bytes in a
previous call (so consumed_bytes_ != 0)? Then the offsets going into
available_in and next_in would not work.

[eustas, 2016-05-19 13:07:54]

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc
File net/filter/brotli_filter.cc (right):

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc...
net/filter/brotli_filter.cc:17: const uint8_t gzip_header[] = {0x1f, 0x8b,
0x08};
On 2016/05/19 11:55:33, cbentzel wrote:
> Nit: Should be kGzipHeader or similar.
> https://google.github.io/styleguide/cppguide.html#Constant_Names

Done.

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc...
net/filter/brotli_filter.cc:64: static_cast<int>(-error_code), 40);
On 2016/05/19 11:55:33, cbentzel wrote:
> This seems a bit fragile if additional error codes get added. Is there a way
to
> add BROTLI_ERROR_MAX to the enum in decode.h and reference it here? It does
mean
> that this would be gated on an update of the brotli library.

It is possible, but I'm not sure what will happen when we update brotli library
and don't touch histograms.xml ...

Going to add constant to brotli github ASAP.

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc...
net/filter/brotli_filter.cc:110: for (size_t i = consumed_bytes_; i <
sizeof(gzip_header); ++i) {
On 2016/05/19 11:55:32, cbentzel wrote:
> Would this ever lead to a case where we read part of the first three bytes in
a
> previous call (so consumed_bytes_ != 0)? Then the offsets going into
> available_in and next_in would not work.

Brotli consumes as much input as possible -> if there are 3 or more bytes of
input, and compressed stream (output) is not empty, then |consumed_bytes_| will
be >= 3 after the first iteration.

If there are less then 3 bytes of input in first round, let's say 1, then brotli
reads them all and |consumed_bytes_| are increased by 1.
In the second round we get 1 more byte of input. |next_in| points to the
beginning of new input. |consumed_bytes_| == 1 is the offset from the beginning
of input (-> beginning of header). And, at last, in thisd round
|consumed_bytes_| == 2 and last byte of header will be checked.

So, even reading input byte-by-byte won't harm.

[Steven Holte, 2016-05-19 19:10:15]

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc
File net/filter/brotli_filter.cc (right):

https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc...
net/filter/brotli_filter.cc:64: static_cast<int>(-error_code), 40);
On 2016/05/19 13:07:54, eustas wrote:
> On 2016/05/19 11:55:33, cbentzel wrote:
> > This seems a bit fragile if additional error codes get added. Is there a way
> to
> > add BROTLI_ERROR_MAX to the enum in decode.h and reference it here? It does
> mean
> > that this would be gated on an update of the brotli library.
> 
> It is possible, but I'm not sure what will happen when we update brotli
library
> and don't touch histograms.xml ...
> 
> Going to add constant to brotli github ASAP.

It's definitely better to be using a BROTLI_ERROR_MAX constant here.  That will
ensure you are not overflowing, which will result in recording a count in bucket
[40, MAX_INT), instead of e.g. [41,42).  We parse histograms.xml from Chrome's
TOT to aid in decoding, but it won't change the chrome behavior.  Once you
update the xml, we'll just be able to decode the enums.

[eustas, 2016-05-20 08:44:09]

On 2016/05/19 19:10:15, Steven Holte wrote:
> https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc
> File net/filter/brotli_filter.cc (right):
> 
>
https://codereview.chromium.org/1995513002/diff/1/net/filter/brotli_filter.cc...
> net/filter/brotli_filter.cc:64: static_cast<int>(-error_code), 40);
> On 2016/05/19 13:07:54, eustas wrote:
> > On 2016/05/19 11:55:33, cbentzel wrote:
> > > This seems a bit fragile if additional error codes get added. Is there a
way
> > to
> > > add BROTLI_ERROR_MAX to the enum in decode.h and reference it here? It
does
> > mean
> > > that this would be gated on an update of the brotli library.
> > 
> > It is possible, but I'm not sure what will happen when we update brotli
> library
> > and don't touch histograms.xml ...
> > 
> > Going to add constant to brotli github ASAP.
> 
> It's definitely better to be using a BROTLI_ERROR_MAX constant here.  That
will
> ensure you are not overflowing, which will result in recording a count in
bucket
> [40, MAX_INT), instead of e.g. [41,42).  We parse histograms.xml from Chrome's
> TOT to aid in decoding, but it won't change the chrome behavior.  Once you
> update the xml, we'll just be able to decode the enums.

Got it. Fixed.

[eustas, 2016-05-20 10:03:59]

Required brotli update (https://codereview.chromium.org/1995193003/) has landed.

[cbentzel, 2016-05-21 19:42:40]

LGTM

https://codereview.chromium.org/1995513002/diff/60001/net/filter/brotli_filte...
File net/filter/brotli_filter.cc (right):

https://codereview.chromium.org/1995513002/diff/60001/net/filter/brotli_filte...
net/filter/brotli_filter.cc:53: static_cast<bool>(gzip_header_detected_));
Why is this static_cast needed?

[eustas, 2016-05-23 09:46:23]

https://codereview.chromium.org/1995513002/diff/60001/net/filter/brotli_filte...
File net/filter/brotli_filter.cc (right):

https://codereview.chromium.org/1995513002/diff/60001/net/filter/brotli_filte...
net/filter/brotli_filter.cc:53: static_cast<bool>(gzip_header_detected_));
On 2016/05/21 19:42:40, cbentzel wrote:
> Why is this static_cast needed?

It is a copy-paste leftover. Removed.

[eustas, 2016-05-23 09:47:20]

Steven, please take another look. Thanks.

[Steven Holte, 2016-05-23 20:33:37]

lgtm

[eustas, 2016-05-24 07:54:18]

The CQ bit was checked by eustas@chromium.org

[eustas, 2016-05-24 07:54:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbentzel@chromium.org
Link to the patchset: https://codereview.chromium.org/1995513002/#ps80001
(title: "Fix nits")

[commit-bot: I haz the power, 2016-05-24 07:54:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995513002/80001

[commit-bot: I haz the power, 2016-05-24 09:15:34]

Description was changed from

==========
TL;DR: adding "gzip-header-detected" and "brotli-error-code" UMA's


BrotliFilter.Status UMA's show that about 0.5% of requests finish with "In
Progress" status and 0.05% with "Error" status.

The same data sliced by platform:

            | Windows | Android |
------------+---------+---------+
In progress |   0.57% |   0.29% |
Error       |   0.00% |   0.15% |
------------+---------+---------+
          Σ |   0.57% |   0.44% |

The sum for Windows and Android looks similar.

This makes me suspicious that it both results may have the same reason:
actual content has invalid (non-brotli) encoding.

To check this I plan to add additional UMA's: one will report if gzip header is
detected, the other one will report brotli error code that will allow to
understand at what point "corrupted" input is detected.

BUG=613113
==========

to

==========
TL;DR: adding "gzip-header-detected" and "brotli-error-code" UMA's


BrotliFilter.Status UMA's show that about 0.5% of requests finish with "In
Progress" status and 0.05% with "Error" status.

The same data sliced by platform:

            | Windows | Android |
------------+---------+---------+
In progress |   0.57% |   0.29% |
Error       |   0.00% |   0.15% |
------------+---------+---------+
          Σ |   0.57% |   0.44% |

The sum for Windows and Android looks similar.

This makes me suspicious that it both results may have the same reason:
actual content has invalid (non-brotli) encoding.

To check this I plan to add additional UMA's: one will report if gzip header is
detected, the other one will report brotli error code that will allow to
understand at what point "corrupted" input is detected.

BUG=613113
==========

[commit-bot: I haz the power, 2016-05-24 09:15:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-24 09:17:46]

Description was changed from

==========
TL;DR: adding "gzip-header-detected" and "brotli-error-code" UMA's


BrotliFilter.Status UMA's show that about 0.5% of requests finish with "In
Progress" status and 0.05% with "Error" status.

The same data sliced by platform:

            | Windows | Android |
------------+---------+---------+
In progress |   0.57% |   0.29% |
Error       |   0.00% |   0.15% |
------------+---------+---------+
          Σ |   0.57% |   0.44% |

The sum for Windows and Android looks similar.

This makes me suspicious that it both results may have the same reason:
actual content has invalid (non-brotli) encoding.

To check this I plan to add additional UMA's: one will report if gzip header is
detected, the other one will report brotli error code that will allow to
understand at what point "corrupted" input is detected.

BUG=613113
==========

to

==========
TL;DR: adding "gzip-header-detected" and "brotli-error-code" UMA's

BrotliFilter.Status UMA's show that about 0.5% of requests finish with "In
Progress" status and 0.05% with "Error" status.

The same data sliced by platform:

            | Windows | Android |
------------+---------+---------+
In progress |   0.57% |   0.29% |
Error       |   0.00% |   0.15% |
------------+---------+---------+
          Σ |   0.57% |   0.44% |

The sum for Windows and Android looks similar.

This makes me suspicious that it both results may have the same reason:
actual content has invalid (non-brotli) encoding.

To check this I plan to add additional UMA's: one will report if gzip header is
detected, the other one will report brotli error code that will allow to
understand at what point "corrupted" input is detected.

BUG=613113

Committed: https://crrev.com/5a2ee1339095755400f7249f6add4e97d29717c6
Cr-Commit-Position: refs/heads/master@{#395552}
==========

[commit-bot: I haz the power, 2016-05-24 09:17:47]

Patchset 5 (id:??) landed as
https://crrev.com/5a2ee1339095755400f7249f6add4e97d29717c6
Cr-Commit-Position: refs/heads/master@{#395552}