[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 4d728239ced8a6a02b0dd5b64e52c2020a1896e8..b761e13cd1ea4470720a9ab7e432510b751010e2 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_.get(),

[engedy, 2017/04/25 22:03:53]

nit: s/.get()//

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+                           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() {
+  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 (check_results_.back().finished) {
+    NotifyResult();
+    return content::NavigationThrottle::ThrottleCheckResult::PROCEED;
+  }
+  defer_time_ = base::TimeTicks::Now();
   return content::NavigationThrottle::ThrottleCheckResult::DEFER;
 }
 
@@ -129,21 +68,53 @@ SubresourceFilterSafeBrowsingActivationThrottle::GetNameForLogging() {
 
 void SubresourceFilterSafeBrowsingActivationThrottle::OnCheckUrlResultOnUI(
     const GURL& url,

[engedy, 2017/04/25 22:03:53]

nit: |url| never used

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+    size_t 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);
+  DCHECK_LE(request_id, check_results_.size());

[engedy, 2017/04/25 22:03:53]

DCHECK_LT

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+
+  CheckResult& result = check_results_[request_id];
+  DCHECK(!result.finished);
+  result.threat_type = threat_type;
+  result.pattern_type = pattern_type;
+  result.finished = true;
+
+  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::CheckUrl() {

[engedy, 2017/04/25 22:03:53]

nit: Maybe CheckCurrentURL() or something more qualified?

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+  check_results_.emplace_back();
+  int id = check_results_.size() - 1;

[engedy, 2017/04/25 22:03:53]

nit: Let's make sure to use size_t consistently.

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+  io_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SubresourceFilterSafeBrowsingClient::CheckUrlOnIO,
+                            base::Unretained(database_client_.get()),

[Nathan Parker, 2017/04/24 22:05:17]

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).

[engedy, 2017/04/25 22:03:53]

Yes, the CL creates new SBDBMgr::Clients, so we should be good.

+                            navigation_handle()->GetURL(), id));
+}
+
+void SubresourceFilterSafeBrowsingActivationThrottle::NotifyResult() {
+  content::WebContents* web_contents = navigation_handle()->GetWebContents();
+  if (!web_contents)
+    return;
+
+  using subresource_filter::ContentSubresourceFilterDriverFactory;
+
+  const 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);
 }
 
 }  //  namespace subresource_filter