[subresource_filter] SB throttle can send multiple speculative requests.

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc

-// 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,
+        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) {
+  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)

[engedy, 2017/04/20 18:56:23]

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.

[engedy, 2017/04/20 18:58:17]

requests, terms

[Charlie Harrison, 2017/04/24 18:25:10]

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.

+    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();
+  }
+}
+
+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) {
-    using subresource_filter::ContentSubresourceFilterDriverFactory;
-    ContentSubresourceFilterDriverFactory* driver_factory =
-        ContentSubresourceFilterDriverFactory::FromWebContents(web_contents);
-    DCHECK(driver_factory);
+  if (!web_contents)
+    return;
+  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);
-  }
-  // 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();
+  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);

[engedy, 2017/04/20 18:56:23]

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.

[Charlie Harrison, 2017/04/24 18:25:10]

Done and done.

 }
 
 }  //  namespace subresource_filter