Added new histogram for analyzing SDCH corruption detection cases.

net/filter/sdch_filter.cc

Index: net/filter/sdch_filter.cc
diff --git a/net/filter/sdch_filter.cc b/net/filter/sdch_filter.cc
index c5384a4ef040abe18f50933b01f490b0ca5e4e89..051514178079aac16b7521e95f1ef87d9fed385a 100644
--- a/net/filter/sdch_filter.cc
+++ b/net/filter/sdch_filter.cc
@@ -18,6 +18,40 @@
 
 namespace net {
 
+namespace {
+
+// Disambiguate various types of responses that trigger a meta-refresh,
+// failure, or fallback to pass-through.
+enum ResponseCorruptionDetectionCause {
+  RESPONSE_NONE,
+
+  // 404 Http Response Code
+  RESPONSE_404 = 1,
+
+  // Not a 200 Http Response Code
+  RESPONSE_NOT_200 = 2,
+
+  // Cached before dictionary retrieved.
+  RESPONSE_OLD_UNENCODED = 3,
+
+  // Speculative but incorrect SDCH filtering was added added.
+  RESPONSE_TENTATIVE_SDCH = 4,
+
+  // Missing correct dict for decoding.
+  RESPONSE_NO_DICTIONARY = 5,
+
+  // Not an SDCH response but should be.
+  RESPONSE_CORRUPT_SDCH = 6,
+
+  // No dictionary was advertised with the request, the server claims
+  // to have encoded with SDCH anyway, but it isn't an SDCH response.
+  RESPONSE_ENCODING_LIE = 7,
+
+  RESPONSE_MAX,
+};
+
+}  // namespace
+
 SdchFilter::SdchFilter(const FilterContext& filter_context)
     : filter_context_(filter_context),
       decoding_status_(DECODING_UNINITIALIZED),
@@ -171,20 +205,24 @@ Filter::FilterStatus SdchFilter::ReadFilteredData(char* dest_buffer,
       // Watch out for an error page inserted by the proxy as part of a 40x
       // error response.  When we see such content molestation, we certainly
       // need to fall into the meta-refresh case.
+      ResponseCorruptionDetectionCause cause = RESPONSE_NONE;
       if (filter_context_.GetResponseCode() == 404) {
         // We could be more generous, but for now, only a "NOT FOUND" code will
         // cause a pass through.  All other bad codes will fall into a
         // meta-refresh.
         SdchManager::SdchErrorRecovery(SdchManager::PASS_THROUGH_404_CODE);
+        cause = RESPONSE_404;
         decoding_status_ = PASS_THROUGH;
       } else if (filter_context_.GetResponseCode() != 200) {
         // We need to meta-refresh, with SDCH disabled.
+        cause = RESPONSE_NOT_200;
       } else if (filter_context_.IsCachedContent()
                  && !dictionary_hash_is_plausible_) {
         // We must have hit the back button, and gotten content that was fetched
         // before we *really* advertised SDCH and a dictionary.
         SdchManager::SdchErrorRecovery(SdchManager::PASS_THROUGH_OLD_CACHED);
         decoding_status_ = PASS_THROUGH;
+        cause = RESPONSE_OLD_UNENCODED;
       } else if (possible_pass_through_) {
         // This is the potentially most graceful response. There really was no
         // error. We were just overly cautious when we added a TENTATIVE_SDCH.
@@ -193,21 +231,24 @@ Filter::FilterStatus SdchFilter::ReadFilteredData(char* dest_buffer,
         // not to use sdch, even though there is a dictionary.  To be
         // conservative, we locally added the tentative sdch (fearing that a
         // proxy stripped it!) and we must now recant (pass through).
-        SdchManager::SdchErrorRecovery(SdchManager::DISCARD_TENTATIVE_SDCH);
+        //
         // However.... just to be sure we don't get burned by proxies that
         // re-compress with gzip or other system, we can sniff to see if this
         // is compressed data etc.  For now, we do nothing, which gets us into
         // the meta-refresh result.
         // TODO(jar): Improve robustness by sniffing for valid text that we can
         // actual use re: decoding_status_ = PASS_THROUGH;
+        cause = RESPONSE_TENTATIVE_SDCH;
       } else if (dictionary_hash_is_plausible_) {
         // We need a meta-refresh since we don't have the dictionary.
         // The common cause is a restart of the browser, where we try to render
         // cached content that was saved when we had a dictionary.
-      } else if (filter_context_.IsSdchResponse()) {
+        cause = RESPONSE_NO_DICTIONARY;
+      } else if (filter_context_.SdchResponseExpected()) {
         // This is a very corrupt SDCH request response.  We can't decode it.
         // We'll use a meta-refresh, and get content without asking for SDCH.
         // This will also progressively disable SDCH for this domain.
+        cause = RESPONSE_CORRUPT_SDCH;
       } else {
         // One of the first 9 bytes precluded consideration as a hash.
         // This can't be an SDCH payload, even though the server said it was.
@@ -220,7 +261,14 @@ Filter::FilterStatus SdchFilter::ReadFilteredData(char* dest_buffer,
         // ... but further back-off on advertising SDCH support.
         url_request_context_->sdch_manager()->BlacklistDomain(
             url_, SdchManager::PASSING_THROUGH_NON_SDCH);
+        cause = RESPONSE_ENCODING_LIE;
       }
+      DCHECK_NE(RESPONSE_NONE, cause);
+      UMA_HISTOGRAM_ENUMERATION(
+          (filter_context_.IsCachedContent() ?
+           "Sdch3.ResponseCorruptionDetectionCached" :
+           "Sdch3.ResponseCorruptionDetectionUncached"),

[Alexei Svitkine (slow), 2014/09/09 18:12:01]

This is incorrect to do. The first invocation of the macro will cache the
Histogram object at the call site. So you shouldn't use a variable-value string
for the first param.

Please make it an else if and maybe add a comment about it.

[Randy Smith (Not in Mondays), 2014/09/09 18:27:06]

Whoops; thank you.  

I'm not sure what you wanted the comment about.  I put in a comment about the
caching of the histogram name; let me know if that's not what you meant.

+          cause, RESPONSE_MAX);
 
       if (decoding_status_ == PASS_THROUGH) {
         dest_buffer_excess_ = dictionary_hash_;  // Send what we scanned.