Added stats on why blacklisted requests were blacklisted.

Added stats on why blacklisted requests were blacklisted.

BUG=None
R=jar@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289343

[Randy Smith (Not in Mondays), 2014-07-22 20:15:05]

Jim: PTAL?  This is to actually debug where the blacklist domain spike is coming
from.

[jar (doing other things), 2014-07-23 14:02:53]

I have to read this more carefully when I get back... one small nit for now from
a brief scan.

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

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.h#newc...
net/base/sdch_manager.h:345: BlacklistInfo()
nit: constuctor should be listed before members.

[jar (doing other things), 2014-08-05 23:33:30]

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:305: blacklist_info->reason = MIN_PROBLEM_CODE;
Why did you change to leaving the entry in the map, rather than erasing it??

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:309: std::string
domain_lower(StringToLowerASCII(domain));
Good bug fix! Old code assumed it was already lower.

nit: Perhaps you should rename |domain| to |domain_lower| in other methods
before use as an index?

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:348: UMA_HISTOGRAM_ENUMERATION("Sdch3.BlacklistReason",
it->second.reason,
Maybe this should go before line 343, since it may otherwise be cleared in line
345.  ...or is this useful in that context?

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

Incorporated all comments; PTAL?

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:305: blacklist_info->reason = MIN_PROBLEM_CODE;
On 2014/08/05 23:33:30, jar wrote:
> Why did you change to leaving the entry in the map, rather than erasing it??

Because the map now contains the exponential backoff information, which is
needed for the next time the domain is blacklisted.

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:309: std::string
domain_lower(StringToLowerASCII(domain));
On 2014/08/05 23:33:30, jar wrote:
> Good bug fix! Old code assumed it was already lower.
> 
> nit: Perhaps you should rename |domain| to |domain_lower| in other methods
> before use as an index?

Done (or got rid of any variable at all), everywhere but in the middle of
AddSdchDictionary, where it's used in a couple of different ways, not just as an
index.

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:348: UMA_HISTOGRAM_ENUMERATION("Sdch3.BlacklistReason",
it->second.reason,
On 2014/08/05 23:33:30, jar wrote:
> Maybe this should go before line 343, since it may otherwise be cleared in
line
> 345.  ...or is this useful in that context?

Ooops; thanks for the catch.  No, I think we still want the error code on the
last rejection.

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

https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.h#newc...
net/base/sdch_manager.h:345: BlacklistInfo()
On 2014/07/23 14:02:53, jar wrote:
> nit: constuctor should be listed before members.

Done.

[jar (doing other things), 2014-08-13 00:46:00]

One requested nit change, and then LGTM.

(FWIW: I think I wrote the original code that I'm asking you to change).

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

https://codereview.chromium.org/414563002/diff/40001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:83: if (target_url.SchemeIsSecure() !=
url_.SchemeIsSecure())
comment (probably no action required): Why did you outlaw (between the last
patch and now) the using an HTTPS acquired dictionary for an HTTP acquired
content?  Were you wary that you didn't want an attacker's HTTP content to
expose a dictionary that was provided via HTTP?

I think that is a good reason... although for all uses I know of for Google and
SDCH, the dictionary contains no Personally Identifiable Info.

That said... it seems prudent to fail safe... and demand secure dictionary iff
secure content.

https://codereview.chromium.org/414563002/diff/40001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:279: if (count > 0)
nit:  It has become "best practice" in the post few years to avoid inducing
overflows in signed integers.  Unsigned ints can handle overflows (and do 2's
complement arithmetic), but it is now considered "unsafe" to do so with signed
ints.

As a result, this logic should be changed.  Instead of watching for a sign
change, you should check before increasing count if there is chance of an
overflow.  Possibly code might check:

int count = (blacklist_info->exponential_count < INT_MAX / 4) ?
    1 + 2 * blacklist_info->exponential_count : INT_MAX;

[Randy Smith (Not in Mondays), 2014-08-13 01:45:11]

Thanks!

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

https://codereview.chromium.org/414563002/diff/40001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:83: if (target_url.SchemeIsSecure() !=
url_.SchemeIsSecure())
On 2014/08/13 00:46:00, jar wrote:
> comment (probably no action required): Why did you outlaw (between the last
> patch and now) the using an HTTPS acquired dictionary for an HTTP acquired
> content?  Were you wary that you didn't want an attacker's HTTP content to
> expose a dictionary that was provided via HTTP?
> 
> I think that is a good reason... although for all uses I know of for Google
and
> SDCH, the dictionary contains no Personally Identifiable Info.
> 
> That said... it seems prudent to fail safe... and demand secure dictionary iff
> secure content.

I was just following Chris' spec; see discussion at
https://groups.google.com/forum/#!topic/sdch/8EEBFYMIB78.

https://codereview.chromium.org/414563002/diff/40001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:279: if (count > 0)
On 2014/08/13 00:46:00, jar wrote:
> nit:  It has become "best practice" in the post few years to avoid inducing
> overflows in signed integers.  Unsigned ints can handle overflows (and do 2's
> complement arithmetic), but it is now considered "unsafe" to do so with signed
> ints.
> 
> As a result, this logic should be changed.  Instead of watching for a sign
> change, you should check before increasing count if there is chance of an
> overflow.  Possibly code might check:
> 
> int count = (blacklist_info->exponential_count < INT_MAX / 4) ?
>     1 + 2 * blacklist_info->exponential_count : INT_MAX;

Makes sense.  I've changed the logic, though I got precise/pedantic on the
INT_MAX calc.  Let me know what you think.

[Randy Smith (Not in Mondays), 2014-08-13 16:11:11]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-08-13 16:12:52]

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

[commit-bot: I haz the power, 2014-08-13 19:10:51]

Committed patchset #5 (80001) as 289343

[jar (doing other things), 2014-08-14 22:50:38]

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

https://codereview.chromium.org/414563002/diff/40001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:279: if (count > 0)
On 2014/08/13 01:45:10, rdsmith wrote:
> On 2014/08/13 00:46:00, jar wrote:
> > nit:  It has become "best practice" in the post few years to avoid inducing
> > overflows in signed integers.  Unsigned ints can handle overflows (and do
2's
> > complement arithmetic), but it is now considered "unsafe" to do so with
signed
> > ints.
> > 
> > As a result, this logic should be changed.  Instead of watching for a sign
> > change, you should check before increasing count if there is chance of an
> > overflow.  Possibly code might check:
> > 
> > int count = (blacklist_info->exponential_count < INT_MAX / 4) ?
> >     1 + 2 * blacklist_info->exponential_count : INT_MAX;
> 
> Makes sense.  I've changed the logic, though I got precise/pedantic on the
> INT_MAX calc.  Let me know what you think.

The distinction between 2^31 and 2^30 in the backoff counter is not critical. 
It is fun that you were precise... and it looks fine to me!