thefrog (Gerrit)

unread,

Jul 15, 2026, 10:16:58 AM (6 days ago) Jul 15

to Varun Khaneja, Chromium LUCI CQ, chromium...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from Varun Khaneja

thefrog voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Varun Khaneja

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

Varun Khaneja (Gerrit)

unread,

Jul 17, 2026, 3:53:46 PM (4 days ago) Jul 17

to thefrog, Chromium LUCI CQ, chromium...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from thefrog

Varun Khaneja voted and added 2 comments

Votes added by Varun Khaneja

Code-Review

+1

2 comments

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

Line 30, Patchset 9: 30 * 60 * 1000, // initial_delay_ms (30 minutes)

Varun Khaneja . unresolved

Why do we have an initial delay of 30 minutes?

Line 32, Patchset 9: 0.5, // jitter_factor

Varun Khaneja . unresolved

Can you please add a comment about why is this 0.5?

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,

Jul 20, 2026, 12:02:55 PM (yesterday) Jul 20

to Chromium LUCI CQ, chromium...@chromium.org, nwoked...@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_get_hash_protocol_manager.cc

Line 30, Patchset 9: 30 * 60 * 1000, // initial_delay_ms (30 minutes)

Varun Khaneja . resolved

Why do we have an initial delay of 30 minutes?

thefrog

This is to match v4's implementation of backoff (which started at 15 - 30 mins). The net::BackoffEntry policy jitter factor is "backwards" sort of -- the jitter_factor is subtracted from the initial delay.

Line 32, Patchset 9: 0.5, // jitter_factor

Varun Khaneja . resolved

Can you please add a comment about why is this 0.5?

thefrog

Done. Also, thanks for these comments! They made me look at the tests again and realize the backoff boundaries weren't strict enough + that another backoff test could be more robust.

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,

Jul 20, 2026, 1:05:44 PM (yesterday) Jul 20

to thefrog, Varun Khaneja, chromium...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

10 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_get_hash_protocol_manager.cc
Insertions: 14, Deletions: 2.

@@ -25,11 +25,23 @@
 
 namespace {
 
+// Backoff policy for V5 GetHash requests.
+// With initial_delay_ms = 30 minutes, multiply_factor = 2.0, and
+// jitter_factor = 0.5, the backoff delay increases exponentially with
+// subsequent failures and has random jitter in [0.5 * base, base]:
+//  - 1st failure: 15 min - 30 min
+//  - 2nd failure: 30 min to 1 hour
+//  - 3rd failure: 1 hour to 2 hours
+//  - 4th failure: 2 hours to 4 hours
+//  - Subsequent failures continue doubling up to the 24 hour maximum backoff
+//    cap.
 const net::BackoffEntry::Policy kV5GetHashBackoffPolicy = {
     0,                    // num_errors_to_ignore


     30 * 60 * 1000,       // initial_delay_ms (30 minutes)

-    2.0,                  // multiply_factor
-    0.5,                  // jitter_factor
+    2.0,                  // multiply_factor (exponentially increases delays
+                          // by 2x for each new failure)
+    0.5,                  // jitter_factor (randomly reduces the delay by up
+                          // to 50%)
     24 * 60 * 60 * 1000,  // maximum_backoff_ms (24 hours)
     -1,                   // entry_lifetime_ms
     false,                // always_use_initial_delay
```
```
The name of the file: components/safe_browsing/core/browser/db/v5_get_hash_protocol_manager_unittest.cc
Insertions: 84, Deletions: 38.

@@ -380,7 +380,7 @@
   std::string expected_url =
       GetExpectedRequestUrl(SBProtocolManagerUtil::GetHashPrefix(full_hash));
 
-  // 1. Trigger first failure to enter backoff (delay [15, 45] mins).
+  // 1. Trigger first failure to enter backoff (delay [15, 30] mins).
   test_url_loader_factory_.AddResponse(
       GURL(expected_url), network::mojom::URLResponseHead::New(), "",
       network::URLLoaderCompletionStatus(net::ERR_CONNECTION_RESET));
@@ -412,10 +412,10 @@
     EXPECT_EQ(test_url_loader_factory_.NumPending(), 0);
   }
 
-  // 4. Fast forward by another 32 minutes (total 46 minutes, which is > max
-  // delay 45 mins). Request should be allowed. We trigger another failure.
-  // Delay should increase to [30, 90] mins.
-  task_environment_.FastForwardBy(base::Minutes(32));
+  // 4. Fast forward by another 17 minutes (total 31 minutes, which is > max
+  // delay 30 mins). Request should be allowed. We trigger another failure.
+  // Delay should increase to [30, 60] mins.
+  task_environment_.FastForwardBy(base::Minutes(17));
   test_url_loader_factory_.AddResponse(
       GURL(expected_url), network::mojom::URLResponseHead::New(), "",
       network::URLLoaderCompletionStatus(net::ERR_CONNECTION_RESET));
@@ -437,10 +437,10 @@
     EXPECT_EQ(test_url_loader_factory_.NumPending(), 0);
   }
 
-  // 6. Fast forward by another 62 minutes (total 91 minutes since second
-  // failure, > max delay 90 mins). Request should be allowed. We let it succeed
+  // 6. Fast forward by another 32 minutes (total 61 minutes since second
+  // failure, > max delay 60 mins). Request should be allowed. We let it succeed
   // this time.
-  task_environment_.FastForwardBy(base::Minutes(62));
+  task_environment_.FastForwardBy(base::Minutes(32));
   std::vector<V5::FullHash> full_hashes = {
       CreateFullHashProto(full_hash, {V5::ThreatType::SOCIAL_ENGINEERING},
                           /*threat_attributes=*/std::nullopt)};
@@ -477,73 +477,116 @@
 TEST_F(V5GetHashProtocolManagerTest, GetFullHashes_Backoff_RetriableErrors) {
   std::unique_ptr<V5GetHashProtocolManager> pm = CreateProtocolManager();
 
-  FullHashStr full_hash("12345678901234567890123456789012");
-  std::string prefix = SBProtocolManagerUtil::GetHashPrefix(full_hash);
-
-  std::map<FullHashStr, std::vector<SBThreatType>> full_hash_to_threat_types;
-  full_hash_to_threat_types[full_hash] = {
+  FullHashStr full_hash1("12345678901234567890123456789012");
+  std::map<FullHashStr, std::vector<SBThreatType>> full_hash_to_threat_types1;
+  full_hash_to_threat_types1[full_hash1] = {
       SBThreatType::SB_THREAT_TYPE_URL_PHISHING};
-
-  std::string expected_url = GetExpectedRequestUrl(prefix);
+  std::string expected_url1 =
+      GetExpectedRequestUrl(SBProtocolManagerUtil::GetHashPrefix(full_hash1));
 
   // 1. Trigger a retriable failure (ERR_NETWORK_CHANGED).
   test_url_loader_factory_.AddResponse(
-      GURL(expected_url), network::mojom::URLResponseHead::New(), "",
+      GURL(expected_url1), network::mojom::URLResponseHead::New(), "",
       network::URLLoaderCompletionStatus(net::ERR_NETWORK_CHANGED));
   {
     base::test::TestFuture<SBThreatType, const ThreatMetadata&> future;
-    pm->GetFullHashes(full_hash_to_threat_types, future.GetCallback());
+    pm->GetFullHashes(full_hash_to_threat_types1, future.GetCallback());
     EXPECT_EQ(future.Get<0>(), SBThreatType::SB_THREAT_TYPE_SAFE);
     EXPECT_EQ(future.Get<1>(), ThreatMetadata());
   }
 
-  // 2. Verify subsequent request is not blocked, and let it fail with a real
-  // error.
+  // 2. Verify subsequent request is not blocked + succeeds with URL_PHISHING.
+  FullHashStr full_hash2("23456789012345678901234567890123");
+  std::map<FullHashStr, std::vector<SBThreatType>> full_hash_to_threat_types2;
+  full_hash_to_threat_types2[full_hash2] = {
+      SBThreatType::SB_THREAT_TYPE_URL_PHISHING};
+  std::string expected_url2 =
+      GetExpectedRequestUrl(SBProtocolManagerUtil::GetHashPrefix(full_hash2));
+  std::vector<V5::FullHash> full_hashes2 = {
+      CreateFullHashProto(full_hash2, {V5::ThreatType::SOCIAL_ENGINEERING},
+                          /*threat_attributes=*/std::nullopt)};
+
+  test_url_loader_factory_.ClearResponses();
+  SetUpDefaultLookupResponse(expected_url2, full_hashes2);
+  {
+    base::test::TestFuture<SBThreatType, const ThreatMetadata&> future;
+    pm->GetFullHashes(full_hash_to_threat_types2, future.GetCallback());
+    EXPECT_EQ(future.Get<0>(), SBThreatType::SB_THREAT_TYPE_URL_PHISHING);
+    EXPECT_EQ(future.Get<1>(), ThreatMetadata());
+  }
+
+  // 3. Trigger a real (non-retriable) error to enter backoff.
+  FullHashStr full_hash3("34567890123456789012345678901234");
+  std::map<FullHashStr, std::vector<SBThreatType>> full_hash_to_threat_types3;
+  full_hash_to_threat_types3[full_hash3] = {
+      SBThreatType::SB_THREAT_TYPE_URL_PHISHING};
+  std::string expected_url3 =
+      GetExpectedRequestUrl(SBProtocolManagerUtil::GetHashPrefix(full_hash3));
+
   test_url_loader_factory_.ClearResponses();
   test_url_loader_factory_.AddResponse(
-      GURL(expected_url), network::mojom::URLResponseHead::New(), "",
+      GURL(expected_url3), network::mojom::URLResponseHead::New(), "",
       network::URLLoaderCompletionStatus(net::ERR_CONNECTION_RESET));
   {
     base::test::TestFuture<SBThreatType, const ThreatMetadata&> future;
-    pm->GetFullHashes(full_hash_to_threat_types, future.GetCallback());
+    pm->GetFullHashes(full_hash_to_threat_types3, future.GetCallback());
     EXPECT_EQ(future.Get<0>(), SBThreatType::SB_THREAT_TYPE_SAFE);
     EXPECT_EQ(future.Get<1>(), ThreatMetadata());
   }
 
-  // 3. Verify we are now in backoff (1st failure delay).
+  // 4. Verify we are now in backoff (returns safe, not phishing).
+  FullHashStr full_hash4("45678901234567890123456789012345");
+  std::map<FullHashStr, std::vector<SBThreatType>> full_hash_to_threat_types4;
+  full_hash_to_threat_types4[full_hash4] = {
+      SBThreatType::SB_THREAT_TYPE_URL_PHISHING};
   test_url_loader_factory_.ClearResponses();
   {
     base::test::TestFuture<SBThreatType, const ThreatMetadata&> future;
-    pm->GetFullHashes(full_hash_to_threat_types, future.GetCallback());
+    pm->GetFullHashes(full_hash_to_threat_types4, future.GetCallback());
     EXPECT_EQ(future.Get<0>(), SBThreatType::SB_THREAT_TYPE_SAFE);
     EXPECT_EQ(future.Get<1>(), ThreatMetadata());
     EXPECT_EQ(test_url_loader_factory_.NumPending(), 0);
   }
 
-  // 4. Fast forward 46 minutes to exit backoff.
-  task_environment_.FastForwardBy(base::Minutes(46));
+  // 5. Fast forward 31 minutes to exit backoff (exceeds max delay 30 mins).
+  task_environment_.FastForwardBy(base::Minutes(31));
 
-  // 5. Trigger a retriable error.
+  // 6. Trigger another retriable error after exiting backoff.
+  FullHashStr full_hash5("56789012345678901234567890123456");
+  std::map<FullHashStr, std::vector<SBThreatType>> full_hash_to_threat_types5;
+  full_hash_to_threat_types5[full_hash5] = {
+      SBThreatType::SB_THREAT_TYPE_URL_PHISHING};
+  std::string expected_url5 =
+      GetExpectedRequestUrl(SBProtocolManagerUtil::GetHashPrefix(full_hash5));
+
   test_url_loader_factory_.ClearResponses();
   test_url_loader_factory_.AddResponse(
-      GURL(expected_url), network::mojom::URLResponseHead::New(), "",
+      GURL(expected_url5), network::mojom::URLResponseHead::New(), "",
       network::URLLoaderCompletionStatus(net::ERR_NETWORK_CHANGED));
   {
     base::test::TestFuture<SBThreatType, const ThreatMetadata&> future;
-    pm->GetFullHashes(full_hash_to_threat_types, future.GetCallback());
+    pm->GetFullHashes(full_hash_to_threat_types5, future.GetCallback());
     EXPECT_EQ(future.Get<0>(), SBThreatType::SB_THREAT_TYPE_SAFE);
     EXPECT_EQ(future.Get<1>(), ThreatMetadata());
   }
 
-  // 6. Verify subsequent request is not blocked.
+  // 7. Verify subsequent request is not blocked + succeeds with URL_PHISHING.
+  FullHashStr full_hash6("67890123456789012345678901234567");
+  std::map<FullHashStr, std::vector<SBThreatType>> full_hash_to_threat_types6;
+  full_hash_to_threat_types6[full_hash6] = {
+      SBThreatType::SB_THREAT_TYPE_URL_PHISHING};
+  std::string expected_url6 =
+      GetExpectedRequestUrl(SBProtocolManagerUtil::GetHashPrefix(full_hash6));
+  std::vector<V5::FullHash> full_hashes6 = {
+      CreateFullHashProto(full_hash6, {V5::ThreatType::SOCIAL_ENGINEERING},
+                          /*threat_attributes=*/std::nullopt)};
+
   test_url_loader_factory_.ClearResponses();
-  test_url_loader_factory_.AddResponse(
-      GURL(expected_url), network::mojom::URLResponseHead::New(), "",
-      network::URLLoaderCompletionStatus(net::ERR_CONNECTION_RESET));
+  SetUpDefaultLookupResponse(expected_url6, full_hashes6);
   {
     base::test::TestFuture<SBThreatType, const ThreatMetadata&> future;
-    pm->GetFullHashes(full_hash_to_threat_types, future.GetCallback());
-    EXPECT_EQ(future.Get<0>(), SBThreatType::SB_THREAT_TYPE_SAFE);
+    pm->GetFullHashes(full_hash_to_threat_types6, future.GetCallback());
+    EXPECT_EQ(future.Get<0>(), SBThreatType::SB_THREAT_TYPE_URL_PHISHING);
     EXPECT_EQ(future.Get<1>(), ThreatMetadata());
   }
 }
@@ -776,10 +819,12 @@
   std::string expected_url =
       GetExpectedRequestUrl(SBProtocolManagerUtil::GetHashPrefix(full_hash));
 
-  // Trigger 10 failures. We need to fast forward past the backoff delay each
-  // time to be allowed to send the next request.
-  std::array<int, 10> wait_times_mins = {46,   91,   181,  361,  721,
-                                         1441, 1441, 1441, 1441, 1441};
+  // Trigger 10 failures. We need to fast forward past the max backoff delay
+  // each time to be allowed to send the next request. Max delays for
+  // failures 1..10 are: 30m, 60m, 120m, 240m, 480m, 960m, and capped at 1440m
+  // (24 hours).
+  std::array<int, 10> wait_times_mins = {31,  61,   121,  241,  481,
+                                         961, 1441, 1441, 1441, 1441};
 
   for (int i = 0; i < 10; ++i) {
     test_url_loader_factory_.ClearResponses();
@@ -791,6 +836,7 @@
       pm->GetFullHashes(full_hash_to_threat_types, future.GetCallback());
       EXPECT_EQ(future.Get<0>(), SBThreatType::SB_THREAT_TYPE_SAFE);
       EXPECT_EQ(future.Get<1>(), ThreatMetadata());
+      EXPECT_EQ(test_url_loader_factory_.total_requests(), i + 1u);
     }
     // Fast forward to exit backoff for the next request.
     task_environment_.FastForwardBy(base::Minutes(wait_times_mins[i]));
```

Change information

Commit message:

Implement remaining V5GetHashProtocolManager logic

This consists of:
 - Caching, reusing the V5SearchHashes cache that hash-prefix real-
   time lookups also use.
 - Backoff, using the same parameters as V4GetHashProtocolManager, but
   using net::BackoffEntry instead of implementing it directly.
 - Metadata coverage in V5SearchHashesUtil and
   IsHashDetailRelevantForLocalChecks for local lists use cases.

NO_IFTTT=Adding new IFTTT lint blocks for V5 Safe Browsing files; no
proto enum definitions were modified.

Bug: 362791941

Change-Id: Ie48ef36f4ef9a314f59f6940a65fd2fd52a73fb1

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

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

Reviewed-by: Varun Khaneja <va...@chromium.org>

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

Files:

M components/safe_browsing/core/browser/db/BUILD.gn
M components/safe_browsing/core/browser/db/v5_get_hash_protocol_manager.cc
M components/safe_browsing/core/browser/db/v5_get_hash_protocol_manager.h
M components/safe_browsing/core/browser/db/v5_get_hash_protocol_manager_unittest.cc
M components/safe_browsing/core/browser/db/v5_search_hashes_util.cc
M components/safe_browsing/core/browser/db/v5_search_hashes_util.h
M components/safe_browsing/core/browser/db/v5_search_hashes_util_unittest.cc
M components/safe_browsing/core/common/proto/safebrowsingv5.proto

Change size: L

Delta: 8 files changed, 840 insertions(+), 23 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Varun Khaneja

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

Implement remaining V5GetHashProtocolManager logic [chromium/src : main]

thefrog (Gerrit)

thefrog voted Commit-Queue+1

Related details

Varun Khaneja (Gerrit)

Varun Khaneja voted and added 2 comments

Votes added by Varun Khaneja

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