Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(471)

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

Created:
8 months ago by Charlie Harrison
Modified:
7 months, 2 weeks ago
CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, melandory, subresource-filter-reviews_chromium.org
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 Review-Url: https://codereview.chromium.org/2834543003 Cr-Commit-Position: refs/heads/master@{#469073} Committed: https://chromium.googlesource.com/chromium/src/+/00eefae39baa22b1ad6a1ae37457a1f026d9f40d

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 #

Total comments: 8

Patch Set 12 : nparker review #

Total comments: 2

Patch Set 13 : shivanisha review #

Patch Set 14 : rebase on #468982 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+900 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 12 13 12 chunks +242 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 11 12 1 chunk +80 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 11 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 11 1 chunk +95 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 11 12 13 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 11 12 13 2 chunks +28 lines, -0 lines 0 comments Download

Messages

Total messages: 95 (59 generated)
Charlie Harrison
engedy: Would you PTAL at this high level? I was wondering if we want to ...
8 months 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.
8 months 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_ != ...
8 months 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 ...
8 months 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 ...
7 months, 4 weeks ago (2017-04-24 12:07:12 UTC) #29
Charlie Harrison
+nparker could you take a look at this change? There was an email thread with ...
7 months, 3 weeks ago (2017-04-24 18:25:10 UTC) #33
Nathan Parker
I didn't review the code carefully, but the concept makes sense to me. Whether to ...
7 months, 3 weeks ago (2017-04-24 22:05:17 UTC) #36
Charlie Harrison
On 2017/04/24 22:05:17, Nathan Parker wrote: > I didn't review the code carefully, but the ...
7 months, 3 weeks ago (2017-04-25 13:54:58 UTC) #37
Charlie Harrison
To clarify, we're trying to reduce the amount of time this navigation throttle defers the ...
7 months, 3 weeks ago (2017-04-25 14:00:03 UTC) #38
Charlie Harrison
+shivanisha, would you please review this too?
7 months, 3 weeks 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 ...
7 months, 3 weeks ago (2017-04-25 22:03:54 UTC) #41
Charlie Harrison
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: ...
7 months, 3 weeks 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, ...
7 months, 3 weeks ago (2017-04-26 14:56:39 UTC) #47
Charlie Harrison
+melandory proposed to me offline to only check first + last because these checks are ...
7 months, 3 weeks ago (2017-04-26 15:23:35 UTC) #48
Charlie Harrison
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 ...
7 months, 3 weeks 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 ...
7 months, 3 weeks 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): > ...
7 months, 3 weeks ago (2017-04-26 15:37:39 UTC) #53
Charlie Harrison
On 2017/04/26 15:37:23, engedy wrote: > On 2017/04/26 15:23:35, Charlie Harrison wrote: > > +melandory ...
7 months, 3 weeks 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 ...
7 months, 3 weeks ago (2017-04-26 15:44:57 UTC) #55
Nathan Parker
How about adding some UMA metrics to help validate the performance tradeoff? Some addition data: ...
7 months, 3 weeks ago (2017-04-26 17:02:17 UTC) #58
Charlie Harrison
On 2017/04/26 17:02:17, Nathan Parker wrote: > How about adding some UMA metrics to help ...
7 months, 3 weeks ago (2017-04-26 17:44:06 UTC) #59
Charlie Harrison
OK I have added a metric that I think makes sense. The histogram logs the ...
7 months, 3 weeks ago (2017-04-26 19:28:55 UTC) #62
Charlie Harrison
+isherman would you PTAL at histograms.xml?
7 months, 3 weeks ago (2017-04-26 19:32:20 UTC) #64
Ilya Sherman
metrics lgtm, thanks
7 months, 3 weeks ago (2017-04-26 23:25:00 UTC) #68
Charlie Harrison
shivanisha: PTAL?
7 months, 3 weeks ago (2017-04-27 03:46:48 UTC) #69
Charlie Harrison
nparker, shivanisha, friendly ping
7 months, 2 weeks ago (2017-05-02 02:56:08 UTC) #70
Nathan Parker
safe browsing related code LGTM with a few comments/nits. https://codereview.chromium.org/2834543003/diff/200001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/200001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h#newcode54 components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:54: ...
7 months, 2 weeks ago (2017-05-03 00:00:43 UTC) #71
Charlie Harrison
Thanks a lot Nathan! Tried to address most of your comments. https://codereview.chromium.org/2834543003/diff/200001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): ...
7 months, 2 weeks ago (2017-05-03 01:20:30 UTC) #74
shivanisha
lgtm % nit https://codereview.chromium.org/2834543003/diff/220001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/220001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h#newcode35 components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:35: // database requests. It will cancel ...
7 months, 2 weeks ago (2017-05-03 14:33:11 UTC) #77
Charlie Harrison
Thanks Shivani, moving to CQ https://codereview.chromium.org/2834543003/diff/220001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/220001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h#newcode35 components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:35: // database requests. It ...
7 months, 2 weeks ago (2017-05-03 15:08:50 UTC) #78
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2834543003/240001
7 months, 2 weeks ago (2017-05-03 15:09:32 UTC) #81
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/426331)
7 months, 2 weeks ago (2017-05-03 15:13:00 UTC) #83
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2834543003/260001
7 months, 2 weeks ago (2017-05-03 15:41:56 UTC) #88
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/375681)
7 months, 2 weeks ago (2017-05-03 18:27:28 UTC) #90
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2834543003/260001
7 months, 2 weeks ago (2017-05-03 18:47:19 UTC) #92
commit-bot: I haz the power
7 months, 2 weeks ago (2017-05-03 19:29:05 UTC) #95
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/00eefae39baa22b1ad6a1ae37457...

Powered by Google App Engine
This is Rietveld 0eb02b776