Added new histogram for analyzing SDCH corruption detection cases.

Added new histogram for analyzing SDCH corruption detection cases.

BUG=None
R=mmenke@chromium.org

Committed: https://crrev.com/f63db7f6e217b2b6895bbc3640ad7ec070096b63
Cr-Commit-Position: refs/heads/master@{#294005}

[Randy Smith (Not in Mondays), 2014-09-04 19:57:02]

Matt: Tossing at you since you have the context from our conversation yesterday;
PTAL?

To summarize for other interested readers: Currently, if SDCH decoding fails
(and there's a reasonable chance that the response is corrupted rather than just
unencoded content), the response is either to return fake HTML that will cause a
meta-refresh, or to fail the request, along with blacklisting the domain from
which the error came for some time.  I'd like to reduce or eliminate the
frequency with which this happens.  

The sledgehammer way to do that is to have the URLRequestHttpJob retry the
request without SDCH dictionary advertisement when corruption is detected.  But
this turns out to be very hard (probably not impossible?) in the context of the
current network stack architecture.  It may be possible to reduce the failure
rate to negligible levels by getting rid of the primary causes for the
meta-refresh/fail.  For that, we need to know what those primary causes are.

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

Note:  I haven't dug into the rest of the SDCH code, since I just don't have the
time to do so.

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

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:183: RESPONSE_200_CACHED : RESPONSE_200);
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.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:183: RESPONSE_200_CACHED : RESPONSE_200);
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.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:188:
SdchManager::SdchErrorRecovery(SdchManager::PASS_THROUGH_OLD_CACHED);
Having both SdchManager::SdchErrorRecovery and RecordCorruptionDetectionCause
makes me a little sad.  :(  We may need them both, though

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:188:
SdchManager::SdchErrorRecovery(SdchManager::PASS_THROUGH_OLD_CACHED);
Question unrelated to this CL:  We end up trying to use this filter in the case
SDCH is supported, but we don't get a response with an SDCH Content-Encoding? 
If that's true, cab we end up here both in the case we didn't have the SDCH
dictionary before...And in the case it was compressed with an older version of
the SDCH dictionary, but we now have a newer one?  We currently only update our
SDCH dictionary after restarting Chrome, but could happen across restarts, and
there's talk about changing that.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:198:
SdchManager::SdchErrorRecovery(SdchManager::DISCARD_TENTATIVE_SDCH);
This looks like a bug to me:  We aren't actually recovering here, are we?  We
could end up logging both this and SdchManager::META_REFRESH_CACHED_RECOVERY (Or
any of the others in the other FILTER_ERROR block below, right?)

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:206: TENTATIVE_SDCH_CACHED : TENTATIVE_SDCH);
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?)

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:220: NON_SDCH_RESPONSE_CACHED : NON_SDCH_RESPONSE);
This is weird.  If it is an SDCH response, we record that it's a
NON_SDCH_RESPONSE?

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.h
File net/filter/sdch_filter.h (right):

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.h#new...
net/filter/sdch_filter.h:58: enum CorruptionDetectionCause {
This stuff doesn't need to be in the header.  Can just put it in an anonymous
namespace in the cc file.

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

Comments incorporated; let me know what you think.

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

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:183: RESPONSE_200_CACHED : RESPONSE_200);
On 2014/09/05 14:52:27, mmenke wrote:
> 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.

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

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:183: RESPONSE_200_CACHED : RESPONSE_200);
On 2014/09/05 14:52:27, mmenke wrote:
> 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.

Moot; saw other comment :-}.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:188:
SdchManager::SdchErrorRecovery(SdchManager::PASS_THROUGH_OLD_CACHED);
On 2014/09/05 14:52:27, mmenke wrote:
> Having both SdchManager::SdchErrorRecovery and RecordCorruptionDetectionCause
> makes me a little sad.  :(  We may need them both, though

I thought about expanding SdchErrorRecovery's domain so that I could separate
out all the cases I'm interested in.  But interpreting that histogram is already
a pain because there's one element that shows up *way* more than the others. 
And I already feel like different portions of that space are being used in
different ways conceptually.  So I decided I wanted to go towards finer
granularity with more conceptually consistency within each one.  If you'd like
to push back against that, let me know--it probably means there's an option I'm
not considering.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:188:
SdchManager::SdchErrorRecovery(SdchManager::PASS_THROUGH_OLD_CACHED);
On 2014/09/05 14:52:26, mmenke wrote:
> Question unrelated to this CL:  We end up trying to use this filter in the
case
> SDCH is supported, but we don't get a response with an SDCH Content-Encoding? 
> If that's true, cab we end up here both in the case we didn't have the SDCH
> dictionary before...And in the case it was compressed with an older version of
> the SDCH dictionary, but we now have a newer one?  We currently only update
our
> SDCH dictionary after restarting Chrome, but could happen across restarts, and
> there's talk about changing that.

So it might be quicker to talk about this in person, but here's the answer to
the questions I think you're asking:
* The SDCH protocol specifies that if the client advertises an SDCH dictionary,
the server *must* either respond with an "X-SDCH-Encode: 0" or encode with that
dictionary.  This is to handle middleboxes that strip away unknown
Content-Encoding.  It disturbs me, but I'm neither looking to change the
protocol at the moment nor am sure how I would change it to avoid this.
* We specify the dictionaries by hash that we're prepared to accept in our
request headers; the server's only allowed to use those dictionaries to encode
the response.  If they use some other dictionary (including a new revision, with
a new hash), that's an error.  Having said that, the hash space isn't *that*
large (first 48 bits of the SHA256 of the dictionary text) and a collision might
happen, landing us in this code (though not this condition; the dictionary has
would be plausible in that case).  It probably won't happen very often :-J.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:198:
SdchManager::SdchErrorRecovery(SdchManager::DISCARD_TENTATIVE_SDCH);
On 2014/09/05 14:52:26, mmenke wrote:
> This looks like a bug to me:  We aren't actually recovering here, are we?  We
> could end up logging both this and SdchManager::META_REFRESH_CACHED_RECOVERY
(Or
> any of the others in the other FILTER_ERROR block below, right?)

I agree with you that there looks like a bug here, at least if you consider two
calls to SdchErrorRecovery() a bug.  I noticed it but put my blinders back on. 
Now that you call it out, I wonder if it's clean for me to simply drop the call
to SdchErrorRecovery here and drop DISCARD_TENTATIVE_SDCH as a valid value for
that histogram, since my new histogram covers it in (IMO) a more understandable
way.  Do you think I can do that without revving the SdchErrorRecovery
histogram?  (I've been sorta avoiding that since it seems like if I'm going to
do that I should rethink the entire namespace.)

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:206: TENTATIVE_SDCH_CACHED : TENTATIVE_SDCH);
On 2014/09/05 14:52:27, mmenke wrote:
> 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?)

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.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:220: NON_SDCH_RESPONSE_CACHED : NON_SDCH_RESPONSE);
On 2014/09/05 14:52:27, mmenke wrote:
> This is weird.  If it is an SDCH response, we record that it's a
> NON_SDCH_RESPONSE?

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.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.h
File net/filter/sdch_filter.h (right):

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.h#new...
net/filter/sdch_filter.h:58: enum CorruptionDetectionCause {
On 2014/09/05 14:52:27, mmenke wrote:
> This stuff doesn't need to be in the header.  Can just put it in an anonymous
> namespace in the cc file.

Done.

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

Quick responses.  I'll actually do a review on Monday.

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

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:183: RESPONSE_200_CACHED : RESPONSE_200);
On 2014/09/07 01:14:16, rdsmith wrote:
> On 2014/09/05 14:52:27, mmenke wrote:
> > 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.
> 
> 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.)

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

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:188:
SdchManager::SdchErrorRecovery(SdchManager::PASS_THROUGH_OLD_CACHED);
On 2014/09/07 01:14:16, rdsmith wrote:
> On 2014/09/05 14:52:26, mmenke wrote:
> > Question unrelated to this CL:  We end up trying to use this filter in the
> case
> > SDCH is supported, but we don't get a response with an SDCH
Content-Encoding? 
> > If that's true, cab we end up here both in the case we didn't have the SDCH
> > dictionary before...And in the case it was compressed with an older version
of
> > the SDCH dictionary, but we now have a newer one?  We currently only update
> our
> > SDCH dictionary after restarting Chrome, but could happen across restarts,
and
> > there's talk about changing that.
> 
> So it might be quicker to talk about this in person, but here's the answer to
> the questions I think you're asking:
> * The SDCH protocol specifies that if the client advertises an SDCH
dictionary,
> the server *must* either respond with an "X-SDCH-Encode: 0" or encode with
that
> dictionary.  This is to handle middleboxes that strip away unknown
> Content-Encoding.  It disturbs me, but I'm neither looking to change the
> protocol at the moment nor am sure how I would change it to avoid this.
> * We specify the dictionaries by hash that we're prepared to accept in our
> request headers; the server's only allowed to use those dictionaries to encode
> the response.  If they use some other dictionary (including a new revision,
with
> a new hash), that's an error.  Having said that, the hash space isn't *that*
> large (first 48 bits of the SHA256 of the dictionary text) and a collision
might
> happen, landing us in this code (though not this condition; the dictionary has
> would be plausible in that case).  It probably won't happen very often :-J.
> 

You misunderstood me.  It may indeed be simpler to talk in person.  What I was
thinking about:

We have dictionary #1, send the request, get an SDCHed response using dictionary
#1, and stick it in our cache.

At some later point, dictionary #1 expires, and we replace it with dictionary
#2.  We then try to request the file again.  We get the cache entry which *has
not* expired, and try using dictionary #2 for the file.  Since we're getting the
file from the cache, we get the version using dictionary #1.  Then would we also
end up here?

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:198:
SdchManager::SdchErrorRecovery(SdchManager::DISCARD_TENTATIVE_SDCH);
On 2014/09/07 01:14:16, rdsmith wrote:
> On 2014/09/05 14:52:26, mmenke wrote:
> > This looks like a bug to me:  We aren't actually recovering here, are we? 
We
> > could end up logging both this and SdchManager::META_REFRESH_CACHED_RECOVERY
> (Or
> > any of the others in the other FILTER_ERROR block below, right?)
> 
> I agree with you that there looks like a bug here, at least if you consider
two
> calls to SdchErrorRecovery() a bug.  I noticed it but put my blinders back on.

> Now that you call it out, I wonder if it's clean for me to simply drop the
call
> to SdchErrorRecovery here and drop DISCARD_TENTATIVE_SDCH as a valid value for
> that histogram, since my new histogram covers it in (IMO) a more
understandable
> way.  Do you think I can do that without revving the SdchErrorRecovery
> histogram?  (I've been sorta avoiding that since it seems like if I'm going to
> do that I should rethink the entire namespace.)

I'd remove it, and leave a gap in the values for the enumeration, with a comment
that the removed position is for an obsolete value.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:220: NON_SDCH_RESPONSE_CACHED : NON_SDCH_RESPONSE);
On 2014/09/07 01:14:16, rdsmith wrote:
> On 2014/09/05 14:52:27, mmenke wrote:
> > This is weird.  If it is an SDCH response, we record that it's a
> > NON_SDCH_RESPONSE?
> 
> 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.  

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]

Quick responses to your quick responses.

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

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:188:
SdchManager::SdchErrorRecovery(SdchManager::PASS_THROUGH_OLD_CACHED);
On 2014/09/07 01:46:03, mmenke wrote:
> On 2014/09/07 01:14:16, rdsmith wrote:
> > On 2014/09/05 14:52:26, mmenke wrote:
> > > Question unrelated to this CL:  We end up trying to use this filter in the
> > case
> > > SDCH is supported, but we don't get a response with an SDCH
> Content-Encoding? 
> > > If that's true, cab we end up here both in the case we didn't have the
SDCH
> > > dictionary before...And in the case it was compressed with an older
version
> of
> > > the SDCH dictionary, but we now have a newer one?  We currently only
update
> > our
> > > SDCH dictionary after restarting Chrome, but could happen across restarts,
> and
> > > there's talk about changing that.
> > 
> > So it might be quicker to talk about this in person, but here's the answer
to
> > the questions I think you're asking:
> > * The SDCH protocol specifies that if the client advertises an SDCH
> dictionary,
> > the server *must* either respond with an "X-SDCH-Encode: 0" or encode with
> that
> > dictionary.  This is to handle middleboxes that strip away unknown
> > Content-Encoding.  It disturbs me, but I'm neither looking to change the
> > protocol at the moment nor am sure how I would change it to avoid this.
> > * We specify the dictionaries by hash that we're prepared to accept in our
> > request headers; the server's only allowed to use those dictionaries to
encode
> > the response.  If they use some other dictionary (including a new revision,
> with
> > a new hash), that's an error.  Having said that, the hash space isn't *that*
> > large (first 48 bits of the SHA256 of the dictionary text) and a collision
> might
> > happen, landing us in this code (though not this condition; the dictionary
has
> > would be plausible in that case).  It probably won't happen very often :-J.
> > 
> 
> You misunderstood me.  It may indeed be simpler to talk in person.  What I was
> thinking about:
> 
> We have dictionary #1, send the request, get an SDCHed response using
dictionary
> #1, and stick it in our cache.
> 
> At some later point, dictionary #1 expires, and we replace it with dictionary
> #2.  We then try to request the file again.  We get the cache entry which *has
> not* expired, and try using dictionary #2 for the file.  Since we're getting
the
> file from the cache, we get the version using dictionary #1.  Then would we
also
> end up here?

I'll take a swing at a response, but yeah, let's queue up a talk in person.  I
think the case you suggest is plausible (I think Ricardo's made some comment
about the dictionary having been fetched later than the original item, and hence
would expire later, but I suspect there are corner cases, and in any case, it's
a heck of a thing to rely on).  But in that case dictionary_hash_is_plausible_
would be true, it would just refer to a dictionary that we don't have (#1
instead of #2).   So I think we'd end up in the DICTIONARY_NOT_FOUND case below.
 

I'll put some thought into whether or not we have enough information to
disambiguate between the "have a new dictionary but a dated cache item" and
"flushed the dictionary between advertising the request and getting back the
response", which I'd like this code to do.  I'm also wondering where we'd end up
if we *didn't* advertise an SDCH dictionary but got back a cached response that
was encoded with some dictionary (that we presumably don't have anymore); I'm
slightly worried that's in the dictionary_hash_is_plausible_ case below, and I
may want to disambiguate it.

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:220: NON_SDCH_RESPONSE_CACHED : NON_SDCH_RESPONSE);
On 2014/09/07 01:46:03, mmenke wrote:
> On 2014/09/07 01:14:16, rdsmith wrote:
> > On 2014/09/05 14:52:27, mmenke wrote:
> > > This is weird.  If it is an SDCH response, we record that it's a
> > > NON_SDCH_RESPONSE?
> > 
> > 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.  
> 
> 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.

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

[mmenke, 2014-09-08 16:12:13]

Just one issue.

https://codereview.chromium.org/537403003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/537403003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:49215: +  <int value="7"
label="NON_SDCH_RESPONSE_CACHED"/>
These need to be updated.

[Randy Smith (Not in Mondays), 2014-09-08 19:21:08]

Matt: In addition to the comments below, I also removed TENTATIVE_SDCH & changed
IsSdchResponse() to SdchResponseExpected(), which looking at the code strikes me
as a better description of what that boolean means.  

Also, I'm finding that there's some flakiness in the SdchBrowserTest, both in
this branch and on trunk (I know, you're going to have a heart attack and die
from that big surprise :-}).  That's upping the priority of my implementing the
fixes we talked about (as the flakiness is no longer theoretical) but I'm not
planning to do that in this CL.

Let me know what you think.

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

https://codereview.chromium.org/537403003/diff/1/net/filter/sdch_filter.cc#ne...
net/filter/sdch_filter.cc:188:
SdchManager::SdchErrorRecovery(SdchManager::PASS_THROUGH_OLD_CACHED);
On 2014/09/08 01:56:12, rdsmith wrote:
> On 2014/09/07 01:46:03, mmenke wrote:
> > On 2014/09/07 01:14:16, rdsmith wrote:
> > > On 2014/09/05 14:52:26, mmenke wrote:
> > > > Question unrelated to this CL:  We end up trying to use this filter in
the
> > > case
> > > > SDCH is supported, but we don't get a response with an SDCH
> > Content-Encoding? 
> > > > If that's true, cab we end up here both in the case we didn't have the
> SDCH
> > > > dictionary before...And in the case it was compressed with an older
> version
> > of
> > > > the SDCH dictionary, but we now have a newer one?  We currently only
> update
> > > our
> > > > SDCH dictionary after restarting Chrome, but could happen across
restarts,
> > and
> > > > there's talk about changing that.
> > > 
> > > So it might be quicker to talk about this in person, but here's the answer
> to
> > > the questions I think you're asking:
> > > * The SDCH protocol specifies that if the client advertises an SDCH
> > dictionary,
> > > the server *must* either respond with an "X-SDCH-Encode: 0" or encode with
> > that
> > > dictionary.  This is to handle middleboxes that strip away unknown
> > > Content-Encoding.  It disturbs me, but I'm neither looking to change the
> > > protocol at the moment nor am sure how I would change it to avoid this.
> > > * We specify the dictionaries by hash that we're prepared to accept in our
> > > request headers; the server's only allowed to use those dictionaries to
> encode
> > > the response.  If they use some other dictionary (including a new
revision,
> > with
> > > a new hash), that's an error.  Having said that, the hash space isn't
*that*
> > > large (first 48 bits of the SHA256 of the dictionary text) and a collision
> > might
> > > happen, landing us in this code (though not this condition; the dictionary
> has
> > > would be plausible in that case).  It probably won't happen very often
:-J.
> > > 
> > 
> > You misunderstood me.  It may indeed be simpler to talk in person.  What I
was
> > thinking about:
> > 
> > We have dictionary #1, send the request, get an SDCHed response using
> dictionary
> > #1, and stick it in our cache.
> > 
> > At some later point, dictionary #1 expires, and we replace it with
dictionary
> > #2.  We then try to request the file again.  We get the cache entry which
*has
> > not* expired, and try using dictionary #2 for the file.  Since we're getting
> the
> > file from the cache, we get the version using dictionary #1.  Then would we
> also
> > end up here?
> 
> I'll take a swing at a response, but yeah, let's queue up a talk in person.  I
> think the case you suggest is plausible (I think Ricardo's made some comment
> about the dictionary having been fetched later than the original item, and
hence
> would expire later, but I suspect there are corner cases, and in any case,
it's
> a heck of a thing to rely on).  But in that case dictionary_hash_is_plausible_
> would be true, it would just refer to a dictionary that we don't have (#1
> instead of #2).   So I think we'd end up in the DICTIONARY_NOT_FOUND case
below.
>  
> 
> I'll put some thought into whether or not we have enough information to
> disambiguate between the "have a new dictionary but a dated cache item" and
> "flushed the dictionary between advertising the request and getting back the
> response", which I'd like this code to do.  I'm also wondering where we'd end
up
> if we *didn't* advertise an SDCH dictionary but got back a cached response
that
> was encoded with some dictionary (that we presumably don't have anymore); I'm
> slightly worried that's in the dictionary_hash_is_plausible_ case below, and I
> may want to disambiguate it.

I'm going to push this code tracing exercise out of this CL, unless you object.

https://codereview.chromium.org/537403003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/537403003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:49215: +  <int value="7"
label="NON_SDCH_RESPONSE_CACHED"/>
On 2014/09/08 16:12:12, mmenke wrote:
> These need to be updated.

Whoops; thank you.

[mmenke, 2014-09-08 19:27:27]

LGTM.

On 2014/09/08 19:21:08, rdsmith wrote:
> Matt: In addition to the comments below, I also removed TENTATIVE_SDCH &
changed
> IsSdchResponse() to SdchResponseExpected(), which looking at the code strikes
me
> as a better description of what that boolean means.  
> 
> Also, I'm finding that there's some flakiness in the SdchBrowserTest, both in
> this branch and on trunk (I know, you're going to have a heart attack and die
> from that big surprise :-}).  That's upping the priority of my implementing
the
> fixes we talked about (as the flakiness is no longer theoretical) but I'm not
> planning to do that in this CL.
> 
> Let me know what you think.

SGTM.

https://codereview.chromium.org/537403003/diff/40001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/537403003/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:130: // defunct = 73,  // PASSING_THROUGH_NON_SDCH plus
defunct 81.
Not sure this comment change really makes things a whole lot clearer.

https://codereview.chromium.org/537403003/diff/40001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/537403003/diff/40001/net/filter/sdch_filter.c...
net/filter/sdch_filter.cc:265: }
DCHECK_NE(RESPONSE_NONE, cause); ?

[Randy Smith (Not in Mondays), 2014-09-08 20:10:10]

rdsmith@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Randy Smith (Not in Mondays), 2014-09-08 20:10:11]

Matt: Thanks!

Alexei: Could you review tools/metrics/histograms.xml?

https://codereview.chromium.org/537403003/diff/40001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/537403003/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:130: // defunct = 73,  // PASSING_THROUGH_NON_SDCH plus
defunct 81.
On 2014/09/08 19:27:27, mmenke wrote:
> Not sure this comment change really makes things a whole lot clearer.

It's a dangling reference without some change.  I've tweaked it to point at the
other enum (though that's a comment pointer layering violation; if you care
about that, I can clean it up).

https://codereview.chromium.org/537403003/diff/40001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/537403003/diff/40001/net/filter/sdch_filter.c...
net/filter/sdch_filter.cc:265: }
On 2014/09/08 19:27:27, mmenke wrote:
> DCHECK_NE(RESPONSE_NONE, cause); ?

Done.

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

https://codereview.chromium.org/537403003/diff/60001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/537403003/diff/60001/net/filter/sdch_filter.c...
net/filter/sdch_filter.cc:270: "Sdch3.ResponseCorruptionDetectionUncached"),
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.

https://codereview.chromium.org/537403003/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/537403003/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:28611: +<histogram
name="Sdch3.ResponseCorruptionDetectionCached"
Nit: I'd rename this (and the corresponding C++) to
Sdch3.ResponseCorruptionDetection.Cached (and the one below to
Sdch3.ResponseCorruptionDetection.Uncached). This will provide better grouping
on the dashboard.

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

Incorporated comments.

https://codereview.chromium.org/537403003/diff/60001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/537403003/diff/60001/net/filter/sdch_filter.c...
net/filter/sdch_filter.cc:270: "Sdch3.ResponseCorruptionDetectionUncached"),
On 2014/09/09 18:12:01, Alexei Svitkine wrote:
> 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.

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.

https://codereview.chromium.org/537403003/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/537403003/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:28611: +<histogram
name="Sdch3.ResponseCorruptionDetectionCached"
On 2014/09/09 18:12:01, Alexei Svitkine wrote:
> Nit: I'd rename this (and the corresponding C++) to
> Sdch3.ResponseCorruptionDetection.Cached (and the one below to
> Sdch3.ResponseCorruptionDetection.Uncached). This will provide better grouping
> on the dashboard.

Good idea; thank you.  Done.

[Alexei Svitkine (slow), 2014-09-09 18:28:04]

lgtm

[Randy Smith (Not in Mondays), 2014-09-09 18:28:55]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-09-09 18:39:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rdsmith@chromium.org/537403003/80001

[commit-bot: I haz the power, 2014-09-09 20:54:00]

Committed patchset #5 (id:80001) as 2526f3196fd24acbe2232f33e53cd2a06b512345

[commit-bot: I haz the power, 2014-09-10 03:55:30]

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