Knock out injected safe-browsing prefixes.

Knock out injected safe-browsing prefixes.

The safe-browsing update code has long injected prefixes when a fullhash was
seen.  This causes incorrect behavior.  That case can be detected by observing
prefixes in the same chunk as fullhashes, which cannot be sent by the server.

BUG=361248
R=jar@chromium.org, mattm@chromium.org

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

[Scott Hess - ex-Googler, 2014-05-01 22:16:21]

Depends on https://codereview.chromium.org/266793002/ , else fails expected
post-read test in SafeBrowsingDatabaseTest.BrowseDatabase.

Replaces https://codereview.chromium.org/232223005/ , I'll add the
histograms.xml change I forgot from that CL.

[Scott Hess - ex-Googler, 2014-05-01 22:19:49]

On 2014/05/01 22:16:21, shess wrote:
> Depends on https://codereview.chromium.org/266793002/ , else fails expected
> post-read test in SafeBrowsingDatabaseTest.BrowseDatabase.
> 
> Replaces https://codereview.chromium.org/232223005/ , I'll add the
> histograms.xml change I forgot from that CL.

Hmm, I see that there's also a new test in that other CL.  Don't review this,
yet, I'll go look at whether that test is useful or not.

[Scott Hess - ex-Googler, 2014-05-01 22:41:41]

add ContainsBrowseUrl test.

[Scott Hess - ex-Googler, 2014-05-01 22:44:13]

OK, pulled in the test.  The last EXPECT of ContainsBrowseURL test was TRUE with
the collision in prefix_hits before this change, with the change it's false
(which I think is the expected result).  Doesn't seem worth checking the test in
separately to show the progress, though.

[mattm, 2014-05-02 01:41:08]

https://codereview.chromium.org/263833005/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_store_unittest.cc (right):

https://codereview.chromium.org/263833005/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_store_unittest.cc:334: // An add with
no corresponding sub.  Both items should be retained.
These comments are a little confusing since none of the adds and subs in this
test correspond.

[Scott Hess - ex-Googler, 2014-05-05 22:41:32]

In adjusting this, I realized that maybe it would also be useful to have a more
end-to-end test that file update works as expected, so I added a tests against
the existing v8 golden file.  Let me know if that's too much.

https://codereview.chromium.org/263833005/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_store_unittest.cc (right):

https://codereview.chromium.org/263833005/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_store_unittest.cc:334: // An add with
no corresponding sub.  Both items should be retained.
On 2014/05/02 01:41:09, mattm wrote:
> These comments are a little confusing since none of the adds and subs in this
> test correspond.

Sorry, these were copied and pasted from other tests.  This test really
shouldn't touch the interactions between subs and adds, just interactions w/in
each group independently.  I'll rearrange/recomment.  I'll also adjust the chunk
numbering to be more independent.

[mattm, 2014-05-06 00:04:47]

lgtm

[Scott Hess - ex-Googler, 2014-05-06 03:36:24]

Jim for histograms.xml.  Thanks!

[Scott Hess - ex-Googler, 2014-05-06 20:39:49]

Jim, ping for histograms.xml?  Thansk.


On Mon, May 5, 2014 at 8:36 PM, <shess@chromium.org> wrote:

> Jim for histograms.xml.  Thanks!
>
> https://codereview.chromium.org/263833005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jar (doing other things), 2014-05-07 00:12:11]

LGTM % nit.  *Please* expand on your prose.

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

https://codereview.chromium.org/263833005/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:22424: +    The number of prefixes
removed as part of http://crbug.com/361248 .
nit: IMO, it would be much better to include some prose that explained this,
rather than referring to code, or bugs.  The goal is to have these missives
explain what is going on.  I can almost get the same amount of info out of the
histogram's cryptic name, as I get out of this would-be-prose :-(.

[Scott Hess - ex-Googler, 2014-05-07 00:42:36]

additional color on the histogram.

[Scott Hess - ex-Googler, 2014-05-07 00:43:51]

PTAL at updated histogram text?

[jar (doing other things), 2014-05-07 01:37:20]

lgtm

[Scott Hess - ex-Googler, 2014-05-07 01:41:37]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2014-05-07 01:41:41]

On 2014/05/07 01:37:20, jar wrote:
> lgtm

Thanks!

[commit-bot: I haz the power, 2014-05-07 01:45:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shess@chromium.org/263833005/60001

[Scott Hess - ex-Googler, 2014-05-07 15:55:56]

The CQ bit was unchecked by shess@chromium.org

[Scott Hess - ex-Googler, 2014-05-07 15:56:11]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2014-05-07 16:00:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shess@chromium.org/263833005/60001

[Scott Hess - ex-Googler, 2014-05-07 16:35:51]

Committed patchset #4 manually as r268815 (presubmit successful).

[Scott Hess - ex-Googler, 2014-05-07 16:36:05]

On 2014/05/07 16:00:08, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/shess@chromium.org/263833005/60001

Sigh.  It spent all night stuck pending, and now it looks like that's where it's
at again.  I'm around for a few hours, dcommit'ing.