Add a feature flag "S13nSafeBrowsingParallelUrlCheck".

If enabled, SafeBrowsing URL checks will behave in the same way as when Network
Service is enabled. In other words, they don't defer starting requests or
following redirects, no matter on desktop or mobile. Instead they only defer
response processing.

Currently the feature is disabled by default. Follow-up work:
- Make the corresponding change on Android WebView.
- Following the finch experiment process, send out a doc/email to request a
  finch experiment.
- Remove the old behavior eventually.

Bug: 715673
Change-Id: I9b23efc58965a82dcb3ba0ccaa91a77bdccbca92
Reviewed-on: https://chromium-review.googlesource.com/625034
Commit-Queue: Yuzhu Shen <yzshen@chromium.org>
Reviewed-by: Varun Khaneja (slow) <vakh@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496481}

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc

@@ -712,7 +712,7 @@ void ChromeResourceDispatcherHostDelegate::AppendStandardResourceThrottles(
 
 #if defined(SAFE_BROWSING_DB_LOCAL) || defined(SAFE_BROWSING_DB_REMOTE)
   if (!first_throttle && io_data->safe_browsing_enabled()->GetValue()) {
-    first_throttle = SafeBrowsingResourceThrottle::MaybeCreate(
+    first_throttle = MaybeCreateSafeBrowsingResourceThrottle(
         request, resource_type, safe_browsing_.get());
   }
 #endif  // defined(SAFE_BROWSING_DB_LOCAL) || defined(SAFE_BROWSING_DB_REMOTE)

chrome/browser/loader/safe_browsing_resource_throttle.cc

@@ -7,19 +7,23 @@
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
 #include "components/safe_browsing/base_ui_manager.h"
+#include "components/safe_browsing/features.h"
 #include "components/safe_browsing_db/v4_protocol_manager_util.h"
 #include "content/public/browser/resource_request_info.h"
 #include "net/http/http_request_headers.h"
 
-// static
-SafeBrowsingResourceThrottle* SafeBrowsingResourceThrottle::MaybeCreate(
+content::ResourceThrottle* MaybeCreateSafeBrowsingResourceThrottle(
     net::URLRequest* request,
     content::ResourceType resource_type,
     safe_browsing::SafeBrowsingService* sb_service) {
-  if (sb_service->database_manager()->IsSupported()) {
-    return new SafeBrowsingResourceThrottle(request, resource_type, sb_service);
+  if (!sb_service->database_manager()->IsSupported())
+    return nullptr;
+
+  if (base::FeatureList::IsEnabled(safe_browsing::kParallelUrlCheck)) {
+    return new SafeBrowsingParallelResourceThrottle(request, resource_type,
+                                                    sb_service);
   }
-  return nullptr;
+  return new SafeBrowsingResourceThrottle(request, resource_type, sb_service);
 }
 
 SafeBrowsingResourceThrottle::SafeBrowsingResourceThrottle(
@@ -39,7 +43,7 @@ SafeBrowsingResourceThrottle::SafeBrowsingResourceThrottle(
           sb_service->database_manager(),
           sb_service->ui_manager())) {}
 
-SafeBrowsingResourceThrottle::~SafeBrowsingResourceThrottle() {}
+SafeBrowsingResourceThrottle::~SafeBrowsingResourceThrottle() = default;
 
 const char* SafeBrowsingResourceThrottle::GetNameForLogging() const {
   return "SafeBrowsingResourceThrottle";
@@ -58,3 +62,21 @@ void SafeBrowsingResourceThrottle::StartDisplayingBlockingPageHelper(
       resource, std::string(), net::HttpRequestHeaders(),
       false /* is_main_frame */, false /* has_user_gesture */);
 }
+
+SafeBrowsingParallelResourceThrottle::SafeBrowsingParallelResourceThrottle(
+    const net::URLRequest* request,
+    content::ResourceType resource_type,
+    safe_browsing::SafeBrowsingService* sb_service)
+    : safe_browsing::BaseParallelResourceThrottle(
+          request,
+          resource_type,
+          new safe_browsing::UrlCheckerDelegateImpl(
+              sb_service->database_manager(),
+              sb_service->ui_manager())) {}
+
+SafeBrowsingParallelResourceThrottle::~SafeBrowsingParallelResourceThrottle() =
+    default;
+
+const char* SafeBrowsingParallelResourceThrottle::GetNameForLogging() const {
+  return "SafeBrowsingParallelResourceThrottle";
+}

chrome/browser/loader/safe_browsing_resource_throttle.h

@@ -9,6 +9,7 @@
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/safe_browsing/url_checker_delegate_impl.h"
 #include "components/safe_browsing/base_resource_throttle.h"
+#include "components/safe_browsing/browser/base_parallel_resource_throttle.h"
 #include "content/public/browser/resource_throttle.h"
 #include "content/public/common/resource_type.h"
 
@@ -20,6 +21,18 @@ namespace safe_browsing {
 class SafeBrowsingService;
 }
 
+// Contructs a resource throttle for SafeBrowsing. It returns a
+// SafeBrowsingParallelResourceThrottle instance if
+// --enable-features=S13nSafeBrowsingParallelUrlCheck is specified; returns
+// a SafeBrowsingResourceThrottle otherwise.
+//
+// It could return nullptr if URL checking is not supported on this
+// build+device.
+content::ResourceThrottle* MaybeCreateSafeBrowsingResourceThrottle(
+    net::URLRequest* request,
+    content::ResourceType resource_type,
+    safe_browsing::SafeBrowsingService* sb_service);
+
 // SafeBrowsingResourceThrottle functions as its base class
 // safe_browsing::BaseResourceThrottle, but dispatches to either the local or
 // remote SafeBrowsingDatabaseManager, depending on if running on desktop or
@@ -50,24 +63,20 @@ class SafeBrowsingService;
 // always defer.
 class SafeBrowsingResourceThrottle
     : public safe_browsing::BaseResourceThrottle {
- public:
-  // Will construct a SafeBrowsingResourceThrottle, or return NULL
-  // if on Android and not in the field trial.
-  static SafeBrowsingResourceThrottle* MaybeCreate(
+ private:
+  friend content::ResourceThrottle* MaybeCreateSafeBrowsingResourceThrottle(
       net::URLRequest* request,
       content::ResourceType resource_type,
       safe_browsing::SafeBrowsingService* sb_service);
 
-  const char* GetNameForLogging() const override;
-
- protected:
   SafeBrowsingResourceThrottle(const net::URLRequest* request,
                                content::ResourceType resource_type,
                                safe_browsing::SafeBrowsingService* sb_service);
 
- private:
   ~SafeBrowsingResourceThrottle() override;
 
+  const char* GetNameForLogging() const override;
+
   // This posts a task to destroy prerender contents
   void MaybeDestroyPrerenderContents(
       const content::ResourceRequestInfo* info) override;
@@ -80,4 +89,29 @@ class SafeBrowsingResourceThrottle
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingResourceThrottle);
 };
 
+// Unlike SafeBrowsingResourceThrottle, this class never defers starting the URL
+// request or following redirects, no matter on mobile or desktop. If any of the
+// checks for the original URL and redirect chain are not complete by the time
+// the response headers are available, the request is deferred until all the
+// checks are done.
+class SafeBrowsingParallelResourceThrottle
+    : public safe_browsing::BaseParallelResourceThrottle {
+ private:
+  friend content::ResourceThrottle* MaybeCreateSafeBrowsingResourceThrottle(
+      net::URLRequest* request,
+      content::ResourceType resource_type,
+      safe_browsing::SafeBrowsingService* sb_service);
+
+  SafeBrowsingParallelResourceThrottle(
+      const net::URLRequest* request,
+      content::ResourceType resource_type,
+      safe_browsing::SafeBrowsingService* sb_service);
+
+  ~SafeBrowsingParallelResourceThrottle() override;
+
+  const char* GetNameForLogging() const override;
+
+  DISALLOW_COPY_AND_ASSIGN(SafeBrowsingParallelResourceThrottle);
+};
+
 #endif  // CHROME_BROWSER_LOADER_SAFE_BROWSING_RESOURCE_THROTTLE_H_

components/safe_browsing/BUILD.gn

@@ -24,6 +24,8 @@ source_set("safe_browsing") {
     "base_resource_throttle.h",
     "base_ui_manager.cc",
     "base_ui_manager.h",
+    "net_event_logger.cc",
+    "net_event_logger.h",
   ]
   public_deps = [
     "//components/security_interstitials/content:security_interstitial_page",

components/safe_browsing/base_resource_throttle.cc

@@ -8,7 +8,6 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/trace_event/trace_event.h"
-#include "base/values.h"
 #include "components/safe_browsing/base_ui_manager.h"
 #include "components/safe_browsing/common/utils.h"
 #include "components/safe_browsing/web_ui/constants.h"
@@ -18,14 +17,11 @@
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/load_flags.h"
-#include "net/log/net_log_capture_mode.h"
-#include "net/log/net_log_source.h"
-#include "net/log/net_log_source_type.h"
+#include "net/log/net_log_event_type.h"
 #include "net/url_request/redirect_info.h"
 #include "net/url_request/url_request.h"
 
 using net::NetLogEventType;
-using net::NetLogSourceType;
 
 namespace safe_browsing {
 
@@ -36,35 +32,6 @@ namespace {
 // aborted, and the URL will be treated as if it were safe.
 const int kCheckUrlTimeoutMs = 5000;
 
-// Return a dictionary with "url"=|url-spec| and optionally
-// |name|=|value| (if not null), for netlogging.
-// This will also add a reference to the original request's net_log ID.
-std::unique_ptr<base::Value> NetLogUrlCallback(
-    const net::URLRequest* request,
-    const GURL& url,
-    const char* name,
-    const char* value,
-    net::NetLogCaptureMode /* capture_mode */) {
-  std::unique_ptr<base::DictionaryValue> event_params(
-      new base::DictionaryValue());
-  event_params->SetString("url", url.spec());
-  if (name && value)
-    event_params->SetString(name, value);
-  request->net_log().source().AddToEventParameters(event_params.get());
-  return std::move(event_params);
-}
-
-// Return a dictionary with |name|=|value|, for netlogging.
-std::unique_ptr<base::Value> NetLogStringCallback(const char* name,
-                                                  const char* value,
-                                                  net::NetLogCaptureMode) {
-  std::unique_ptr<base::DictionaryValue> event_params(
-      new base::DictionaryValue());
-  if (name && value)
-    event_params->SetString(name, value);
-  return std::move(event_params);
-}
-
 }  // namespace
 
 // TODO(eroman): Downgrade these CHECK()s to DCHECKs once there is more
@@ -84,44 +51,22 @@ BaseResourceThrottle::BaseResourceThrottle(
       state_(STATE_NONE),
       defer_state_(DEFERRED_NONE),
       resource_type_(resource_type),
-      net_log_with_source_(
-          net::NetLogWithSource::Make(request->net_log().net_log(),
-                                      NetLogSourceType::SAFE_BROWSING)) {}
+      net_event_logger_(&request->net_log()) {}
 
 BaseResourceThrottle::~BaseResourceThrottle() {
   if (defer_state_ != DEFERRED_NONE) {
-    EndNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, nullptr, nullptr);
+    net_event_logger_.EndNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED,
+                                     nullptr, nullptr);
   }
 
   if (state_ == STATE_CHECKING_URL) {
     database_manager_->CancelCheck(this);
-    EndNetLogEvent(NetLogEventType::SAFE_BROWSING_CHECKING_URL, "result",
-                   "request_canceled");
+    net_event_logger_.EndNetLogEvent(
+        NetLogEventType::SAFE_BROWSING_CHECKING_URL, "result",
+        "request_canceled");
   }
 }
 
-// Note on net_log calls: SAFE_BROWSING_DEFERRED events must be wholly
-// nested within SAFE_BROWSING_CHECKING_URL events.  Synchronous checks
-// are not logged at all.
-void BaseResourceThrottle::BeginNetLogEvent(NetLogEventType type,
-                                            const GURL& url,
-                                            const char* name,
-                                            const char* value) {
-  net_log_with_source_.BeginEvent(
-      type, base::Bind(&NetLogUrlCallback, request_, url, name, value));
-  request_->net_log().AddEvent(
-      type, net_log_with_source_.source().ToEventParametersCallback());
-}
-
-void BaseResourceThrottle::EndNetLogEvent(NetLogEventType type,
-                                          const char* name,
-                                          const char* value) {
-  net_log_with_source_.EndEvent(type,
-                                base::Bind(&NetLogStringCallback, name, value));
-  request_->net_log().AddEvent(
-      type, net_log_with_source_.source().ToEventParametersCallback());
-}
-
 void BaseResourceThrottle::WillStartRequest(bool* defer) {
   // We need to check the new URL before starting the request.
   if (CheckUrl(request_->url()))
@@ -138,8 +83,9 @@ void BaseResourceThrottle::WillStartRequest(bool* defer) {
   defer_state_ = DEFERRED_START;
   defer_start_time_ = base::TimeTicks::Now();
   *defer = true;
-  BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, request_->url(),
-                   "defer_reason", "at_start");
+  net_event_logger_.BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED,
+                                     request_->url(), "defer_reason",
+                                     "at_start");
 }
 
 void BaseResourceThrottle::WillProcessResponse(bool* defer) {
@@ -153,8 +99,9 @@ void BaseResourceThrottle::WillProcessResponse(bool* defer) {
     defer_state_ = DEFERRED_PROCESSING;
     defer_start_time_ = base::TimeTicks::Now();
     *defer = true;
-    BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, request_->url(),
-                     "defer_reason", "at_response");
+    net_event_logger_.BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED,
+                                       request_->url(), "defer_reason",
+                                       "at_response");
   }
 }
 
@@ -201,7 +148,7 @@ void BaseResourceThrottle::WillRedirectRequest(
 
   defer_start_time_ = base::TimeTicks::Now();
   *defer = true;
-  BeginNetLogEvent(
+  net_event_logger_.BeginNetLogEvent(
       NetLogEventType::SAFE_BROWSING_DEFERRED, redirect_info.new_url,
       "defer_reason",
       defer_state_ == DEFERRED_REDIRECT ? "redirect" : "unchecked_redirect");
@@ -230,9 +177,10 @@ void BaseResourceThrottle::OnCheckBrowseUrlResult(
   state_ = STATE_NONE;
 
   if (defer_state_ != DEFERRED_NONE) {
-    EndNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, nullptr, nullptr);
+    net_event_logger_.EndNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED,
+                                     nullptr, nullptr);
   }
-  EndNetLogEvent(
+  net_event_logger_.EndNetLogEvent(
       NetLogEventType::SAFE_BROWSING_CHECKING_URL, "result",
       threat_type_ == SB_THREAT_TYPE_SAFE ? "safe" : "unsafe");
 
@@ -338,8 +286,9 @@ bool BaseResourceThrottle::CheckUrl(const GURL& url) {
 
   state_ = STATE_CHECKING_URL;
   url_being_checked_ = url;
-  BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_CHECKING_URL, url, nullptr,
-                   nullptr);
+  // Note on net_log calls: Synchronous checks are not logged at all.
+  net_event_logger_.BeginNetLogEvent(
+      NetLogEventType::SAFE_BROWSING_CHECKING_URL, url, nullptr, nullptr);
 
   // Start a timer to abort the check if it takes too long.
   // TODO(nparker): Set this only when we defer, based on remaining time,
@@ -393,9 +342,9 @@ void BaseResourceThrottle::ResumeRequest() {
       // We're now waiting for the unchecked_redirect_url_.
       defer_state_ = DEFERRED_REDIRECT;
       resume = false;
-      BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED,
-                       unchecked_redirect_url_, "defer_reason",
-                       "resumed_redirect");
+      net_event_logger_.BeginNetLogEvent(
+          NetLogEventType::SAFE_BROWSING_DEFERRED, unchecked_redirect_url_,
+          "defer_reason", "resumed_redirect");
     }
   }
 

components/safe_browsing/base_resource_throttle.h

@@ -12,13 +12,12 @@
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "components/safe_browsing/base_ui_manager.h"
+#include "components/safe_browsing/net_event_logger.h"
 #include "components/safe_browsing_db/database_manager.h"
 #include "components/safe_browsing_db/v4_protocol_manager_util.h"
 #include "components/security_interstitials/content/unsafe_resource.h"
 #include "content/public/browser/resource_throttle.h"
 #include "content/public/common/resource_type.h"
-#include "net/log/net_log_event_type.h"
-#include "net/log/net_log_with_source.h"
 #include "url/gurl.h"
 
 namespace content {
@@ -140,15 +139,6 @@ class BaseResourceThrottle
 
   void ResumeRequest();
 
-  // For marking network events.  |name| and |value| can be null.
-  void BeginNetLogEvent(net::NetLogEventType type,
-                        const GURL& url,
-                        const char* name,
-                        const char* value);
-  void EndNetLogEvent(net::NetLogEventType type,
-                      const char* name,
-                      const char* value);
-
   // The result of the most recent safe browsing check. Only valid to read this
   // when state_ != STATE_CHECKING_URL.
   safe_browsing::SBThreatType threat_type_;
@@ -175,7 +165,7 @@ class BaseResourceThrottle
   DeferState defer_state_;
 
   const content::ResourceType resource_type_;
-  net::NetLogWithSource net_log_with_source_;
+  NetEventLogger net_event_logger_;
 
   DISALLOW_COPY_AND_ASSIGN(BaseResourceThrottle);
 };

components/safe_browsing/browser/BUILD.gn

@@ -6,6 +6,8 @@ import("//build/config/features.gni")
 
 source_set("browser") {
   sources = [
+    "base_parallel_resource_throttle.cc",
+    "base_parallel_resource_throttle.h",
     "browser_url_loader_throttle.cc",
     "browser_url_loader_throttle.h",
     "mojo_safe_browsing_impl.cc",

components/safe_browsing/browser/base_parallel_resource_throttle.cc

+// 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/safe_browsing/browser/base_parallel_resource_throttle.h"
+
+#include <utility>
+
+#include "components/safe_browsing/browser/browser_url_loader_throttle.h"
+#include "components/safe_browsing/browser/url_checker_delegate.h"
+#include "content/public/browser/resource_request_info.h"
+#include "content/public/common/resource_request.h"
+#include "net/http/http_request_headers.h"
+#include "net/log/net_log_with_source.h"
+#include "net/url_request/url_request.h"
+
+namespace safe_browsing {
+
+void BaseParallelResourceThrottle::URLLoaderThrottleDelegateImpl::
+    CancelWithError(int error_code) {
+  owner_->CancelWithError(error_code);
+}
+
+void BaseParallelResourceThrottle::URLLoaderThrottleDelegateImpl::Resume() {
+  owner_->Resume();
+}
+
+BaseParallelResourceThrottle::BaseParallelResourceThrottle(
+    const net::URLRequest* request,
+    content::ResourceType resource_type,
+    scoped_refptr<UrlCheckerDelegate> url_checker_delegate)
+    : request_(request),
+      resource_type_(resource_type),
+      url_loader_throttle_delegate_(this),
+      net_event_logger_(&request->net_log()) {
+  const content::ResourceRequestInfo* info =
+      content::ResourceRequestInfo::ForRequest(request_);
+  url_loader_throttle_ = BrowserURLLoaderThrottle::MaybeCreate(
+      std::move(url_checker_delegate), info->GetWebContentsGetterForRequest());
+  url_loader_throttle_->set_delegate(&url_loader_throttle_delegate_);
+  url_loader_throttle_->set_net_event_logger(&net_event_logger_);
+}
+
+BaseParallelResourceThrottle::~BaseParallelResourceThrottle() = default;
+
+void BaseParallelResourceThrottle::WillStartRequest(bool* defer) {
+  content::ResourceRequest resource_request;
+
+  net::HttpRequestHeaders full_headers;
+  resource_request.headers = request_->GetFullRequestHeaders(&full_headers)
+                                 ? full_headers.ToString()
+                                 : request_->extra_request_headers().ToString();
+
+  resource_request.load_flags = request_->load_flags();
+  resource_request.resource_type = resource_type_;
+
+  const content::ResourceRequestInfo* info =
+      content::ResourceRequestInfo::ForRequest(request_);
+  resource_request.has_user_gesture = info && info->HasUserGesture();
+
+  resource_request.url = request_->url();
+  resource_request.method = request_->method();
+
+  url_loader_throttle_->WillStartRequest(resource_request, defer);
+  DCHECK(!*defer);
+}
+
+void BaseParallelResourceThrottle::WillRedirectRequest(
+    const net::RedirectInfo& redirect_info,
+    bool* defer) {
+  url_loader_throttle_->WillRedirectRequest(redirect_info, defer);
+  DCHECK(!*defer);
+}
+
+void BaseParallelResourceThrottle::WillProcessResponse(bool* defer) {
+  url_loader_throttle_->WillProcessResponse(defer);
+}
+const char* BaseParallelResourceThrottle::GetNameForLogging() const {
+  return "BaseParallelResourceThrottle";
+}
+
+}  // namespace safe_browsing

components/safe_browsing/browser/base_parallel_resource_throttle.h

+// 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.
+
+#ifndef COMPONENTS_SAFE_BROWSING_BROWSER_BASE_PARALLEL_RESOURCE_THROTTLE_H_
+#define COMPONENTS_SAFE_BROWSING_BROWSER_BASE_PARALLEL_RESOURCE_THROTTLE_H_
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "components/safe_browsing/net_event_logger.h"
+#include "content/public/browser/resource_throttle.h"
+#include "content/public/common/resource_type.h"
+#include "content/public/common/url_loader_throttle.h"
+
+namespace net {
+class URLRequest;
+}
+
+namespace safe_browsing {
+
+class BrowserURLLoaderThrottle;
+class UrlCheckerDelegate;
+
+// A thin wrapper around BrowserURLLoaderThrottle to adapt to the
+// content::ResourceThrottle interface.
+// Used when --enable-features=S13nSafeBrowsingParallelUrlCheck is in effect.
+class BaseParallelResourceThrottle : public content::ResourceThrottle {
+ protected:
+  BaseParallelResourceThrottle(
+      const net::URLRequest* request,
+      content::ResourceType resource_type,
+      scoped_refptr<UrlCheckerDelegate> url_checker_delegate);
+
+  ~BaseParallelResourceThrottle() override;
+
+  // content::ResourceThrottle implementation:
+  void WillStartRequest(bool* defer) override;
+  void WillRedirectRequest(const net::RedirectInfo& redirect_info,
+                           bool* defer) override;
+  void WillProcessResponse(bool* defer) override;
+  const char* GetNameForLogging() const override;
+
+ private:
+  class URLLoaderThrottleDelegateImpl
+      : public content::URLLoaderThrottle::Delegate {
+   public:
+    explicit URLLoaderThrottleDelegateImpl(BaseParallelResourceThrottle* owner)
+        : owner_(owner) {}
+    ~URLLoaderThrottleDelegateImpl() override = default;
+
+    // content::URLLoaderThrottle::Delegate implementation:
+    void CancelWithError(int error_code) override;
+    void Resume() override;
+
+   private:
+    BaseParallelResourceThrottle* const owner_;
+    DISALLOW_COPY_AND_ASSIGN(URLLoaderThrottleDelegateImpl);
+  };
+
+  const net::URLRequest* const request_;
+  const content::ResourceType resource_type_;
+
+  // The following two members need to outlive |url_loader_throttle_| which
+  // holds raw pointers to them.
+  URLLoaderThrottleDelegateImpl url_loader_throttle_delegate_;
+  NetEventLogger net_event_logger_;
+
+  std::unique_ptr<BrowserURLLoaderThrottle> url_loader_throttle_;
+
+  DISALLOW_COPY_AND_ASSIGN(BaseParallelResourceThrottle);
+};
+
+}  // namespace safe_browsing
+
+#endif  // COMPONENTS_SAFE_BROWSING_BROWSER_BASE_PARALLEL_RESOURCE_THROTTLE_H_

components/safe_browsing/browser/browser_url_loader_throttle.cc

@@ -8,7 +8,9 @@
 #include "components/safe_browsing/browser/safe_browsing_url_checker_impl.h"
 #include "components/safe_browsing/browser/url_checker_delegate.h"
 #include "components/safe_browsing/common/utils.h"
+#include "components/safe_browsing/net_event_logger.h"
 #include "content/public/common/resource_request.h"
+#include "net/log/net_log_event_type.h"
 #include "net/url_request/redirect_info.h"
 
 namespace safe_browsing {
@@ -33,7 +35,12 @@ BrowserURLLoaderThrottle::BrowserURLLoaderThrottle(
     : url_checker_delegate_(std::move(url_checker_delegate)),
       web_contents_getter_(web_contents_getter) {}
 
-BrowserURLLoaderThrottle::~BrowserURLLoaderThrottle() = default;
+BrowserURLLoaderThrottle::~BrowserURLLoaderThrottle() {
+  if (deferred_ && net_event_logger_) {
+    net_event_logger_->EndNetLogEvent(
+        net::NetLogEventType::SAFE_BROWSING_DEFERRED, nullptr, nullptr);
+  }
+}
 
 void BrowserURLLoaderThrottle::WillStartRequest(
     const content::ResourceRequest& request,
@@ -47,6 +54,9 @@ void BrowserURLLoaderThrottle::WillStartRequest(
       request.headers, request.load_flags, request.resource_type,
       request.has_user_gesture, std::move(url_checker_delegate_),
       web_contents_getter_);
+  if (net_event_logger_)
+    url_checker_->set_net_event_logger(net_event_logger_);
+
   url_checker_->CheckUrl(
       request.url, request.method,
       base::BindOnce(&BrowserURLLoaderThrottle::OnCheckUrlResult,
@@ -81,6 +91,18 @@ void BrowserURLLoaderThrottle::WillProcessResponse(bool* defer) {
   deferred_ = true;
   defer_start_time_ = base::TimeTicks::Now();
   *defer = true;
+  if (net_event_logger_) {
+    net_event_logger_->BeginNetLogEvent(
+        net::NetLogEventType::SAFE_BROWSING_DEFERRED,
+        url_checker_->GetCurrentlyCheckingUrl(), "defer_reason", "at_response");
+  }
+}
+
+void BrowserURLLoaderThrottle::set_net_event_logger(
+    NetEventLogger* net_event_logger) {
+  net_event_logger_ = net_event_logger;
+  if (url_checker_)
+    url_checker_->set_net_event_logger(net_event_logger);
 }
 
 void BrowserURLLoaderThrottle::OnCheckUrlResult(bool proceed,
@@ -95,6 +117,11 @@ void BrowserURLLoaderThrottle::OnCheckUrlResult(bool proceed,
     if (pending_checks_ == 0 && deferred_) {
       LogDelay(base::TimeTicks::Now() - defer_start_time_);
       deferred_ = false;
+      if (net_event_logger_) {
+        net_event_logger_->EndNetLogEvent(
+            net::NetLogEventType::SAFE_BROWSING_DEFERRED, nullptr, nullptr);
+      }
+
       delegate_->Resume();
     }
   } else {

components/safe_browsing/browser/browser_url_loader_throttle.h

@@ -19,14 +19,14 @@ class WebContents;
 
 namespace safe_browsing {
 
-class UrlCheckerDelegate;
+class NetEventLogger;
 class SafeBrowsingUrlCheckerImpl;
+class UrlCheckerDelegate;
 
 // BrowserURLLoaderThrottle is used in the browser process to query
 // SafeBrowsing to determine whether a URL and also its redirect URLs are safe
 // to load. It defers response processing until all URL checks are completed;
 // cancels the load if any URLs turn out to be bad.
-// Used when --enable-network-service is in effect.
 class BrowserURLLoaderThrottle : public content::URLLoaderThrottle {
  public:
   static std::unique_ptr<BrowserURLLoaderThrottle> MaybeCreate(
@@ -42,6 +42,8 @@ class BrowserURLLoaderThrottle : public content::URLLoaderThrottle {
                            bool* defer) override;
   void WillProcessResponse(bool* defer) override;
 
+  void set_net_event_logger(NetEventLogger* net_event_logger);
+
  private:
   // |web_contents_getter| is used for displaying SafeBrowsing UI when
   // necessary.
@@ -65,6 +67,8 @@ class BrowserURLLoaderThrottle : public content::URLLoaderThrottle {
   base::TimeTicks defer_start_time_;
   bool deferred_ = false;
 
+  NetEventLogger* net_event_logger_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(BrowserURLLoaderThrottle);
 };
 

components/safe_browsing/browser/safe_browsing_url_checker_impl.cc

@@ -7,12 +7,14 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/trace_event/trace_event.h"
 #include "components/safe_browsing/browser/url_checker_delegate.h"
+#include "components/safe_browsing/net_event_logger.h"
 #include "components/safe_browsing/web_ui/constants.h"
 #include "components/security_interstitials/content/unsafe_resource.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_request_headers.h"
+#include "net/log/net_log_event_type.h"
 
 namespace safe_browsing {
 namespace {
@@ -53,8 +55,14 @@ SafeBrowsingUrlCheckerImpl::SafeBrowsingUrlCheckerImpl(
 SafeBrowsingUrlCheckerImpl::~SafeBrowsingUrlCheckerImpl() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  if (state_ == STATE_CHECKING_URL)
+  if (state_ == STATE_CHECKING_URL) {
     database_manager_->CancelCheck(this);
+    if (net_event_logger_) {
+      net_event_logger_->EndNetLogEvent(
+          net::NetLogEventType::SAFE_BROWSING_CHECKING_URL, "result",
+          "request_canceled");
+    }
+  }
 }
 
 void SafeBrowsingUrlCheckerImpl::CheckUrl(const GURL& url,
@@ -68,6 +76,11 @@ void SafeBrowsingUrlCheckerImpl::CheckUrl(const GURL& url,
   ProcessUrls();
 }
 
+const GURL& SafeBrowsingUrlCheckerImpl::GetCurrentlyCheckingUrl() const {
+  return next_index_ < urls_.size() ? urls_[next_index_].url
+                                    : GURL::EmptyGURL();
+}
+
 void SafeBrowsingUrlCheckerImpl::OnCheckBrowseUrlResult(
     const GURL& url,
     SBThreatType threat_type,
@@ -77,6 +90,13 @@ void SafeBrowsingUrlCheckerImpl::OnCheckBrowseUrlResult(
   DCHECK_EQ(urls_[next_index_].url, url);
 
   timer_.Stop();
+
+  if (net_event_logger_) {
+    net_event_logger_->EndNetLogEvent(
+        net::NetLogEventType::SAFE_BROWSING_CHECKING_URL, "result",
+        threat_type == SB_THREAT_TYPE_SAFE ? "safe" : "unsafe");
+  }
+
   if (threat_type == SB_THREAT_TYPE_SAFE) {
     state_ = STATE_NONE;
 
@@ -185,6 +205,12 @@ void SafeBrowsingUrlCheckerImpl::ProcessUrls() {
     SBThreatType threat_type = CheckWebUIUrls(url);
     if (threat_type != safe_browsing::SB_THREAT_TYPE_SAFE) {
       state_ = STATE_CHECKING_URL;
+      if (net_event_logger_) {
+        net_event_logger_->BeginNetLogEvent(
+            net::NetLogEventType::SAFE_BROWSING_CHECKING_URL, url, nullptr,
+            nullptr);
+      }
+
       content::BrowserThread::PostTask(
           content::BrowserThread::IO, FROM_HERE,
           base::Bind(&SafeBrowsingUrlCheckerImpl::OnCheckBrowseUrlResult,
@@ -202,6 +228,12 @@ void SafeBrowsingUrlCheckerImpl::ProcessUrls() {
     }
 
     state_ = STATE_CHECKING_URL;
+    if (net_event_logger_) {
+      net_event_logger_->BeginNetLogEvent(
+          net::NetLogEventType::SAFE_BROWSING_CHECKING_URL, url, nullptr,
+          nullptr);
+    }
+
     // Start a timer to abort the check if it takes too long.
     timer_.Start(FROM_HERE,
                  base::TimeDelta::FromMilliseconds(kCheckUrlTimeoutMs), this,

components/safe_browsing/browser/safe_browsing_url_checker_impl.h

@@ -22,6 +22,7 @@ class WebContents;
 
 namespace safe_browsing {
 
+class NetEventLogger;
 class UrlCheckerDelegate;
 
 // A SafeBrowsingUrlCheckerImpl instance is used to perform SafeBrowsing check
@@ -29,7 +30,6 @@ class UrlCheckerDelegate;
 // be used to handle queries from renderers. But it is also used to handle
 // queries from the browser. In that case, the public methods are called
 // directly instead of through Mojo.
-// Used when --enable-network-service is in effect.
 class SafeBrowsingUrlCheckerImpl : public mojom::SafeBrowsingUrlChecker,
                                    public SafeBrowsingDatabaseManager::Client {
  public:
@@ -50,6 +50,12 @@ class SafeBrowsingUrlCheckerImpl : public mojom::SafeBrowsingUrlChecker,
                 const std::string& method,
                 CheckUrlCallback callback) override;
 
+  const GURL& GetCurrentlyCheckingUrl() const;
+
+  void set_net_event_logger(NetEventLogger* net_event_logger) {
+    net_event_logger_ = net_event_logger;
+  }
+
  private:
   // SafeBrowsingDatabaseManager::Client implementation:
   void OnCheckBrowseUrlResult(const GURL& url,
@@ -116,6 +122,8 @@ class SafeBrowsingUrlCheckerImpl : public mojom::SafeBrowsingUrlChecker,
   // Timer to abort the SafeBrowsing check if it takes too long.
   base::OneShotTimer timer_;
 
+  NetEventLogger* net_event_logger_ = nullptr;
+
   base::WeakPtrFactory<SafeBrowsingUrlCheckerImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingUrlCheckerImpl);

components/safe_browsing/features.cc

@@ -30,6 +30,15 @@ const base::Feature kLocalDatabaseManagerEnabled{
     "SafeBrowsingV4LocalDatabaseManagerEnabled",
     base::FEATURE_DISABLED_BY_DEFAULT};
 
+// If enabled, SafeBrowsing URL checks don't defer starting requests or
+// following redirects, no matter on desktop or mobile. Instead they only defer
+// response processing.
+// Please note that when --enable-features=NetworkService is in effect,
+// SafeBrowsing URL checks never block starting requests or following redirects.
+// S13nSafeBrowsingParallelUrlCheck is ignored in that case.
+const base::Feature kParallelUrlCheck{"S13nSafeBrowsingParallelUrlCheck",
+                                      base::FEATURE_DISABLED_BY_DEFAULT};
+
 const base::Feature kPasswordFieldOnFocusPinging{
     "PasswordFieldOnFocusPinging", base::FEATURE_ENABLED_BY_DEFAULT};
 
@@ -60,6 +69,7 @@ constexpr struct {
     {&kAdSamplerTriggerFeature, false},
     {&kGoogleBrandedPhishingWarning, false},
     {&kLocalDatabaseManagerEnabled, true},
+    {&kParallelUrlCheck, true},
     {&kPasswordFieldOnFocusPinging, true},
     {&kPasswordProtectionInterstitial, false},
     {&kProtectedPasswordEntryPinging, true},

components/safe_browsing/features.h

@@ -22,6 +22,7 @@ namespace safe_browsing {
 extern const base::Feature kAdSamplerTriggerFeature;
 extern const base::Feature kGoogleBrandedPhishingWarning;
 extern const base::Feature kLocalDatabaseManagerEnabled;
+extern const base::Feature kParallelUrlCheck;
 extern const base::Feature kPasswordFieldOnFocusPinging;
 extern const base::Feature kPasswordProtectionInterstitial;
 extern const base::Feature kProtectedPasswordEntryPinging;

components/safe_browsing/net_event_logger.cc

+// 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/safe_browsing/net_event_logger.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/values.h"
+#include "net/log/net_log_capture_mode.h"
+#include "net/log/net_log_source.h"
+#include "net/log/net_log_source_type.h"
+#include "url/gurl.h"
+
+namespace safe_browsing {
+namespace {
+
+// Return a dictionary with "url"=|url-spec| and optionally
+// |name|=|value| (if not null), for netlogging.
+// This will also add a reference to the original request's net_log ID.
+std::unique_ptr<base::Value> NetLogUrlCallback(
+    const net::NetLogWithSource* net_log,
+    const GURL& url,
+    const char* name,
+    const char* value,
+    net::NetLogCaptureMode /* capture_mode */) {
+  std::unique_ptr<base::DictionaryValue> event_params(
+      new base::DictionaryValue());
+  event_params->SetString("url", url.spec());
+  if (name && value)
+    event_params->SetString(name, value);
+  net_log->source().AddToEventParameters(event_params.get());
+  return std::move(event_params);
+}
+
+// Return a dictionary with |name|=|value|, for netlogging.
+std::unique_ptr<base::Value> NetLogStringCallback(const char* name,
+                                                  const char* value,
+                                                  net::NetLogCaptureMode) {
+  std::unique_ptr<base::DictionaryValue> event_params(
+      new base::DictionaryValue());
+  if (name && value)
+    event_params->SetString(name, value);
+  return std::move(event_params);
+}
+
+}  // namespace
+
+NetEventLogger::NetEventLogger(const net::NetLogWithSource* net_log)
+    : net_log_(net_log),
+      net_log_with_sb_source_(
+          net::NetLogWithSource::Make(net_log_->net_log(),
+                                      net::NetLogSourceType::SAFE_BROWSING)) {}
+
+void NetEventLogger::BeginNetLogEvent(net::NetLogEventType type,
+                                      const GURL& url,
+                                      const char* name,
+                                      const char* value) {
+  net_log_with_sb_source_.BeginEvent(
+      type, base::Bind(&NetLogUrlCallback, net_log_, url, name, value));
+  net_log_->AddEvent(
+      type, net_log_with_sb_source_.source().ToEventParametersCallback());
+}
+
+void NetEventLogger::EndNetLogEvent(net::NetLogEventType type,
+                                    const char* name,
+                                    const char* value) {
+  net_log_with_sb_source_.EndEvent(
+      type, base::Bind(&NetLogStringCallback, name, value));
+  net_log_->AddEvent(
+      type, net_log_with_sb_source_.source().ToEventParametersCallback());
+}
+
+}  // namespace safe_browsing

components/safe_browsing/net_event_logger.h

+// 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.
+
+#ifndef COMPONENTS_SAFE_BROWSING_NET_EVENT_LOGGER_H_
+#define COMPONENTS_SAFE_BROWSING_NET_EVENT_LOGGER_H_
+
+#include "base/macros.h"
+#include "net/log/net_log_event_type.h"
+#include "net/log/net_log_with_source.h"
+
+class GURL;
+
+namespace safe_browsing {
+
+// A helper class to log network events for SafeBrowsing.
+class NetEventLogger {
+ public:
+  // |net_log| must outlive this class.
+  explicit NetEventLogger(const net::NetLogWithSource* net_log);
+
+  // For marking network events.  |name| and |value| can be null.
+  void BeginNetLogEvent(net::NetLogEventType type,
+                        const GURL& url,
+                        const char* name,
+                        const char* value);
+  void EndNetLogEvent(net::NetLogEventType type,
+                      const char* name,
+                      const char* value);
+
+ private:
+  const net::NetLogWithSource* net_log_;
+  net::NetLogWithSource net_log_with_sb_source_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetEventLogger);
+};
+
+}  // namespace safe_browsing
+
+#endif  // COMPONENTS_SAFE_BROWSING_NET_EVENT_LOGGER_H_