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..2d78aefeedae115cddf8830615e8dafd45ccbe10 100644
--- a/net/filter/sdch_filter.cc
+++ b/net/filter/sdch_filter.cc
@@ -179,6 +179,8 @@ Filter::FilterStatus SdchFilter::ReadFilteredData(char* dest_buffer,
         decoding_status_ = PASS_THROUGH;
       } else if (filter_context_.GetResponseCode() != 200) {
         // We need to meta-refresh, with SDCH disabled.
+        RecordCorruptionDetectionCause(filter_context_.IsCachedContent() ?
+                               RESPONSE_200_CACHED : RESPONSE_200);

[mmenke, 2014/09/05 14:52:27]

nit:  this doesn't follow the indentation rules.  I suggest either:

RecordCorruptionDetectionCause(filter_context_.IsCachedContent() ?
                                   RESPONSE_200_CACHED :
                                   RESPONSE_200);

RecordCorruptionDetectionCause(
    filter_context_.IsCachedContent() ?
        RESPONSE_200_CACHED : RESPONSE_200);

Alternatively, see other comment.

[mmenke, 2014/09/05 14:52:27]

I think it's weird that we're only recording a subset of corruption cases in
both this and the SdchManager::SdchErrorRecovery cases.  Getting a total count
from the two histograms seems difficult, if not impossible.

I suggest adding a CorruptionDetectionCause outside the if, set the cause in
each if branch, and then record the histogram at the end.  Wouldn't need the
function in that case, either.

[Randy Smith (Not in Mondays), 2014/09/07 01:14:16]

Done.  (I agree with you about SdchErrorRecovery; it feels like we should clean
that up.  But I'm not taking that on in this CL, unless you insist.)

[Randy Smith (Not in Mondays), 2014/09/07 01:14:16]

Moot; saw other comment :-}.

[mmenke, 2014/09/07 01:46:03]

I agree that we shouldn't clean it up (Or fix the issue I noted about it below)
in this CL.

       } else if (filter_context_.IsCachedContent()
                  && !dictionary_hash_is_plausible_) {
         // We must have hit the back button, and gotten content that was fetched
@@ -200,14 +202,22 @@ Filter::FilterStatus SdchFilter::ReadFilteredData(char* dest_buffer,
         // the meta-refresh result.
         // TODO(jar): Improve robustness by sniffing for valid text that we can
         // actual use re: decoding_status_ = PASS_THROUGH;
+        RecordCorruptionDetectionCause(filter_context_.IsCachedContent() ?
+                               TENTATIVE_SDCH_CACHED : TENTATIVE_SDCH);

[mmenke, 2014/09/05 14:52:27]

I know this is the language used elsewhere, but I think TENTATIVE_SDCH is a bit
ambiguous.  I don't have a better name, so I suggest text in your histogram
enumeration instead (Given all the text here, maybe just a pointer to this
function at the start of the enum?)

[Randy Smith (Not in Mondays), 2014/09/07 01:14:16]

I've played around some with the doc, both pointing in pointers as you've
suggested and putting some short comments at the enum definition.  Let me know
what you think.

       } 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.
+        RecordCorruptionDetectionCause(
+            filter_context_.IsCachedContent() ?
+            DICTIONARY_NOT_FOUND_CACHED : DICTIONARY_NOT_FOUND);
       } else if (filter_context_.IsSdchResponse()) {
         // 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.
+        RecordCorruptionDetectionCause(
+            filter_context_.IsCachedContent() ?
+            NON_SDCH_RESPONSE_CACHED : NON_SDCH_RESPONSE);

[mmenke, 2014/09/05 14:52:27]

This is weird.  If it is an SDCH response, we record that it's a
NON_SDCH_RESPONSE?

[Randy Smith (Not in Mondays), 2014/09/07 01:14:16]

IsSdchResponse() being true means that we advertised a dictionary and the server
didn't shoot us down with an X-SDCH-Encode: 0.  This section of the if means
that the dictionary has wasn't plausible.  Therefore, I call it a non-SDCH
response.  (I'm not sure how we get into the final else clause of the if.)  But
I hear you about it being confused; changed to CORRUPT_SDCH_RESPONSE.  

[mmenke, 2014/09/07 01:46:03]

Ahh...  IsSdchResponse means is this a response to an SDCH request, since we
also use this filter in the case we...erm...get an SDCH response to a non-SDCH
request, in which case IsSdchResponse would be false...  I think the naming
there is more than a little confusing, though I haven't made any attempt to
understand the rest of the code here.

[Randy Smith (Not in Mondays), 2014/09/08 01:56:12]

I tend to agree with you, although that may also be because I'm still finding
the code a little bit confusing.  I need to get more on top of this code (and
then maybe clean it up  bit).

       } 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.
@@ -393,4 +403,10 @@ int SdchFilter::OutputBufferExcess(char* const dest_buffer,
   return amount;
 }
 
+void SdchFilter::RecordCorruptionDetectionCause(
+    CorruptionDetectionCause cause) {
+ UMA_HISTOGRAM_ENUMERATION(
+     "Sdch3.CorruptionDetectionCause", cause, META_REFRESH_CAUSE_MAX);
+}
+
 }  // namespace net