Add more BrotliFilter information to UMA.

net/filter/brotli_filter.cc

Index: net/filter/brotli_filter.cc
diff --git a/net/filter/brotli_filter.cc b/net/filter/brotli_filter.cc
index 7ce402ecb684358ff35db3193c6383ac927f3779..5faabb4b9074625c92c7ae468e5e96f690a13bfa 100644
--- a/net/filter/brotli_filter.cc
+++ b/net/filter/brotli_filter.cc
@@ -13,6 +13,10 @@
 
 namespace net {
 
+namespace {
+const uint8_t gzip_header[] = {0x1f, 0x8b, 0x08};

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

Nit: Should be kGzipHeader or similar.
https://google.github.io/styleguide/cppguide.html#Constant_Names

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

Done.

+}
+
 // BrotliFilter applies Brotli content decoding to a data stream.
 // Brotli format specification: http://www.ietf.org/id/draft-alakuijala-brotli
 //
@@ -26,20 +30,27 @@ class BrotliFilter : public Filter {
         used_memory_(0),
         used_memory_maximum_(0),
         consumed_bytes_(0),
-        produced_bytes_(0) {
+        produced_bytes_(0),
+        gzip_header_detected_(true) {
     brotli_state_ = BrotliCreateState(BrotliFilter::AllocateMemory,
                                       BrotliFilter::FreeMemory, this);
     CHECK(brotli_state_);
   }
 
   ~BrotliFilter() override {
+    BrotliErrorCode error_code = BrotliGetErrorCode(brotli_state_);
     BrotliDestroyState(brotli_state_);
     brotli_state_ = nullptr;
     DCHECK(used_memory_ == 0);
 
+    // Don't report that gzip header was detected in case of lack of input.
+    gzip_header_detected_ &= (consumed_bytes_ >= sizeof(gzip_header));
+
     UMA_HISTOGRAM_ENUMERATION(
         "BrotliFilter.Status", static_cast<int>(decoding_status_),
         static_cast<int>(DecodingStatus::DECODING_STATUS_COUNT));
+    UMA_HISTOGRAM_BOOLEAN("BrotliFilter.GzipHeaderDetected",
+                          static_cast<bool>(gzip_header_detected_));
     if (decoding_status_ == DecodingStatus::DECODING_DONE) {
       // CompressionPercent is undefined when there is no output produced.
       if (produced_bytes_ != 0) {
@@ -48,6 +59,10 @@ class BrotliFilter : public Filter {
             static_cast<int>((consumed_bytes_ * 100) / produced_bytes_));
       }
     }
+    if (error_code < 0) {
+      UMA_HISTOGRAM_ENUMERATION("BrotliFilter.ErrorCode",
+                                static_cast<int>(-error_code), 40);

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

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.

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

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.

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

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.

+    }
 
     // All code here is for gathering stats, and can be removed when
     // BrotliFilter is considered stable.
@@ -90,6 +105,14 @@ class BrotliFilter : public Filter {
     size_t available_out = output_buffer_size;
     uint8_t* next_out = bit_cast<uint8_t*>(dest_buffer);
     size_t total_out = 0;
+
+    // Check if start of the input stream looks like gzip stream.
+    for (size_t i = consumed_bytes_; i < sizeof(gzip_header); ++i) {

[cbentzel, 2016/05/19 11:55:32]

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]

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.

+      size_t j = i - consumed_bytes_;
+      if (j < available_in && gzip_header[i] != next_in[j])
+        gzip_header_detected_ = false;
+    }
+
     BrotliResult result =
         BrotliDecompressStream(&available_in, &next_in, &available_out,
                                &next_out, &total_out, brotli_state_);
@@ -180,6 +203,8 @@ class BrotliFilter : public Filter {
   size_t consumed_bytes_;
   size_t produced_bytes_;
 
+  bool gzip_header_detected_;
+
   DISALLOW_COPY_AND_ASSIGN(BrotliFilter);
 };