thefrog (Gerrit)

unread,

9:28 AM (8 hours ago) 9:28 AM

to Daniel Rubery, Daniel White, Anunoy Ghosh, Chromium LUCI CQ, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, nwoked...@chromium.org, thefro...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from Daniel Rubery and Daniel White

thefrog added 1 comment

Patchset-level comments

File-level comment, Patchset 20 (Latest):

thefrog . resolved

Please review the following:
 - @dru...@chromium.org: c*
 - @daniel...@google.com: ios
 
@anu...@chromium.org cc fyi for context for downstream CLs

Open in Gerrit

Related details

Attention is currently required from:

Daniel Rubery
Daniel White

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Daniel White (Gerrit)

unread,

9:34 AM (8 hours ago) 9:34 AM

to thefrog, Daniel Rubery, Anunoy Ghosh, Chromium LUCI CQ, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, nwoked...@chromium.org, thefro...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from Daniel Rubery and thefrog

Daniel White voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Daniel Rubery
thefrog

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Daniel Rubery (Gerrit)

unread,

12:37 PM (5 hours ago) 12:37 PM

to thefrog, Daniel White, Anunoy Ghosh, Chromium LUCI CQ, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, nwoked...@chromium.org, thefro...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from thefrog

Daniel Rubery voted and added 2 comments

Votes added by Daniel Rubery

Code-Review

+1

2 comments

File components/safe_browsing/core/browser/db/v5_search_hashes_cache_unittest.cc

Line 167, Patchset 20: // Don't expect cache results for eeee and ffff, since they are not in the

Daniel Rubery . unresolved

I think this comment is a better fit for its original location, since it explains the block of EXPECT_TRUE/EXPECT_FALSE?

Line 479, Patchset 20: ClearExpiredResultsHelper(cache);

Daniel Rubery . unresolved

I'm not sure this helper function is doing anything useful? It just calls ClearExpiredResults, which is the original code

Open in Gerrit

Related details

Attention is currently required from:

thefrog

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

thefrog (Gerrit)

unread,

2:42 PM (3 hours ago) 2:42 PM

to Daniel Rubery, Daniel White, Anunoy Ghosh, Chromium LUCI CQ, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, nwoked...@chromium.org, thefro...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

thefrog added 2 comments

File components/safe_browsing/core/browser/db/v5_search_hashes_cache_unittest.cc

Line 167, Patchset 20: // Don't expect cache results for eeee and ffff, since they are not in the

Daniel Rubery . resolved

I think this comment is a better fit for its original location, since it explains the block of EXPECT_TRUE/EXPECT_FALSE?

thefrog

Oops, thanks!

Line 479, Patchset 20: ClearExpiredResultsHelper(cache);

Daniel Rubery . resolved

I'm not sure this helper function is doing anything useful? It just calls ClearExpiredResults, which is the original code

thefrog

This one is because `ClearExpiredResults` is private now that the VerdictCacheManager usage is within the cache class, and the test class is friended so moving it to a method within the test class lets the test call that (instead of friending all the individual test cases).

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

4:48 PM (1 hour ago) 4:48 PM

to thefrog, Daniel Rubery, Daniel White, Anunoy Ghosh, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, nwoked...@chromium.org, thefro...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

21 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: components/safe_browsing/core/browser/db/v5_search_hashes_cache_unittest.cc
Insertions: 2, Deletions: 2.

@@ -164,13 +164,13 @@
   EXPECT_TRUE(cache->SearchCache({"eeee", "ffff"}).empty());
   CheckAndResetCacheHitsAndMisses(/*num_hits=*/0, /*num_misses=*/2);
 
-  // Don't expect cache results for eeee and ffff, since they are not in the
-  // cache. Expect cache results for all other prefixes.
   std::set<std::string> hash_prefixes_to_search = {"aaaa", "bbbb", "cccc",
                                                    "dddd", "eeee", "ffff"};
   auto cache_results = cache->SearchCache(hash_prefixes_to_search);
   CheckAndResetCacheHitsAndMisses(/*num_hits=*/4, /*num_misses=*/2);
 
+  // Don't expect cache results for eeee and ffff, since they are not in the
+  // cache. Expect cache results for all other prefixes.
   EXPECT_EQ(cache_results.size(), 4u);
   EXPECT_TRUE(cache_results.contains("aaaa"));
   EXPECT_TRUE(cache_results.contains("bbbb"));
```

Change information

Commit message:

Create V5SearchHashesCache from HashRealTimeCache

The newer v5 API will start to be used by local checks (HPD) instead of
just hash-prefix real-time (HPRT) checks. Since eventually the two
mechanisms hit the same v5 SearchHashes endpoint, they will both share
the same local cache per profile.

This CL creates the V5SearchHashesCache mostly copied directly from the
existing HashRealTimeCache. For now, its factory creates it in all of
the same scenarios that the only consumer (HashRealTimeService) was
creating it.

Specifically, this CL:
1. Removes the HPRT cache from VerdictCacheManager and recreates it as
   its own V5SearchHashesCache class.
2. Creates the chrome and ios factories for V5SearchHashesCache.
3. Makes HashRealTimeService dependent on the new V5SearchHashesCache
   instead of on VerdictCacheManager.
4. Copies over the main caching code from HashRealTimeCache to the new
   V5SearchHashesCache.
5. Copies over the logic to clear expired entries from the cache every
   30 minutes from VerdictCacheManager to V5SearchHashesCache.
6. Copies over the artificial URL caching logic from VerdictCacheManager
   to V5SearchHashesCache.
7. Does some renaming for HPRT-related things to note V5SearchHashes
   instead. Some will be done in downstream CLs instead.
8. Improves iOS tests in SafeBrowsingServiceTest which were previously
   not fully checking whether HPRT was returned the right verdict.

Note that the behavior of the cache remains HPRT-focused for now. Later
CLs will broaden the focus.

Bypass-Check-License: file renamed

Bug: 362791941

Change-Id: Ib6af83ed908dc0eed8bab72b1f178ccafe602d41

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8047822

Reviewed-by: Daniel Rubery <dru...@chromium.org>

Reviewed-by: Daniel White <daniel...@google.com>

Commit-Queue: thefrog <the...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1659817}

Files:

M chrome/browser/safe_browsing/BUILD.gn
M chrome/browser/safe_browsing/hash_realtime_service_factory.cc
M chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
A chrome/browser/safe_browsing/v5_search_hashes_cache_factory.cc
A chrome/browser/safe_browsing/v5_search_hashes_cache_factory.h
A chrome/browser/safe_browsing/v5_search_hashes_cache_factory_unittest.cc
M chrome/test/BUILD.gn
M components/safe_browsing/core/browser/BUILD.gn
M components/safe_browsing/core/browser/db/BUILD.gn
A components/safe_browsing/core/browser/db/v5_search_hashes_cache.cc
A components/safe_browsing/core/browser/db/v5_search_hashes_cache.h
R components/safe_browsing/core/browser/db/v5_search_hashes_cache_unittest.cc
M components/safe_browsing/core/browser/hashprefix_realtime/BUILD.gn
D components/safe_browsing/core/browser/hashprefix_realtime/hash_realtime_cache.cc
D components/safe_browsing/core/browser/hashprefix_realtime/hash_realtime_cache.h
M components/safe_browsing/core/browser/hashprefix_realtime/hash_realtime_service.cc
M components/safe_browsing/core/browser/hashprefix_realtime/hash_realtime_service.h
M components/safe_browsing/core/browser/hashprefix_realtime/hash_realtime_service_unittest.cc
M components/safe_browsing/core/browser/verdict_cache_manager.cc
M components/safe_browsing/core/browser/verdict_cache_manager.h
M components/safe_browsing/core/browser/verdict_cache_manager_unittest.cc
M components/safe_browsing/core/common/safebrowsing_switches.cc
M components/safe_browsing/core/common/safebrowsing_switches.h
M ios/chrome/browser/safe_browsing/model/BUILD.gn
M ios/chrome/browser/safe_browsing/model/hash_realtime_service_factory.mm
M ios/chrome/browser/safe_browsing/model/safe_browsing_egtest.mm
A ios/chrome/browser/safe_browsing/model/v5_search_hashes_cache_factory.h
A ios/chrome/browser/safe_browsing/model/v5_search_hashes_cache_factory.mm
A ios/chrome/browser/safe_browsing/model/v5_search_hashes_cache_factory_unittest.mm
M ios/components/security_interstitials/safe_browsing/BUILD.gn
M ios/components/security_interstitials/safe_browsing/fake_safe_browsing_client.h
M ios/components/security_interstitials/safe_browsing/fake_safe_browsing_client.mm
M ios/components/security_interstitials/safe_browsing/safe_browsing_service_unittest.mm

Change size: XL

Delta: 33 files changed, 800 insertions(+), 509 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Daniel Rubery, +1 by Daniel White

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Create V5SearchHashesCache from HashRealTimeCache [chromium/src : main]

thefrog (Gerrit)

thefrog added 1 comment

Related details

Daniel White (Gerrit)

Daniel White voted Code-Review+1

Related details

Daniel Rubery (Gerrit)

Daniel Rubery voted and added 2 comments

Votes added by Daniel Rubery

2 comments

Related details

thefrog (Gerrit)

thefrog added 2 comments

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information