[subresource_filter] SB throttle can send multiple speculative requests.

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc

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 d75a56a34888595fe72228aeef84b87fccbfc5ca..4646dcf532822bbe9208911054eb9a276c0eb4c1 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
@@ -4,141 +4,111 @@
 
 #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,
+        std::unique_ptr<base::Timer> timer,
         scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
             database_manager)
     : NavigationThrottle(handle),
+      database_manager_(std::move(database_manager)),
       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_client_(new SubresourceFilterSafeBrowsingClient(
+                           std::move(timer),
+                           database_manager_.get(),
+                           AsWeakPtr(),
+                           base::ThreadTaskRunnerHandle::Get()),
+                       base::OnTaskRunnerDeleter(io_task_runner_)) {}
 
 SubresourceFilterSafeBrowsingActivationThrottle::
     ~SubresourceFilterSafeBrowsingActivationThrottle() {}
 
 content::NavigationThrottle::ThrottleCheckResult
+SubresourceFilterSafeBrowsingActivationThrottle::WillStartRequest() {
+  CheckUrl();
+  return content::NavigationThrottle::ThrottleCheckResult::PROCEED;
+}
+
+content::NavigationThrottle::ThrottleCheckResult
+SubresourceFilterSafeBrowsingActivationThrottle::WillRedirectRequest() {
+  CheckUrl();
+  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 (last_received_request_id_ == current_request_id_) {
+    NotifyResult();
+    return content::NavigationThrottle::ThrottleCheckResult::PROCEED;
+  }
+  defer_time_ = base::TimeTicks::Now();
   return content::NavigationThrottle::ThrottleCheckResult::DEFER;
 }
 
 void SubresourceFilterSafeBrowsingActivationThrottle::OnCheckUrlResultOnUI(
     const GURL& url,
+    int request_id,
     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);
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // Ignore results from previous requests if there is a more recent one in
+  // flight by the time this task is run.
+  if (current_request_id_ != request_id)
+    return;
+  last_received_request_id_ = request_id;
+
+  DCHECK_EQ(navigation_handle()->GetURL(), url);
+  threat_type_ = threat_type;
+  pattern_type_ = pattern_type;
+
+  // If the throttle has deferred the navigation, there are no more redirects
+  // and this is the final URL.
+  if (!defer_time_.is_null()) {
+    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::CheckUrl() {
+  io_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SubresourceFilterSafeBrowsingClient::CheckUrlOnIO,
+                 base::Unretained(database_client_.get()),
+                 navigation_handle()->GetURL(), ++current_request_id_));
+}
+
+void SubresourceFilterSafeBrowsingActivationThrottle::NotifyResult() {
+  content::WebContents* web_contents = navigation_handle()->GetWebContents();
+  if (!web_contents)
+    return;
+  using subresource_filter::ContentSubresourceFilterDriverFactory;
+  ContentSubresourceFilterDriverFactory* driver_factory =
+      ContentSubresourceFilterDriverFactory::FromWebContents(web_contents);
+  DCHECK(driver_factory);
+
+  driver_factory->OnMainResourceMatchedSafeBrowsingBlacklist(
+      navigation_handle()->GetURL(), std::vector<GURL>(), threat_type_,
+      pattern_type_);
+
+  base::TimeDelta delay = defer_time_.is_null()
+                              ? base::TimeDelta::FromMilliseconds(0)
+                              : base::TimeTicks::Now() - defer_time_;
+  UMA_HISTOGRAM_TIMES("SubresourceFilter.SafeBrowsing.NavigationDelay", delay);
 }
 
 }  //  namespace subresource_filter