Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(551)

Unified Diff: components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc

Issue 2834543003: [subresource_filter] SB throttle can send multiple speculative requests. (Closed)
Patch Set: Add NoRedirectSpeculation metric Created 3 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
diff --git a/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc b/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
index 4d728239ced8a6a02b0dd5b64e52c2020a1896e8..6b6c4921c73957ce3f701d5a38e56f826e154bbd 100644
--- a/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
+++ b/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
@@ -1,124 +1,63 @@
-// Copyright (c) 2017 The Chromium Authors. All rights reserved.
+// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
+#include <utility>
#include <vector>
+#include "base/metrics/histogram_macros.h"
#include "base/timer/timer.h"
-#include "components/safe_browsing_db/v4_local_database_manager.h"
#include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
+#include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/web_contents.h"
-namespace {
-
-// Maximum time in milliseconds to wait for the Safe Browsing service to
-// verify a URL. After this amount of time the outstanding check will be
-// aborted, and the URL will be treated as if it didn't belong to the
-// Subresource Filter only list.
-constexpr base::TimeDelta kCheckURLTimeout = base::TimeDelta::FromSeconds(5);
-
-} // namespace
-
namespace subresource_filter {
-class SubresourceFilterSafeBrowsingActivationThrottle::SBDatabaseClient
- : public safe_browsing::SafeBrowsingDatabaseManager::Client {
- public:
- SBDatabaseClient(
- scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
- database_manager,
- base::WeakPtr<SubresourceFilterSafeBrowsingActivationThrottle> throttle,
- scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
- : database_manager_(std::move(database_manager)),
- throttle_(throttle),
- io_task_runner_(io_task_runner) {}
-
- ~SBDatabaseClient() override {
- DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
- database_manager_->CancelCheck(this);
- }
-
- void CheckUrlOnIO(const GURL& url) {
- DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
- DCHECK(!url.is_empty());
- url_being_checked_ = url;
- if (database_manager_->CheckUrlForSubresourceFilter(url, this)) {
- OnCheckBrowseUrlResult(url, safe_browsing::SB_THREAT_TYPE_SAFE,
- safe_browsing::ThreatMetadata());
- return;
- }
- timer_.Start(FROM_HERE, kCheckURLTimeout, this,
- &SubresourceFilterSafeBrowsingActivationThrottle::
- SBDatabaseClient::OnCheckUrlTimeout);
- }
-
- void OnCheckBrowseUrlResult(
- const GURL& url,
- safe_browsing::SBThreatType threat_type,
- const safe_browsing::ThreatMetadata& metadata) override {
- DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
- DCHECK_EQ(url_being_checked_, url);
- timer_.Stop(); // Cancel the timeout timer.
- io_task_runner_->PostTask(
- FROM_HERE,
- base::Bind(&SubresourceFilterSafeBrowsingActivationThrottle::
- OnCheckUrlResultOnUI,
- throttle_, url, threat_type, metadata.threat_pattern_type));
- }
-
- // Callback for when the safe browsing check has taken longer than
- // kCheckURLTimeout.
- void OnCheckUrlTimeout() {
- DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
- database_manager_->CancelCheck(this);
-
- OnCheckBrowseUrlResult(url_being_checked_,
- safe_browsing::SB_THREAT_TYPE_SAFE,
- safe_browsing::ThreatMetadata());
- }
-
- private:
- scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> database_manager_;
-
- // Timer to abort the safe browsing check if it takes too long.
- base::OneShotTimer timer_;
- GURL url_being_checked_;
-
- base::WeakPtr<SubresourceFilterSafeBrowsingActivationThrottle> throttle_;
- scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
-
- DISALLOW_COPY_AND_ASSIGN(SBDatabaseClient);
-};
-
SubresourceFilterSafeBrowsingActivationThrottle::
SubresourceFilterSafeBrowsingActivationThrottle(
content::NavigationHandle* handle,
+ scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
database_manager)
: NavigationThrottle(handle),
- io_task_runner_(content::BrowserThread::GetTaskRunnerForThread(
- content::BrowserThread::IO)),
- database_client_(
- new SubresourceFilterSafeBrowsingActivationThrottle::SBDatabaseClient(
- std::move(database_manager),
- AsWeakPtr(),
- base::ThreadTaskRunnerHandle::Get()),
- base::OnTaskRunnerDeleter(io_task_runner_)) {}
+ database_manager_(std::move(database_manager)),
+ io_task_runner_(io_task_runner),
+ database_client_(new SubresourceFilterSafeBrowsingClient(
+ database_manager_,
+ AsWeakPtr(),
+ io_task_runner,
+ base::ThreadTaskRunnerHandle::Get()),
+ base::OnTaskRunnerDeleter(io_task_runner_)) {}
SubresourceFilterSafeBrowsingActivationThrottle::
- ~SubresourceFilterSafeBrowsingActivationThrottle() {}
+ ~SubresourceFilterSafeBrowsingActivationThrottle() {
+ // TODO(csharrison): Log metrics based on check_results_.
+}
+
+content::NavigationThrottle::ThrottleCheckResult
+SubresourceFilterSafeBrowsingActivationThrottle::WillStartRequest() {
+ CheckCurrentUrl();
+ return content::NavigationThrottle::ThrottleCheckResult::PROCEED;
+}
+
+content::NavigationThrottle::ThrottleCheckResult
+SubresourceFilterSafeBrowsingActivationThrottle::WillRedirectRequest() {
+ CheckCurrentUrl();
+ return content::NavigationThrottle::ThrottleCheckResult::PROCEED;
+}
content::NavigationThrottle::ThrottleCheckResult
SubresourceFilterSafeBrowsingActivationThrottle::WillProcessResponse() {
- io_task_runner_->PostTask(
- FROM_HERE, base::Bind(&SubresourceFilterSafeBrowsingActivationThrottle::
- SBDatabaseClient::CheckUrlOnIO,
- base::Unretained(database_client_.get()),
- navigation_handle()->GetURL()));
+ // No need to defer the navigation if the check already happened.
+ if (check_results_.back().finished) {
+ NotifyResult();
+ return content::NavigationThrottle::ThrottleCheckResult::PROCEED;
+ }
+ defer_time_ = base::TimeTicks::Now();
return content::NavigationThrottle::ThrottleCheckResult::DEFER;
}
@@ -128,22 +67,59 @@ SubresourceFilterSafeBrowsingActivationThrottle::GetNameForLogging() {
}
void SubresourceFilterSafeBrowsingActivationThrottle::OnCheckUrlResultOnUI(
- const GURL& url,
- safe_browsing::SBThreatType threat_type,
- safe_browsing::ThreatPatternType pattern_type) {
- content::WebContents* web_contents = navigation_handle()->GetWebContents();
- if (web_contents) {
- using subresource_filter::ContentSubresourceFilterDriverFactory;
- ContentSubresourceFilterDriverFactory* driver_factory =
- ContentSubresourceFilterDriverFactory::FromWebContents(web_contents);
- DCHECK(driver_factory);
-
- driver_factory->OnMainResourceMatchedSafeBrowsingBlacklist(
- url, std::vector<GURL>(), threat_type, pattern_type);
+ const SubresourceFilterSafeBrowsingClient::CheckResult& result) {
+ DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+ size_t request_id = result.request_id;
+ DCHECK_LT(request_id, check_results_.size());
+
+ auto& stored_result = check_results_.at(request_id);
+ DCHECK(!stored_result.finished);
+ stored_result = result;
+ if (!defer_time_.is_null() && request_id == check_results_.size() - 1) {
+ NotifyResult();
+ navigation_handle()->Resume();
}
- // TODO(https://crbug.com/704508): We should measure the delay introduces by
- // this check. Similarly, as it's done the Safe Browsing Resource throttle.
- navigation_handle()->Resume();
+}
+
+void SubresourceFilterSafeBrowsingActivationThrottle::CheckCurrentUrl() {
+ check_results_.emplace_back();
+ size_t id = check_results_.size() - 1;
+ io_task_runner_->PostTask(
+ FROM_HERE, base::Bind(&SubresourceFilterSafeBrowsingClient::CheckUrlOnIO,
+ base::Unretained(database_client_.get()),
+ navigation_handle()->GetURL(), id));
+}
+
+void SubresourceFilterSafeBrowsingActivationThrottle::NotifyResult() {
+ content::WebContents* web_contents = navigation_handle()->GetWebContents();
+ if (!web_contents)
+ return;
+
+ using subresource_filter::ContentSubresourceFilterDriverFactory;
+
+ const SubresourceFilterSafeBrowsingClient::CheckResult& result =
+ check_results_.back();
+ ContentSubresourceFilterDriverFactory* driver_factory =
+ ContentSubresourceFilterDriverFactory::FromWebContents(web_contents);
+ DCHECK(driver_factory);
+
+ driver_factory->OnMainResourceMatchedSafeBrowsingBlacklist(
+ navigation_handle()->GetURL(), std::vector<GURL>(), result.threat_type,
+ result.pattern_type);
+
+ base::TimeDelta delay = defer_time_.is_null()
+ ? base::TimeDelta::FromMilliseconds(0)
+ : base::TimeTicks::Now() - defer_time_;
+ UMA_HISTOGRAM_TIMES("SubresourceFilter.PageLoad.SafeBrowsingDelay", delay);
+
+ // Log a histogram for the delay we would have introduced if the throttle only
+ // speculatively checks URLs on WillStartRequest. This is only different from
+ // the actual delay if there was at least one redirect.
+ base::TimeDelta no_redirect_speculation_delay =
+ check_results_.size() > 1 ? result.check_time : delay;
+ UMA_HISTOGRAM_TIMES(
+ "SubresourceFilter.PageLoad.SafeBrowsingDelay.NoRedirectSpeculation",
+ no_redirect_speculation_delay);
}
} // namespace subresource_filter

Powered by Google App Engine
This is Rietveld 408576698