Try to detect internal corruption of histogram instances....

Try to detect internal corruption of histogram instances.

Corruptions can include changes in bucket boundaries,
large changes in sample counts (in a bucket), etc.

We now detect problems, and don't forward the corrupt
data any further. This means it won't exit the renderer
and go to the browser if corrupt, and it won't exit
the browser and be sent up via UMA if corrupt.

IF the would-be corruption is caused by a race to
snapshot the data, then a later snapshot should get
the clean copy, and all data (across the precluded
period) will be sent onward.

BUG=61281
r=mbelshe

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64687

[Mike Belshe, 2010-11-01 20:09:09]

lgtm with comments

http://codereview.chromium.org/4174002/diff/18001/19001
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/4174002/diff/18001/19001#newcode309
base/metrics/histogram.cc:309: }
nit:  It seems like there should be a function:
   int64 ComputeChecksum();

Then ResetRangeChecksum should be:
    range_checksum_ = ComputeChecksum();

and HasValidRangeChecksum should be:
    return range_checksum_ == ComputeChecksum();

http://codereview.chromium.org/4174002/diff/18001/19001#newcode551
base/metrics/histogram.cc:551: const int kCommonRaceBasedCountMismatch = 3;
nit:  I'd prefer 1 for slop (or 0 :-)  your call.

http://codereview.chromium.org/4174002/diff/18001/19002
File base/metrics/histogram.h (right):

http://codereview.chromium.org/4174002/diff/18001/19002#newcode401
base/metrics/histogram.h:401: Inconsistencies FindCorruption(const SampleSet&
snapshot) const;
nit:  rename "FindCorruption" to "VerifyData" or "CheckForCorruption"?

http://codereview.chromium.org/4174002/diff/18001/19003
File base/metrics/histogram_unittest.cc (right):

http://codereview.chromium.org/4174002/diff/18001/19003#newcode316
base/metrics/histogram_unittest.cc:316: 
nit:  nix blank line

http://codereview.chromium.org/4174002/diff/18001/19004
File chrome/common/metrics_helpers.cc (right):

http://codereview.chromium.org/4174002/diff/18001/19004#newcode496
chrome/common/metrics_helpers.cc:496: static ProblemMap* inconsistencies =
new(ProblemMap);
bug: shouldn't this be "new ProblemMap()"

http://codereview.chromium.org/4174002/diff/18001/19005
File chrome/renderer/renderer_histogram_snapshots.cc (right):

http://codereview.chromium.org/4174002/diff/18001/19005#newcode72
chrome/renderer/renderer_histogram_snapshots.cc:72: static ProblemMap*
inconsistencies = new(ProblemMap);
bug:  I think this needs to be "new ProblemMap()"

note also:  this static is not thread-safe on initialization.  But I think that
is ok here.

[jar (doing other things), 2010-11-01 21:33:55]

Changes made per comments by mbelshe.

http://codereview.chromium.org/4174002/diff/18001/19001
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/4174002/diff/18001/19001#newcode309
base/metrics/histogram.cc:309: }
On 2010/11/01 20:09:10, Mike Belshe wrote:
> nit:  It seems like there should be a function:
>    int64 ComputeChecksum();
> 
> Then ResetRangeChecksum should be:
>     range_checksum_ = ComputeChecksum();
> 
> and HasValidRangeChecksum should be:
>     return range_checksum_ == ComputeChecksum();
> 

Done.

http://codereview.chromium.org/4174002/diff/18001/19001#newcode551
base/metrics/histogram.cc:551: const int kCommonRaceBasedCountMismatch = 3;
On 2010/11/01 20:09:10, Mike Belshe wrote:
> nit:  I'd prefer 1 for slop (or 0 :-)  your call.

Done.

http://codereview.chromium.org/4174002/diff/18001/19002
File base/metrics/histogram.h (right):

http://codereview.chromium.org/4174002/diff/18001/19002#newcode401
base/metrics/histogram.h:401: Inconsistencies FindCorruption(const SampleSet&
snapshot) const;
Per our offline discussion, if we had a bool return, we should change the
name... but with the current return value identifying the nature (if any!) of
corruption, this name is about as good as any.

On 2010/11/01 20:09:10, Mike Belshe wrote:
> nit:  rename "FindCorruption" to "VerifyData" or "CheckForCorruption"?

http://codereview.chromium.org/4174002/diff/18001/19003
File base/metrics/histogram_unittest.cc (right):

http://codereview.chromium.org/4174002/diff/18001/19003#newcode316
base/metrics/histogram_unittest.cc:316: 
On 2010/11/01 20:09:10, Mike Belshe wrote:
> nit:  nix blank line

Done.

http://codereview.chromium.org/4174002/diff/18001/19004
File chrome/common/metrics_helpers.cc (right):

http://codereview.chromium.org/4174002/diff/18001/19004#newcode496
chrome/common/metrics_helpers.cc:496: static ProblemMap* inconsistencies =
new(ProblemMap);
On 2010/11/01 20:09:10, Mike Belshe wrote:
> bug: shouldn't this be "new ProblemMap()"

Done.

http://codereview.chromium.org/4174002/diff/18001/19005
File chrome/renderer/renderer_histogram_snapshots.cc (right):

http://codereview.chromium.org/4174002/diff/18001/19005#newcode72
chrome/renderer/renderer_histogram_snapshots.cc:72: static ProblemMap*
inconsistencies = new(ProblemMap);
On 2010/11/01 20:09:10, Mike Belshe wrote:
> bug:  I think this needs to be "new ProblemMap()"
> 
> note also:  this static is not thread-safe on initialization.  But I think
that
> is ok here.

Done.