Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834543003:
[subresource_filter] SB throttle can send multiple speculative requests. (Closed)
|
Created:
3 years, 8 months ago by Charlie Harrison Modified:
3 years, 7 months 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 #Messages
Total messages: 95 (59 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + clamy@chromium.org, engedy@chromium.org
engedy: Would you PTAL at this high level? I was wondering if we want to send a request for every redirect. clamy: Could you look at the //content parts? https://codereview.chromium.org/2834543003/diff/20001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2834543003/diff/20001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:416: // This call blocks the current task until the timeout occurs after a few Comment could be clarified.
Description was changed from ========== [subresource_filter] Spruce up the safe browsing throttle This CL adds functionality for sending multiple SB requests. BUG=TODO ========== to ========== [subresource_filter] SB throttle can send multiple speculative requests. This CL adds functionality for sending multiple SB requests. BUG=TODO ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Sorry I did not have time to look at this. Will look at it tomorrow.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please some high level comments below. https://codereview.chromium.org/2834543003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2834543003/diff/60001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:71: if (current_request_id_ != request_id) Have you considered content based addressing for these request? In less fancy term, just using the URL? The results should be changing under our feet. https://codereview.chromium.org/2834543003/diff/60001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:111: UMA_HISTOGRAM_TIMES("SubresourceFilter.SafeBrowsing.NavigationDelay", delay); While I do see an argument being made for this namespace, have you considered something like SubresourceFilter.PageLoad.SafeBrowsingDelay? It might also be interesting to collect (into another histogram) just how long a SB round trip took. In case we see that the NavigationDelay is higher than we'd like, we could use that other histogram to figure out if it's the SB ping taking too long, or that things just aren't getting parallelized well. https://codereview.chromium.org/2834543003/diff/80001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2834543003/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:298: base::MockTimer* mock_timer() { return mock_timer_; } Without having read this unit test too thoroughly: Instead of mocking out the entire timer, would it be possible to just mock out time by means of ScopedMockTimeMessageLoopTaskRunner and passing in its MockClock? That could be used to test timing constraint quite precisely, too. https://codereview.chromium.org/2834543003/diff/80001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc (right): https://codereview.chromium.org/2834543003/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:42: current_request_ = Have you checked with the Safe Browsing folks if they can handle the QPS increase this might cause? (Or even if they have a good guess of how much of an increase we are talking about here.) Also, I'm wondering how much we save by `cancelling` the request. Have you thought about just letting each of these requests run its course (only killing them if they are still pending when the result for the final URL is back and we are proceeding with the navigation). That would also allow us to reinstate the redirect chain patterns that was deprecated. Probably the SB folks know more here as well.
Sorry for the typos, should have read my comments before sending them out. https://codereview.chromium.org/2834543003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2834543003/diff/60001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:71: if (current_request_id_ != request_id) On 2017/04/20 18:56:23, engedy wrote: > Have you considered content based addressing for these request? In less fancy > term, just using the URL? The results should be changing under our feet. requests, terms https://codereview.chromium.org/2834543003/diff/80001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc (right): https://codereview.chromium.org/2834543003/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:42: current_request_ = On 2017/04/20 18:56:23, engedy wrote: > Have you checked with the Safe Browsing folks if they can handle the QPS > increase this might cause? (Or even if they have a good guess of how much of an > increase we are talking about here.) > > Also, I'm wondering how much we save by `cancelling` the request. Have you > thought about just letting each of these requests run its course (only killing > them if they are still pending when the result for the final URL is back and we > are proceeding with the navigation). That would also allow us to reinstate the > redirect chain patterns that was deprecated. Probably the SB folks know more > here as well. ... redirect chain patterns *histogram* ...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Thanks! content/ lgtm with two nits. https://codereview.chromium.org/2834543003/diff/120001/content/public/test/ca... File content/public/test/cancelling_navigation_throttle.h (right): https://codereview.chromium.org/2834543003/diff/120001/content/public/test/ca... content/public/test/cancelling_navigation_throttle.h:19: // Consider using it in conjunction with NavigationSimulator. nit: Could you add a TODO(clamy): Check if this could be used in unit tests exercising NavigationThrottle code. ? https://codereview.chromium.org/2834543003/diff/120001/content/public/test/ca... content/public/test/cancelling_navigation_throttle.h:22: enum CancelTime { nit: could this be made an enum class?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + nparker@chromium.org
+nparker could you take a look at this change? There was an email thread with awoz@ about this change for context. In general I think we could be fine with reducing this to just check the first and last URL (if they are different), rather than all the redirects. It would be nice to have the full data to evaluate the tradeoffs, but I'm not sure we have it. https://codereview.chromium.org/2834543003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2834543003/diff/60001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:71: if (current_request_id_ != request_id) On 2017/04/20 18:58:17, engedy wrote: > On 2017/04/20 18:56:23, engedy wrote: > > Have you considered content based addressing for these request? In less fancy > > term, just using the URL? The results should be changing under our feet. > > requests, terms The current PS keeps a list of check results, so I think I still need the id (i.e. the index) to maintain the sorting. https://codereview.chromium.org/2834543003/diff/60001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:111: UMA_HISTOGRAM_TIMES("SubresourceFilter.SafeBrowsing.NavigationDelay", delay); On 2017/04/20 18:56:23, engedy wrote: > While I do see an argument being made for this namespace, have you considered > something like SubresourceFilter.PageLoad.SafeBrowsingDelay? > > It might also be interesting to collect (into another histogram) just how long a > SB round trip took. In case we see that the NavigationDelay is higher than we'd > like, we could use that other histogram to figure out if it's the SB ping taking > too long, or that things just aren't getting parallelized well. Done and done. https://codereview.chromium.org/2834543003/diff/120001/content/public/test/ca... File content/public/test/cancelling_navigation_throttle.h (right): https://codereview.chromium.org/2834543003/diff/120001/content/public/test/ca... content/public/test/cancelling_navigation_throttle.h:19: // Consider using it in conjunction with NavigationSimulator. On 2017/04/24 12:07:12, clamy wrote: > nit: Could you add a > TODO(clamy): Check if this could be used in unit tests exercising > NavigationThrottle code. > ? Done. https://codereview.chromium.org/2834543003/diff/120001/content/public/test/ca... content/public/test/cancelling_navigation_throttle.h:22: enum CancelTime { On 2017/04/24 12:07:12, clamy wrote: > nit: could this be made an enum class? It could, but tests using these enums are already extremely verbose without the extra "CancelTime" . I'd prefer to keep as-is unless you feel strongly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I didn't review the code carefully, but the concept makes sense to me. Whether to check all redirects or just the first+last depends on your threat model. For malware/uws/phishing, we definitely want to check all because the last one might be bad but not yet blacklisted, whereas one in the middle might be known-bad. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:94: base::Unretained(database_client_.get()), FYI the database_manager doesn't support multiple simultaneous calls using the same client. It _looks_ like you're creating a new ClientRequest (i.e. the SBDbMgr::Client) for each request, so this should be ok. FYI I'd like to change this callback scheme that the DbMnager uses at some point in the ~next quarter or so, to use a callback and a cancelable request_id, rather than the ::Client with its methods, but what you're doing here makes sense for now (we need to wait for Pver4 to rollout and Pver3 code to be removed).
On 2017/04/24 22:05:17, Nathan Parker wrote: > I didn't review the code carefully, but the concept makes sense to me. Whether > to check all redirects or just the first+last depends on your threat model. For > malware/uws/phishing, we definitely want to check all because the last one might > be bad but not yet blacklisted, whereas one in the middle might be known-bad. Ok, for this feature we only want to activate if the very last URL is known-bad. However, to reduce latency we are speculatively checking for every redirect (in the hope it will be the last). Does that make sense? > > https://codereview.chromium.org/2834543003/diff/140001/components/subresource... > File > components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc > (right): > > https://codereview.chromium.org/2834543003/diff/140001/components/subresource... > components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:94: > base::Unretained(database_client_.get()), > FYI the database_manager doesn't support multiple simultaneous calls using the > same client. It _looks_ like you're creating a new ClientRequest (i.e. the > SBDbMgr::Client) for each request, so this should be ok. > > FYI I'd like to change this callback scheme that the DbMnager uses at some point > in the ~next quarter or so, to use a callback and a cancelable request_id, > rather than the ::Client with its methods, but what you're doing here makes > sense for now (we need to wait for Pver4 to rollout and Pver3 code to be > removed).
To clarify, we're trying to reduce the amount of time this navigation throttle defers the navigation waiting for the SB check.
csharrison@chromium.org changed reviewers: + shivanisha@chromium.org
+shivanisha, would you please review this too?
Splendid work. LGTM % comments. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc:37: DCHECK(checks_.find(client) == checks_.end()); #include "base/logging.h" https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:30: database_manager_.get(), nit: s/.get()// https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:70: const GURL& url, nit: |url| never used https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:75: DCHECK_LE(request_id, check_results_.size()); DCHECK_LT https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:89: void SubresourceFilterSafeBrowsingActivationThrottle::CheckUrl() { nit: Maybe CheckCurrentURL() or something more qualified? https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:91: int id = check_results_.size() - 1; nit: Let's make sure to use size_t consistently. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:94: base::Unretained(database_client_.get()), On 2017/04/24 22:05:17, Nathan Parker wrote: > FYI the database_manager doesn't support multiple simultaneous calls using the > same client. It _looks_ like you're creating a new ClientRequest (i.e. the > SBDbMgr::Client) for each request, so this should be ok. > > FYI I'd like to change this callback scheme that the DbMnager uses at some point > in the ~next quarter or so, to use a callback and a cancelable request_id, > rather than the ::Client with its methods, but what you're doing here makes > sense for now (we need to wait for Pver4 to rollout and Pver3 code to be > removed). Yes, the CL creates new SBDBMgr::Clients, so we should be good. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:7: #include <map> nit: map, set, string all look unused. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:218: void RunUntilIdle() { idea: As an alternative, have you considered using ScopedMockTimeMessageLoopTaskRunner and passing in the same task runner for both IO and UI threads? Would that allow us to somehow set a timeout on the FakeSBDB, and have a test where we start deferring, but do not end up timing out? It looks like this is not currently tested, but seems to be the case that would happen in practice most often. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:219: test_task_runner_->RunUntilIdle(); nit: s//test_io_task_runner/? https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:252: std::tie(cancel_time_, result_sync_) = GetParam(); nit: Not very familiar with parameterized tests. Is it too early to do this in the ctor? https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:285: tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix, Let's verify timing histograms here too! https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:27: scoped_refptr<base::SingleThreadTaskRunner> ui_io_task_runner) nit: s//ui_task_runner|throttle_task_runner/ https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:45: raw_request->Start(); nit: Add comment that |raw_request| might be destroyed after this line. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:82: database_manager_->CancelCheck(this); Could you please double-check that spurious calls to this are OK, plus that it cannot happen that there is already a task posted to invoke the callback (or it is handled). https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:95: FROM_HERE, kCheckURLTimeout, nit: #include "base/location.h" https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:97: base::Unretained(this))); nit: #include "base/bind_helpers.h" https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:103: const safe_browsing::ThreatMetadata& metadata) { nit: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:32: // comes in for URL B while URL A is in flight, we cancel the check to URL A. comment nit: Update this line. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:38: const base::WeakPtr<SubresourceFilterSafeBrowsingActivationThrottle>& nit: I believe the latest style is to not pass smart pointers by const&. How about taking this by value, and moving it into the member on the initializer list? https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:45: void CheckUrlOnIO(const GURL& url, size_t request_id); #include <stddef.h> for size_t (in all places) https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:69: class SubresourceFilterSafeBrowsingClientRequest Can this be moved to the .cc file?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed most of your comments, thanks for the review. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc:37: DCHECK(checks_.find(client) == checks_.end()); On 2017/04/25 22:03:53, engedy wrote: > #include "base/logging.h" Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:30: database_manager_.get(), On 2017/04/25 22:03:53, engedy wrote: > nit: s/.get()// Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:70: const GURL& url, On 2017/04/25 22:03:53, engedy wrote: > nit: |url| never used Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:75: DCHECK_LE(request_id, check_results_.size()); On 2017/04/25 22:03:53, engedy wrote: > DCHECK_LT Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:89: void SubresourceFilterSafeBrowsingActivationThrottle::CheckUrl() { On 2017/04/25 22:03:53, engedy wrote: > nit: Maybe CheckCurrentURL() or something more qualified? Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:91: int id = check_results_.size() - 1; On 2017/04/25 22:03:53, engedy wrote: > nit: Let's make sure to use size_t consistently. Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:7: #include <map> On 2017/04/25 22:03:53, engedy wrote: > nit: map, set, string all look unused. Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:218: void RunUntilIdle() { On 2017/04/25 22:03:54, engedy wrote: > idea: As an alternative, have you considered using > ScopedMockTimeMessageLoopTaskRunner and passing in the same task runner for both > IO and UI threads? Would that allow us to somehow set a timeout on the FakeSBDB, > and have a test where we start deferring, but do not end up timing out? It looks > like this is not currently tested, but seems to be the case that would happen in > practice most often. I don't think this is tenable because the NavigationSimulator uses RunLoop() internally to wait for task completion, which is incompatible with ScopedMockTimeMessageLoopTaskRunner. Since the commit() code path posts a task to flush the IO thread run loop, we still get in a situation where the defer path is taken and exercised without a timeout (though I agree it would be nice to have finer grained control). Let me add a TODO here to consider augmenting these tests + the NavigationSimulator infrastructure to take a MockTimeTaskRunner as input, and giving users control over it. The design will take some thought though, and may block on crbug.com/703346 https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:219: test_task_runner_->RunUntilIdle(); On 2017/04/25 22:03:54, engedy wrote: > nit: s//test_io_task_runner/? Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:252: std::tie(cancel_time_, result_sync_) = GetParam(); On 2017/04/25 22:03:53, engedy wrote: > nit: Not very familiar with parameterized tests. Is it too early to do this in > the ctor? Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:285: tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix, On 2017/04/25 22:03:54, engedy wrote: > Let's verify timing histograms here too! Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:27: scoped_refptr<base::SingleThreadTaskRunner> ui_io_task_runner) On 2017/04/25 22:03:54, engedy wrote: > nit: s//ui_task_runner|throttle_task_runner/ Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:45: raw_request->Start(); On 2017/04/25 22:03:54, engedy wrote: > nit: Add comment that |raw_request| might be destroyed after this line. Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:82: database_manager_->CancelCheck(this); On 2017/04/25 22:03:54, engedy wrote: > Could you please double-check that spurious calls to this are OK, plus that it > cannot happen that there is already a task posted to invoke the callback (or it > is handled). For the former: I don't think spurious calls are always ok (the remote DB DCHECKs this fact). Let me tweak the class. For the latter: I also think this is fine AFAICT because we use weak ptrs to service the posted tasks. So as long as we immediately destroy the request we should be ok. nparker: would you verify? https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:95: FROM_HERE, kCheckURLTimeout, On 2017/04/25 22:03:54, engedy wrote: > nit: #include "base/location.h" Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:97: base::Unretained(this))); On 2017/04/25 22:03:54, engedy wrote: > nit: #include "base/bind_helpers.h" Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:103: const safe_browsing::ThreatMetadata& metadata) { On 2017/04/25 22:03:54, engedy wrote: > nit: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:32: // comes in for URL B while URL A is in flight, we cancel the check to URL A. On 2017/04/25 22:03:54, engedy wrote: > comment nit: Update this line. Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:38: const base::WeakPtr<SubresourceFilterSafeBrowsingActivationThrottle>& On 2017/04/25 22:03:54, engedy wrote: > nit: I believe the latest style is to not pass smart pointers by const&. How > about taking this by value, and moving it into the member on the initializer > list? Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:45: void CheckUrlOnIO(const GURL& url, size_t request_id); On 2017/04/25 22:03:54, engedy wrote: > #include <stddef.h> for size_t (in all places) Done. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:69: class SubresourceFilterSafeBrowsingClientRequest On 2017/04/25 22:03:54, engedy wrote: > Can this be moved to the .cc file? Do you mind if I move it to a separate file (that's what I've done). I've been frustrated recently untangling anonymous classes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Still LGTM, thanks! https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:218: void RunUntilIdle() { On 2017/04/26 14:30:25, Charlie Harrison wrote: > On 2017/04/25 22:03:54, engedy wrote: > > idea: As an alternative, have you considered using > > ScopedMockTimeMessageLoopTaskRunner and passing in the same task runner for > both > > IO and UI threads? Would that allow us to somehow set a timeout on the > FakeSBDB, > > and have a test where we start deferring, but do not end up timing out? It > looks > > like this is not currently tested, but seems to be the case that would happen > in > > practice most often. > > I don't think this is tenable because the NavigationSimulator uses RunLoop() > internally to wait for task completion, which is incompatible with > ScopedMockTimeMessageLoopTaskRunner. > > Since the commit() code path posts a task to flush the IO thread run loop, we > still get in a situation where the defer path is taken and exercised without a > timeout (though I agree it would be nice to have finer grained control). > > Let me add a TODO here to consider augmenting these tests + the > NavigationSimulator infrastructure to take a MockTimeTaskRunner as input, and > giving users control over it. The design will take some thought though, and may > block on crbug.com/703346 SGTM. Thanks for digging up that bug! :) https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc:82: database_manager_->CancelCheck(this); On 2017/04/26 14:30:25, Charlie Harrison wrote: > On 2017/04/25 22:03:54, engedy wrote: > > Could you please double-check that spurious calls to this are OK, plus that it > > cannot happen that there is already a task posted to invoke the callback (or > it > > is handled). > > For the former: I don't think spurious calls are always ok (the remote DB > DCHECKs this fact). Let me tweak the class. > > For the latter: I also think this is fine AFAICT because we use weak ptrs to > service the posted tasks. So as long as we immediately destroy the request we > should be ok. > > nparker: would you verify? To clarify the latter: I was wondering if the following might happen. 1.) SubresourceFilterSafeBrowsingClientRequest::OnCheckBrowseUrlResult is posted by something inside SBDBMgr to the IO thread. 2.) SubresourceFilterSafeBrowsingClient is destroyed, and destroys all ClientRequests along with it. 3.) SBDBMgr::CancelCheck called, but too late. 4.) The task posted in (1) is processed anyway and results in UAF. https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:69: class SubresourceFilterSafeBrowsingClientRequest On 2017/04/26 14:30:25, Charlie Harrison wrote: > On 2017/04/25 22:03:54, engedy wrote: > > Can this be moved to the .cc file? > > Do you mind if I move it to a separate file (that's what I've done). I've been > frustrated recently untangling anonymous classes. Fine by me!
+melandory proposed to me offline to only check first + last because these checks are relatively expensive. The one concern I have is that for the small percent of navigations that redirect (~10% I think), we will be fully in the critical path of the navigation. i.e. We will defer the throttle and issue the last check at the same time. It would be a very simple change to alter behavior here so we could always change our mind after examining metrics too. nparker, engedy, I am interested in your thoughts here too.
https://codereview.chromium.org/2834543003/diff/140001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc (right): https://codereview.chromium.org/2834543003/diff/140001/components/subresource... 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 14:30:25, Charlie Harrison wrote: > > On 2017/04/25 22:03:54, engedy wrote: > > > Could you please double-check that spurious calls to this are OK, plus that > it > > > cannot happen that there is already a task posted to invoke the callback (or > > it > > > is handled). > > > > For the former: I don't think spurious calls are always ok (the remote DB > > DCHECKs this fact). Let me tweak the class. > > > > For the latter: I also think this is fine AFAICT because we use weak ptrs to > > service the posted tasks. So as long as we immediately destroy the request we > > should be ok. > > > > nparker: would you verify? > > To clarify the latter: I was wondering if the following might happen. > > 1.) SubresourceFilterSafeBrowsingClientRequest::OnCheckBrowseUrlResult > is posted by something inside SBDBMgr to the IO thread. > 2.) SubresourceFilterSafeBrowsingClient is destroyed, and destroys all > ClientRequests along with it. > 3.) SBDBMgr::CancelCheck called, but too late. > 4.) The task posted in (1) is processed anyway and results in UAF. > Yes that was my understanding of your question. As far as I could tell in the code, this is handled by using weak pointers that are invalidated by calls to Cancel. So the tasks should never be serviced if Cancel is called (even if it is already posted).
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/26 15:23:35, Charlie Harrison wrote: > +melandory proposed to me offline to only check first + last because these > checks are relatively expensive. > > The one concern I have is that for the small percent of navigations that > redirect (~10% I think), we will be fully in the critical path of the > navigation. i.e. We will defer the throttle and issue the last check at the same > time. > > It would be a very simple change to alter behavior here so we could always > change our mind after examining metrics too. > > nparker, engedy, I am interested in your thoughts here too. I agree that we should collect metrics and keep a close eye on them. I think I would start with the "check at every URL" approach that is currently implemented by this CL for the following reasons: 1.) Navigations with no redirects: this approach and "check first + last" approach are equivalent. 2.) One redirect (total of 2 URLs): the two approaches initiate the same number of requests, but "checking at every URL" delays the navigation less thanks to the speculative request at WillRedirectRequest time. I'll defer to Nathan on how much benefit there is to cancelling a request (as opposed to ignoring it and letting it run its course), but my impression is that probably not too much? 3.) Two or more redirects: the approach in this CL initiates more requests, but once again, delays the navigation less. So the current approach is at least not worse in cases (1) and (2), and the trade-off seems reasonable to me in case (3). Also, I expect 2 or more redirects would be relatively rare for main frame navigations? (But I don't have numbers.)
On 2017/04/26 15:29:17, Charlie Harrison wrote: > https://codereview.chromium.org/2834543003/diff/140001/components/subresource... > File > components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc > (right): > > https://codereview.chromium.org/2834543003/diff/140001/components/subresource... > 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 14:30:25, Charlie Harrison wrote: > > > On 2017/04/25 22:03:54, engedy wrote: > > > > Could you please double-check that spurious calls to this are OK, plus > that > > it > > > > cannot happen that there is already a task posted to invoke the callback > (or > > > it > > > > is handled). > > > > > > For the former: I don't think spurious calls are always ok (the remote DB > > > DCHECKs this fact). Let me tweak the class. > > > > > > For the latter: I also think this is fine AFAICT because we use weak ptrs to > > > service the posted tasks. So as long as we immediately destroy the request > we > > > should be ok. > > > > > > nparker: would you verify? > > > > To clarify the latter: I was wondering if the following might happen. > > > > 1.) SubresourceFilterSafeBrowsingClientRequest::OnCheckBrowseUrlResult > > is posted by something inside SBDBMgr to the IO thread. > > 2.) SubresourceFilterSafeBrowsingClient is destroyed, and destroys all > > ClientRequests along with it. > > 3.) SBDBMgr::CancelCheck called, but too late. > > 4.) The task posted in (1) is processed anyway and results in UAF. > > > > Yes that was my understanding of your question. As far as I could tell in the > code, this is handled by using weak pointers that are invalidated by calls to > Cancel. So the tasks should never be serviced if Cancel is called (even if it is > already posted). Got it. Thanks for checking!
On 2017/04/26 15:37:23, engedy wrote: > On 2017/04/26 15:23:35, Charlie Harrison wrote: > > +melandory proposed to me offline to only check first + last because these > > checks are relatively expensive. > > > > The one concern I have is that for the small percent of navigations that > > redirect (~10% I think), we will be fully in the critical path of the > > navigation. i.e. We will defer the throttle and issue the last check at the > same > > time. > > > > It would be a very simple change to alter behavior here so we could always > > change our mind after examining metrics too. > > > > nparker, engedy, I am interested in your thoughts here too. > > I agree that we should collect metrics and keep a close eye on them. I think I > would start with the "check at every URL" approach that is currently implemented > by this CL for the following reasons: > > 1.) Navigations with no redirects: this approach and "check first + last" > approach are equivalent. > 2.) One redirect (total of 2 URLs): the two approaches initiate the same > number of requests, but "checking at every URL" delays the navigation less > thanks to the speculative request at WillRedirectRequest time. I'll defer to > Nathan on how much benefit there is to cancelling a request (as opposed to > ignoring it and letting it run its course), but my impression is that probably > not too much? > 3.) Two or more redirects: the approach in this CL initiates more requests, > but once again, delays the navigation less. > > So the current approach is at least not worse in cases (1) and (2), and the > trade-off seems reasonable to me in case (3). Also, I expect 2 or more redirects > would be relatively rare for main frame navigations? (But I don't have numbers.) This seems pretty reasonable to me. FWIW I recently landed a top level histogram PageLoad.Navigation.RedirectChainLength which indicates (canary data) that >= 2 redirects happen with less than 2% frequency per navigation.
On 2017/04/26 15:43:35, Charlie Harrison wrote: > On 2017/04/26 15:37:23, engedy wrote: > > On 2017/04/26 15:23:35, Charlie Harrison wrote: > > > +melandory proposed to me offline to only check first + last because these > > > checks are relatively expensive. > > > > > > The one concern I have is that for the small percent of navigations that > > > redirect (~10% I think), we will be fully in the critical path of the > > > navigation. i.e. We will defer the throttle and issue the last check at the > > same > > > time. > > > > > > It would be a very simple change to alter behavior here so we could always > > > change our mind after examining metrics too. > > > > > > nparker, engedy, I am interested in your thoughts here too. > > > > I agree that we should collect metrics and keep a close eye on them. I think I > > would start with the "check at every URL" approach that is currently > implemented > > by this CL for the following reasons: > > > > 1.) Navigations with no redirects: this approach and "check first + last" > > approach are equivalent. > > 2.) One redirect (total of 2 URLs): the two approaches initiate the same > > number of requests, but "checking at every URL" delays the navigation less > > thanks to the speculative request at WillRedirectRequest time. I'll defer to > > Nathan on how much benefit there is to cancelling a request (as opposed to > > ignoring it and letting it run its course), but my impression is that probably > > not too much? > > 3.) Two or more redirects: the approach in this CL initiates more requests, > > but once again, delays the navigation less. > > > > So the current approach is at least not worse in cases (1) and (2), and the > > trade-off seems reasonable to me in case (3). Also, I expect 2 or more > redirects > > would be relatively rare for main frame navigations? (But I don't have > numbers.) > > This seems pretty reasonable to me. > > FWIW I recently landed a top level histogram > PageLoad.Navigation.RedirectChainLength which indicates (canary data) that >= 2 > redirects happen with less than 2% frequency per navigation. That certainly speaks for the current solution, then.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
How about adding some UMA metrics to help validate the performance tradeoff? Some addition data: * For Malware/Phishing lists (several million entries in total) bout 0.1% of URL checks will have a local-database hit and a local-cache miss, such that they have to make a full-hash-check network call of a 100 ms (typically, on desktop) * Every URL check has to spend CPU on computing hashes and doing a binary search through the local DB. * Canceling a request doesn't save any real work, since if there's a full-hash-check in progress it won't be canceled -- but the result will be dropped. So I think it's a trade off of 1) more CPU for every multi-redirect fetch vs 2) The potential to get a head-start on the few-100ms network call on probaby < 0.1% of requests
On 2017/04/26 17:02:17, Nathan Parker wrote: > How about adding some UMA metrics to help validate the performance tradeoff? > > Some addition data: > * For Malware/Phishing lists (several million entries in total) bout 0.1% of > URL checks will have a local-database hit and a local-cache miss, such that they > have to make a full-hash-check network call of a 100 ms (typically, on desktop) > * Every URL check has to spend CPU on computing hashes and doing a binary > search through the local DB. > * Canceling a request doesn't save any real work, since if there's a > full-hash-check in progress it won't be canceled -- but the result will be > dropped. > > So I think it's a trade off of > 1) more CPU for every multi-redirect fetch > vs > 2) The potential to get a head-start on the few-100ms network call on probaby > < 0.1% of requests Hey Nathan, thanks a lot for those numbers. Regarding new metrics, it seems like what we really want is a count of requests which 1. Are the last check in the redirect chain 2. Go over the network This way we can evaluate what % of checks after WillStart are useful. Is it true that if the DB does not respond synchronously, it is always a network request?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK I have added a metric that I think makes sense. The histogram logs the time we *would* have delayed the navigation if there was no redirect speculation. This histogram should be comparable to the real delay histogram.
csharrison@chromium.org changed reviewers: + isherman@chromium.org
+isherman would you PTAL at histograms.xml?
Description was changed from ========== [subresource_filter] SB throttle can send multiple speculative requests. This CL adds functionality for sending multiple SB requests. BUG=TODO ========== to ========== [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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
metrics lgtm, thanks
shivanisha: PTAL?
nparker, shivanisha, friendly ping
safe browsing related code LGTM with a few comments/nits. https://codereview.chromium.org/2834543003/diff/200001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:54: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner); The args here are different than in the .cc file. Is there a more accurate name for these task runners, and could you do with just using the global io/ui task runners? https://codereview.chromium.org/2834543003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:66: std::map<SubresourceFilterSafeBrowsingClientRequest*, nit: flat_map would likely have better RAM/CPU performance. Your choice. https://codereview.chromium.org/2834543003/diff/200001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.h (right): https://codereview.chromium.org/2834543003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.h:47: void OnCheckBrowseUrlResult( nit: Add comment that this implements the SBDB::Client https://codereview.chromium.org/2834543003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.h:72: const size_t request_id_; Maybe add a comment as to what request_id is? In what scope is it unique?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot Nathan! Tried to address most of your comments. https://codereview.chromium.org/2834543003/diff/200001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:54: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner); On 2017/05/03 00:00:42, Nathan Parker wrote: > The args here are different than in the .cc file. Is there a more accurate name > for these task runners, and could you do with just using the global io/ui task > runners? Renamed the task runner to throttle_task_runner. AFAIK grabbing the current task runner (as we do in the throttle) is the preferred way of doing things now to make code more sequence friendly. Not sure of a better name for io_task_runner, but we inject it in tests to use the mock time task runner (for artificially advancing time). https://codereview.chromium.org/2834543003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:66: std::map<SubresourceFilterSafeBrowsingClientRequest*, On 2017/05/03 00:00:42, Nathan Parker wrote: > nit: flat_map would likely have better RAM/CPU performance. Your choice. Good call, done. https://codereview.chromium.org/2834543003/diff/200001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.h (right): https://codereview.chromium.org/2834543003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.h:47: void OnCheckBrowseUrlResult( On 2017/05/03 00:00:42, Nathan Parker wrote: > nit: Add comment that this implements the SBDB::Client Done. https://codereview.chromium.org/2834543003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.h:72: const size_t request_id_; On 2017/05/03 00:00:42, Nathan Parker wrote: > Maybe add a comment as to what request_id is? In what scope is it unique? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nit https://codereview.chromium.org/2834543003/diff/220001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/220001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:35: // database requests. It will cancel cancel any outgoing requests when it is nit: 'cancel' mentioned twice
Thanks Shivani, moving to CQ https://codereview.chromium.org/2834543003/diff/220001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h (right): https://codereview.chromium.org/2834543003/diff/220001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h:35: // database requests. It will cancel cancel any outgoing requests when it is On 2017/05/03 14:33:11, shivanisha wrote: > nit: 'cancel' mentioned twice Done.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, engedy@chromium.org, nparker@chromium.org, isherman@chromium.org, shivanisha@chromium.org Link to the patchset: https://codereview.chromium.org/2834543003/#ps240001 (title: "shivanisha review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, clamy@chromium.org, shivanisha@chromium.org, isherman@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2834543003/#ps260001 (title: "rebase on #468982")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1493837168097920,
"parent_rev": "2d691f86ea2a0e9057f82a4ae28f7c00fff26f6e", "commit_rev":
"00eefae39baa22b1ad6a1ae37457a1f026d9f40d"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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/+/00eefae39baa22b1ad6a1ae37457... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/00eefae39baa22b1ad6a1ae37457... |
