Safe-browsing PrefixSet cleanups.

Safe-browsing PrefixSet cleanups.

Make sure SBPrefix is a fixed size.
PrefixSet tests for single-element set, set with large deltas, and
  int32 space edge cases.
PrefixSet::GetPrefixes() can be const.
Consolidate the SafeBrowsingDatabase GetPrefixes() checking code.
Check whether deltas fit by directly checking whether the delta fit.
Add a histogram for checking if SBPrefix really was crazy.

BUG=71832
TEST=none

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

[Scott Hess - ex-Googler, 2011-03-17 22:58:36]

I have an additional change which does more thorough checking of the data, with
additional histograms.  But I keep getting distracted by unrelated stuff, and
I'm OOT next week, so this collects various dribs and drabs which seem good to
get in sooner rather than later.

[Scott Hess - ex-Googler, 2011-03-17 23:15:36]

On 2011/03/17 22:58:36, shess wrote:
> I have an additional change which does more thorough checking of the data,
with
> additional histograms.  But I keep getting distracted by unrelated stuff, and
> I'm OOT next week, so this collects various dribs and drabs which seem good to
> get in sooner rather than later.

First-case logging occurred to me as a way to isolate whether this is all coming
from a single user or not.

[lzheng, 2011-03-18 00:37:34]

LGTM.

I can take another look if you agree we should a histograms to catch if it is
the size not matching or the restored not equal to the original?

http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/prefix_set.cc (right):

http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/prefix_set.cc:62: if (delta !=
static_cast<unsigned>(delta16) || run_length >= kMaxRun) {
I feel delta >= kMaxDelta is the same as delta !=
static_cast<unsigned>(delta16). Can you confirm?

http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:249:
RecordPrefixSetInfo(PREFIX_SET_GETPREFIXES_BROKEN);
When you get here, do you want to see if it is restored.size() != unique.size()
or std::equal(unique.begin(), unique.end(), restored.begin())?

[Scott Hess - ex-Googler, 2011-03-18 02:04:59]

On 2011/03/18 00:37:34, lzheng wrote:
> I can take another look if you agree we should
> a histograms to catch if it is the size not
> matching or the restored not equal to the original?

I added a new histogram for the size being different, and left the existing one
stand for continuity with the previous code.

http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/prefix_set.cc (right):

http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/prefix_set.cc:62: if (delta !=
static_cast<unsigned>(delta16) || run_length >= kMaxRun) {
On 2011/03/18 00:37:34, lzheng wrote:
> I feel delta >= kMaxDelta is the same as delta !=
> static_cast<unsigned>(delta16). Can you confirm?

Should be identical.  It was one of the notions that occurred to me while going
over things looking for possible off-by-one and other edge cases.

It also occurred to me that we never encode 0, so we could use that, but I
decided that the gain was really minimal for the complexity (the "delta too
large" case only fires a dozen times or so for the current safe-browsing list,
so 65536 might never be encoded ever).

http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:249:
RecordPrefixSetInfo(PREFIX_SET_GETPREFIXES_BROKEN);
On 2011/03/18 00:37:34, lzheng wrote:
> When you get here, do you want to see if it is restored.size() !=
unique.size()
> or std::equal(unique.begin(), unique.end(), restored.begin())?

Actually, I had another histogram for this, I can pull it over.

[lzheng1, 2011-03-18 02:27:53]

LGTM

On Thu, Mar 17, 2011 at 7:04 PM, <shess@chromium.org> wrote:

> On 2011/03/18 00:37:34, lzheng wrote:
>
>> I can take another look if you agree we should
>> a histograms to catch if it is the size not
>> matching or the restored not equal to the original?
>>
>
> I added a new histogram for the size being different, and left the existing
> one
> stand for continuity with the previous code.
>
>
>
>
>
http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
> File chrome/browser/safe_browsing/prefix_set.cc (right):
>
>
>
http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/prefix_set.cc:62: if (delta !=
> static_cast<unsigned>(delta16) || run_length >= kMaxRun) {
> On 2011/03/18 00:37:34, lzheng wrote:
>
>> I feel delta >= kMaxDelta is the same as delta !=
>> static_cast<unsigned>(delta16). Can you confirm?
>>
>
> Should be identical.  It was one of the notions that occurred to me
> while going over things looking for possible off-by-one and other edge
> cases.
>
> It also occurred to me that we never encode 0, so we could use that, but
> I decided that the gain was really minimal for the complexity (the
> "delta too large" case only fires a dozen times or so for the current
> safe-browsing list, so 65536 might never be encoded ever).
>
>
>
>
http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
> File chrome/browser/safe_browsing/safe_browsing_database.cc (right):
>
>
>
http://codereview.chromium.org/6711021/diff/1006/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/safe_browsing_database.cc:249:
> RecordPrefixSetInfo(PREFIX_SET_GETPREFIXES_BROKEN);
> On 2011/03/18 00:37:34, lzheng wrote:
>
>> When you get here, do you want to see if it is restored.size() !=
>>
> unique.size()
>
>> or std::equal(unique.begin(), unique.end(), restored.begin())?
>>
>
> Actually, I had another histogram for this, I can pull it over.
>
>
> http://codereview.chromium.org/6711021/
>