Chromium Code Reviews
Help | Chromium Project | Sign in
(29)

Issue 2834543003: [subresource_filter] SB throttle can send multiple speculative requests.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 2 days ago by Charlie (ooo-ish until may 2)
Modified:
2 days, 11 hours ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org, asvitkine+watch_chromium.org, melandory
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[subresource_filter] SB throttle can send multiple speculative requests. This CL adds functionality for sending multiple requests to the safe browsing database. We utilize this functionality to speculatively send requests on WillStartRequest and WillRedirectRequest. In order to evaluate whether speculative checks on every redirect are necessary, we log metrics for what the navigation delay would look like if that check was removed. BUG=671962

Patch Set 1 #

Patch Set 2 : add cancelling navigation throttle #

Total comments: 1

Patch Set 3 : minor style tweaks #

Patch Set 4 : fix constexpr declaration #

Total comments: 5

Patch Set 5 : Remove UAF #

Total comments: 3

Patch Set 6 : remove rly bad dcheck #

Patch Set 7 : engedy review #

Total comments: 4

Patch Set 8 : reviews, etc #

Total comments: 48

Patch Set 9 : engedy review #

Patch Set 10 : actually include the client_request files :P #

Patch Set 11 : Add NoRedirectSpeculation metric #

Unified diffs Side-by-side diffs Delta from patch set Stats (+896 lines, -247 lines) Patch
M chrome/browser/subresource_filter/navigation_throttle_util.cc View 1 2 3 4 5 6 3 chunks +6 lines, -2 lines 0 comments Download
M components/subresource_filter/content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h View 1 2 3 4 5 6 2 chunks +4 lines, -0 lines 0 comments Download
M components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc View 1 2 3 4 5 6 7 8 3 chunks +21 lines, -3 lines 0 comments Download
M components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +26 lines, -7 lines 0 comments Download
M components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +86 lines, -110 lines 0 comments Download
M components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 chunks +243 lines, -18 lines 0 comments Download
A components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +81 lines, -0 lines 0 comments Download
A components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +59 lines, -0 lines 0 comments Download
A components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +89 lines, -0 lines 0 comments Download
A components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +101 lines, -0 lines 0 comments Download
A content/public/test/cancelling_navigation_throttle.h View 1 2 3 4 5 6 7 1 chunk +62 lines, -0 lines 0 comments Download
A content/public/test/cancelling_navigation_throttle.cc View 1 2 3 4 5 6 7 1 chunk +63 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M content/test/navigation_simulator_unittest.cc View 1 2 3 4 5 6 7 7 chunks +21 lines, -107 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 2 chunks +28 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 69 (44 generated)
Charlie (ooo-ish until may 2)
engedy: Would you PTAL at this high level? I was wondering if we want to ...
1 week, 2 days ago (2017-04-19 23:58:24 UTC) #6
clamy
Sorry I did not have time to look at this. Will look at it tomorrow.
1 week, 1 day ago (2017-04-20 17:10:35 UTC) #14
engedy
Please some high level comments below. https://codereview.chromium.org/2834543003/diff/60001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2834543003/diff/60001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc#newcode71 components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:71: if (current_request_id_ != ...
1 week, 1 day ago (2017-04-20 18:56:23 UTC) #17
engedy
Sorry for the typos, should have read my comments before sending them out. https://codereview.chromium.org/2834543003/diff/60001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc File ...
1 week, 1 day ago (2017-04-20 18:58:18 UTC) #18
clamy
Thanks! content/ lgtm with two nits. https://codereview.chromium.org/2834543003/diff/120001/content/public/test/cancelling_navigation_throttle.h File content/public/test/cancelling_navigation_throttle.h (right): https://codereview.chromium.org/2834543003/diff/120001/content/public/test/cancelling_navigation_throttle.h#newcode19 content/public/test/cancelling_navigation_throttle.h:19: // Consider using ...
5 days, 3 hours ago (2017-04-24 12:07:12 UTC) #29
Charlie (ooo-ish until may 2)
+nparker could you take a look at this change? There was an email thread with ...
4 days, 21 hours ago (2017-04-24 18:25:10 UTC) #33
Nathan Parker (ooo till May 1)
I didn't review the code carefully, but the concept makes sense to me. Whether to ...
4 days, 17 hours ago (2017-04-24 22:05:17 UTC) #36
Charlie (ooo-ish until may 2)
On 2017/04/24 22:05:17, Nathan Parker wrote: > I didn't review the code carefully, but the ...
4 days, 1 hour ago (2017-04-25 13:54:58 UTC) #37
Charlie (ooo-ish until may 2)
To clarify, we're trying to reduce the amount of time this navigation throttle defers the ...
4 days, 1 hour ago (2017-04-25 14:00:03 UTC) #38
Charlie (ooo-ish until may 2)
+shivanisha, would you please review this too?
4 days ago (2017-04-25 14:47:30 UTC) #40
engedy
Splendid work. LGTM % comments. https://codereview.chromium.org/2834543003/diff/140001/components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc File components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc#newcode37 components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc:37: DCHECK(checks_.find(client) == checks_.end()); #include ...
3 days, 17 hours ago (2017-04-25 22:03:54 UTC) #41
Charlie (ooo-ish until may 2)
Addressed most of your comments, thanks for the review. https://codereview.chromium.org/2834543003/diff/140001/components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc File components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc#newcode37 components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc:37: ...
3 days ago (2017-04-26 14:30:26 UTC) #44
engedy
Still LGTM, thanks! https://codereview.chromium.org/2834543003/diff/140001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc#newcode218 components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:218: void RunUntilIdle() { On 2017/04/26 14:30:25, ...
3 days ago (2017-04-26 14:56:39 UTC) #47
Charlie (ooo-ish until may 2)
+melandory proposed to me offline to only check first + last because these checks are ...
3 days ago (2017-04-26 15:23:35 UTC) #48
Charlie (ooo-ish until may 2)
https://codereview.chromium.org/2834543003/diff/140001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc#newcode82 components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:82: database_manager_->CancelCheck(this); On 2017/04/26 14:56:39, engedy wrote: > On 2017/04/26 ...
2 days, 23 hours ago (2017-04-26 15:29:17 UTC) #49
engedy
On 2017/04/26 15:23:35, Charlie Harrison wrote: > +melandory proposed to me offline to only check ...
2 days, 23 hours ago (2017-04-26 15:37:23 UTC) #52
engedy
On 2017/04/26 15:29:17, Charlie Harrison wrote: > https://codereview.chromium.org/2834543003/diff/140001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc > File > components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc > (right): > ...
2 days, 23 hours ago (2017-04-26 15:37:39 UTC) #53
Charlie (ooo-ish until may 2)
On 2017/04/26 15:37:23, engedy wrote: > On 2017/04/26 15:23:35, Charlie Harrison wrote: > > +melandory ...
2 days, 23 hours ago (2017-04-26 15:43:35 UTC) #54
engedy
On 2017/04/26 15:43:35, Charlie Harrison wrote: > On 2017/04/26 15:37:23, engedy wrote: > > On ...
2 days, 23 hours ago (2017-04-26 15:44:57 UTC) #55
Nathan Parker (ooo till May 1)
How about adding some UMA metrics to help validate the performance tradeoff? Some addition data: ...
2 days, 22 hours ago (2017-04-26 17:02:17 UTC) #58
Charlie (ooo-ish until may 2)
On 2017/04/26 17:02:17, Nathan Parker wrote: > How about adding some UMA metrics to help ...
2 days, 21 hours ago (2017-04-26 17:44:06 UTC) #59
Charlie (ooo-ish until may 2)
OK I have added a metric that I think makes sense. The histogram logs the ...
2 days, 19 hours ago (2017-04-26 19:28:55 UTC) #62
Charlie (ooo-ish until may 2)
+isherman would you PTAL at histograms.xml?
2 days, 19 hours ago (2017-04-26 19:32:20 UTC) #64
Ilya Sherman
metrics lgtm, thanks
2 days, 16 hours ago (2017-04-26 23:25:00 UTC) #68
Charlie (ooo-ish until may 2)
2 days, 11 hours ago (2017-04-27 03:46:48 UTC) #69
shivanisha: PTAL?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46