Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(462)

Issue 507653003: Remove safe-browsing code to fix up injected prefixes. (Closed)

Created:
6 years, 3 months ago by Scott Hess - ex-Googler
Modified:
6 years, 3 months ago
CC:
chromium-reviews, asvitkine+watch_chromium.org
Base URL:
http://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Remove safe-browsing code to fix up injected prefixes. An older implementation of the safe-browsing database code injected prefixes when receiving full hashes, due to an incorrect reading of how full-hash matches should happen. Current code checks the full hashes and prefixes independently, so the duplicate prefixes are no longer generated. This code removed duplicate prefixes found in existing databases. 99.99% of runs over the past two weeks have not removed any such duplicate prefixes. In the current code hitting a full hash has the same end result as hitting a prefix, so the primary downside of removing this code is slight excess storage. Insofar as full hashes are deleted by sub, there may be stale prefixes left behind. The stale prefixes will be deleted when the chunk in question is deleted. The result may be a small number of unexpected gethash requests which will return misses (and thus not affect the browsing experience other than adding a little latency). BUG=361248 Committed: https://crrev.com/6434e337b1c0eba3c30d497d7d1a785f4a3d1e5b Cr-Commit-Position: refs/heads/master@{#292019}

Patch Set 1 #

Patch Set 2 : Revert original CL more thoroughly. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -191 lines) Patch
M chrome/browser/safe_browsing/prefix_set.h View 1 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_store.cc View 1 3 chunks +0 lines, -54 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc View 1 2 chunks +2 lines, -73 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_store_unittest.cc View 1 chunk +0 lines, -63 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
Scott Hess - ex-Googler
shess@chromium.org changed reviewers: + asvitkine@chromium.org, mattm@chromium.org
6 years, 3 months ago (2014-08-26 19:40:08 UTC) #1
Scott Hess - ex-Googler
Original CL this mostly reverts: https://codereview.chromium.org/263833005/ asvitkine, histograms.xml change just marks something obsolete. mattm, let ...
6 years, 3 months ago (2014-08-26 19:40:08 UTC) #2
Alexei Svitkine (slow)
lgtm, but only looked at the xml
6 years, 3 months ago (2014-08-26 20:25:57 UTC) #3
mattm
lgtm
6 years, 3 months ago (2014-08-26 20:49:46 UTC) #4
Scott Hess - ex-Googler
The CQ bit was checked by shess@chromium.org
6 years, 3 months ago (2014-08-26 20:50:45 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shess@chromium.org/507653003/20001
6 years, 3 months ago (2014-08-26 20:51:17 UTC) #6
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux ...
6 years, 3 months ago (2014-08-26 22:20:41 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (20001) as 8e3ab2c41aab3edb6e046ba4ed8036f072b31458
6 years, 3 months ago (2014-08-26 23:04:02 UTC) #8
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 02:46:58 UTC) #9
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/6434e337b1c0eba3c30d497d7d1a785f4a3d1e5b
Cr-Commit-Position: refs/heads/master@{#292019}

Powered by Google App Engine
This is Rietveld 408576698