[ABANDONED] [WIP] Refactor SafeBrowsingResourceThrottle in preparation for WebSocket

components/safe_browsing/request_checker.cc

Index: components/safe_browsing/request_checker.cc
diff --git a/components/safe_browsing/base_resource_throttle.cc b/components/safe_browsing/request_checker.cc
similarity index 59%
rename from components/safe_browsing/base_resource_throttle.cc
rename to components/safe_browsing/request_checker.cc
index 87a8812e2bf04cc7dc711e6263edb30d40f1a8d0..544e471103f3059b32c19990677a6078133d9e35 100644
--- a/components/safe_browsing/base_resource_throttle.cc
+++ b/components/safe_browsing/request_checker.cc
@@ -1,18 +1,20 @@
-// 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/safe_browsing/base_resource_throttle.h"
+#include "components/safe_browsing/request_checker.h"
+
+#include <memory>
+#include <utility>
 
 #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_db/database_manager.h"
 #include "components/safe_browsing_db/util.h"
 #include "components/security_interstitials/content/unsafe_resource.h"
 #include "content/public/browser/browser_thread.h"
-#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"
@@ -30,7 +32,8 @@ 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 were safe.
-const int kCheckUrlTimeoutMs = 5000;
+// Mutable for testing purposes.
+int g_check_url_timeout_ms = 5000;
 
 // Return a dictionary with "url"=|url-spec| and optionally
 // |name|=|value| (if not null), for netlogging.
@@ -63,39 +66,26 @@ std::unique_ptr<base::Value> NetLogStringCallback(const char* name,
 
 }  // namespace
 
-// TODO(eroman): Downgrade these CHECK()s to DCHECKs once there is more
-//               unit test coverage.
-
-BaseResourceThrottle::BaseResourceThrottle(
+RequestChecker::RequestChecker(
     const net::URLRequest* request,
     content::ResourceType resource_type,
     scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
-    scoped_refptr<BaseUIManager> ui_manager)
-    : ui_manager_(ui_manager),
-      threat_type_(SB_THREAT_TYPE_SAFE),
-      database_manager_(database_manager),
-      request_(request),
-      state_(STATE_NONE),
+    scoped_refptr<BaseUIManager> ui_manager,
+    Delegate* delegate)
+    : state_(STATE_NONE),
       defer_state_(DEFERRED_NONE),
       resource_type_(resource_type),
+      threat_type_(SB_THREAT_TYPE_SAFE),
+      request_(request),
+      ui_manager_(std::move(ui_manager)),
+      database_manager_(std::move(database_manager)),
       net_log_with_source_(
           net::NetLogWithSource::Make(request->net_log().net_log(),
-                                      NetLogSourceType::SAFE_BROWSING)) {}
-
-// static
-BaseResourceThrottle* BaseResourceThrottle::MaybeCreate(
-    net::URLRequest* request,
-    content::ResourceType resource_type,
-    scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
-    scoped_refptr<BaseUIManager> ui_manager) {
-  if (database_manager->IsSupported()) {
-    return new BaseResourceThrottle(request, resource_type,
-                                    database_manager, ui_manager);
-  }
-  return nullptr;
-}
+                                      NetLogSourceType::SAFE_BROWSING)),
+      delegate_(std::move(delegate)),
+      weak_factory_(this) {}
 
-BaseResourceThrottle::~BaseResourceThrottle() {
+RequestChecker::~RequestChecker() {
   if (defer_state_ != DEFERRED_NONE) {
     EndNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, nullptr, nullptr);
   }
@@ -107,130 +97,84 @@ BaseResourceThrottle::~BaseResourceThrottle() {
   }
 }
 
-// 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) {
+RequestChecker::DeferDecision RequestChecker::CheckNewRequest() {
   // We need to check the new URL before starting the request.
-  if (CheckUrl(request_->url()))
-    return;
+  DeferDecision decision = CheckUrl(request_->url());
+  DCHECK_NE(decision, PROCEED_ALWAYS_ASYNC);
+  if (decision != DEFER)
+    return decision;
 
   // We let the check run in parallel with resource load only if this
   // db_manager only supports asynchronous checks, like on mobile.
   // Otherwise, we defer now.
   if (database_manager_->ChecksAreAlwaysAsync())
-    return;
+    return PROCEED_ALWAYS_ASYNC;
 
   // If the URL couldn't be verified synchronously, defer starting the
   // request until the check has completed.
   defer_state_ = DEFERRED_START;
   defer_start_time_ = base::TimeTicks::Now();
-  *defer = true;
   BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, request_->url(),
                    "defer_reason", "at_start");
+  return DEFER;
 }
 
-void BaseResourceThrottle::WillProcessResponse(bool* defer) {
-  CHECK_EQ(defer_state_, DEFERRED_NONE);
-  // TODO(nparker): Maybe remove this check, since it should have no effect.
-  if (!database_manager_->ChecksAreAlwaysAsync())
-    return;
-
-  if (state_ == STATE_CHECKING_URL ||
-      state_ == STATE_DISPLAYING_BLOCKING_PAGE) {
-    defer_state_ = DEFERRED_PROCESSING;
-    defer_start_time_ = base::TimeTicks::Now();
-    *defer = true;
-    BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, request_->url(),
-                     "defer_reason", "at_response");
-  }
-}
-
-bool BaseResourceThrottle::MustProcessResponseBeforeReadingBody() {
-  // On Android, SafeBrowsing may only decide to cancel the request when the
-  // response has been received. Therefore, no part of it should be cached
-  // until this ResourceThrottle has been able to check the response. This
-  // prevents the following scenario:
-  //   1) A request is made for foo.com which has been hacked.
-  //   2) The request is only canceled at WillProcessResponse stage, but part of
-  //   it has been cached.
-  //   3) foo.com is no longer hacked and removed from the SafeBrowsing list.
-  //   4) The user requests foo.com, which is not on the SafeBrowsing list. This
-  //   is deemed safe. However, the resource is actually served from cache,
-  //   using the version that was previously stored.
-  //   5) This results in the  user accessing an unsafe resource without being
-  //   notified that it's dangerous.
-  // TODO(clamy): Add a browser test that checks this specific scenario.
-  return true;
-}
-
-void BaseResourceThrottle::WillRedirectRequest(
-    const net::RedirectInfo& redirect_info,
-    bool* defer) {
+RequestChecker::DeferDecision RequestChecker::CheckRedirect(
+    const GURL& new_url) {
   CHECK_EQ(defer_state_, DEFERRED_NONE);
 
   // Prev check completed and was safe.
   if (state_ == STATE_NONE) {
     // Save the redirect urls for possible malware detail reporting later.
-    redirect_urls_.push_back(redirect_info.new_url);
+    redirect_urls_.push_back(new_url);
 
     // We need to check the new URL before following the redirect.
-    if (CheckUrl(redirect_info.new_url))
-      return;
+    DeferDecision decision = CheckUrl(new_url);
+    if (decision == PROCEED_SAFE)
+      return PROCEED_SAFE;
+    DCHECK_EQ(decision, DEFER);
     defer_state_ = DEFERRED_REDIRECT;
   } else {
-    CHECK(state_ == STATE_CHECKING_URL ||
-          state_ == STATE_DISPLAYING_BLOCKING_PAGE);
+    DCHECK(state_ == STATE_CHECKING_URL ||
+           state_ == STATE_DISPLAYING_BLOCKING_PAGE);
+    DCHECK(database_manager_->ChecksAreAlwaysAsync());
     // We can't check this new URL until we have finished checking
     // the prev one, or resumed from the blocking page.
-    unchecked_redirect_url_ = redirect_info.new_url;
+    unchecked_redirect_url_ = new_url;
     defer_state_ = DEFERRED_UNCHECKED_REDIRECT;
   }
 
   defer_start_time_ = base::TimeTicks::Now();
-  *defer = true;
   BeginNetLogEvent(
-      NetLogEventType::SAFE_BROWSING_DEFERRED, redirect_info.new_url,
-      "defer_reason",
+      NetLogEventType::SAFE_BROWSING_DEFERRED, new_url, "defer_reason",
       defer_state_ == DEFERRED_REDIRECT ? "redirect" : "unchecked_redirect");
+  return DEFER;
 }
 
-const char* BaseResourceThrottle::GetNameForLogging() const {
-  return "BaseResourceThrottle";
-}
+RequestChecker::DeferDecision RequestChecker::ShouldDeferResponse() {
+  CHECK_EQ(defer_state_, DEFERRED_NONE);
+  if (state_ == STATE_CHECKING_URL ||
+      state_ == STATE_DISPLAYING_BLOCKING_PAGE) {
+    DCHECK(database_manager_->ChecksAreAlwaysAsync());
+    defer_state_ = DEFERRED_PROCESSING;
+    defer_start_time_ = base::TimeTicks::Now();
+    BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, request_->url(),
+                     "defer_reason", "at_response");
+    return DEFER;
+  }
 
-void BaseResourceThrottle::MaybeDestroyPrerenderContents(
-    const content::ResourceRequestInfo* info) {}
+  return PROCEED_SAFE;
+}
 
 // SafeBrowsingService::Client implementation, called on the IO thread once
 // the URL has been classified.
-void BaseResourceThrottle::OnCheckBrowseUrlResult(
-    const GURL& url,
-    SBThreatType threat_type,
-    const ThreatMetadata& metadata) {
-  CHECK_EQ(state_, STATE_CHECKING_URL);
-  CHECK(url.is_valid());
-  CHECK(url_being_checked_.is_valid());
-  CHECK_EQ(url, url_being_checked_);
+void RequestChecker::OnCheckBrowseUrlResult(const GURL& url,
+                                            SBThreatType threat_type,
+                                            const ThreatMetadata& metadata) {
+  DCHECK_EQ(state_, STATE_CHECKING_URL);
+  DCHECK(url.is_valid());
+  DCHECK(url_being_checked_.is_valid());
+  DCHECK_EQ(url, url_being_checked_);
 
   timer_.Stop();  // Cancel the timeout timer.
   threat_type_ = threat_type;
@@ -239,9 +183,8 @@ void BaseResourceThrottle::OnCheckBrowseUrlResult(
   if (defer_state_ != DEFERRED_NONE) {
     EndNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, nullptr, nullptr);
   }
-  EndNetLogEvent(
-      NetLogEventType::SAFE_BROWSING_CHECKING_URL, "result",
-      threat_type_ == SB_THREAT_TYPE_SAFE ? "safe" : "unsafe");
+  EndNetLogEvent(NetLogEventType::SAFE_BROWSING_CHECKING_URL, "result",
+                 threat_type_ == SB_THREAT_TYPE_SAFE ? "safe" : "unsafe");
 
   if (threat_type == SB_THREAT_TYPE_SAFE) {
     if (defer_state_ != DEFERRED_NONE) {
@@ -254,16 +197,13 @@ void BaseResourceThrottle::OnCheckBrowseUrlResult(
     return;
   }
 
-  const content::ResourceRequestInfo* info =
-      content::ResourceRequestInfo::ForRequest(request_);
-
   if (request_->load_flags() & net::LOAD_PREFETCH) {
-    // Destroy the prefetch with FINAL_STATUS_SAFEBROSWING.
+    // Destroy the prefetch with FINAL_STATUS_SAFEBROWSING.
     if (resource_type_ == content::RESOURCE_TYPE_MAIN_FRAME) {
-      MaybeDestroyPrerenderContents(info);
+      delegate_->MaybeDestroyPrerenderContents(request_);
     }
     // Don't prefetch resources that fail safe browsing, disallow them.
-    Cancel();
+    delegate_->CancelResourceLoad();
     UMA_HISTOGRAM_ENUMERATION("SB2.ResourceTypes2.UnsafePrefetchCanceled",
                               resource_type_, content::RESOURCE_TYPE_LAST_TYPE);
     return;
@@ -280,44 +220,20 @@ void BaseResourceThrottle::OnCheckBrowseUrlResult(
   resource.is_subframe = resource_type_ == content::RESOURCE_TYPE_SUB_FRAME;
   resource.threat_type = threat_type;
   resource.threat_metadata = metadata;
-  resource.callback = base::Bind(
-      &BaseResourceThrottle::OnBlockingPageComplete, AsWeakPtr());
+  resource.callback = base::Bind(&RequestChecker::OnBlockingPageComplete,
+                                 weak_factory_.GetWeakPtr());
   resource.callback_thread = content::BrowserThread::GetTaskRunnerForThread(
       content::BrowserThread::IO);
-  resource.web_contents_getter = info->GetWebContentsGetterForRequest();
+  resource.web_contents_getter =
+      delegate_->GetWebContentsGetterForRequest(request_);
   resource.threat_source = database_manager_->GetThreatSource();
 
   state_ = STATE_DISPLAYING_BLOCKING_PAGE;
 
-  StartDisplayingBlockingPageHelper(resource);
+  delegate_->StartDisplayingBlockingPage(std::move(resource), ui_manager_);
 }
 
-void BaseResourceThrottle::StartDisplayingBlockingPageHelper(
-    security_interstitials::UnsafeResource resource) {
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&BaseResourceThrottle::StartDisplayingBlockingPage,
-                 AsWeakPtr(), ui_manager_, resource));
-}
-
-// Static
-void BaseResourceThrottle::StartDisplayingBlockingPage(
-    const base::WeakPtr<BaseResourceThrottle>& throttle,
-    scoped_refptr<BaseUIManager> ui_manager,
-    const security_interstitials::UnsafeResource& resource) {
-  content::WebContents* web_contents = resource.web_contents_getter.Run();
-  if (web_contents) {
-    ui_manager->DisplayBlockingPage(resource);
-    return;
-  }
-
-  // Tab is gone or it's being prerendered.
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::Bind(&BaseResourceThrottle::Cancel, throttle));
-}
-
-void BaseResourceThrottle::OnBlockingPageComplete(bool proceed) {
+void RequestChecker::OnBlockingPageComplete(bool proceed) {
   CHECK_EQ(state_, STATE_DISPLAYING_BLOCKING_PAGE);
   state_ = STATE_NONE;
 
@@ -327,21 +243,12 @@ void BaseResourceThrottle::OnBlockingPageComplete(bool proceed) {
       ResumeRequest();
     }
   } else {
-    CancelResourceLoad();
+    delegate_->CancelResourceLoad();
   }
 }
 
-void BaseResourceThrottle::CancelResourceLoad() {
-  Cancel();
-}
-
-scoped_refptr<BaseUIManager> BaseResourceThrottle::ui_manager() {
-  return ui_manager_;
-}
-
-bool BaseResourceThrottle::CheckUrl(const GURL& url) {
-  TRACE_EVENT1("loader", "BaseResourceThrottle::CheckUrl", "url",
-               url.spec());
+RequestChecker::DeferDecision RequestChecker::CheckUrl(const GURL& url) {
+  TRACE_EVENT1("loader", "RequestChecker::CheckUrl", "url", url.spec());
   CHECK_EQ(state_, STATE_NONE);
   // To reduce aggregate latency on mobile, check only the most dangerous
   // resource types.
@@ -350,7 +257,7 @@ bool BaseResourceThrottle::CheckUrl(const GURL& url) {
     // to be consistent with the other PVer4 metrics.
     UMA_HISTOGRAM_ENUMERATION("SB2.ResourceTypes2.Skipped", resource_type_,
                               content::RESOURCE_TYPE_LAST_TYPE);
-    return true;
+    return PROCEED_SKIPPED;
   }
 
   // TODO(vakh): Consider changing this metric to SafeBrowsing.V4ResourceType to
@@ -361,7 +268,7 @@ bool BaseResourceThrottle::CheckUrl(const GURL& url) {
   if (database_manager_->CheckBrowseUrl(url, this)) {
     threat_type_ = SB_THREAT_TYPE_SAFE;
     ui_manager_->LogPauseDelay(base::TimeDelta());  // No delay.
-    return true;
+    return PROCEED_SAFE;
   }
 
   state_ = STATE_CHECKING_URL;
@@ -372,13 +279,14 @@ bool BaseResourceThrottle::CheckUrl(const GURL& url) {
   // Start a timer to abort the check if it takes too long.
   // TODO(nparker): Set this only when we defer, based on remaining time,
   // so we don't cancel earlier than necessary.
-  timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(kCheckUrlTimeoutMs),
-               this, &BaseResourceThrottle::OnCheckUrlTimeout);
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromMilliseconds(g_check_url_timeout_ms), this,
+               &RequestChecker::OnCheckUrlTimeout);
 
-  return false;
+  return DEFER;
 }
 
-void BaseResourceThrottle::OnCheckUrlTimeout() {
+void RequestChecker::OnCheckUrlTimeout() {
   CHECK_EQ(state_, STATE_CHECKING_URL);
 
   database_manager_->CancelCheck(this);
@@ -387,28 +295,68 @@ void BaseResourceThrottle::OnCheckUrlTimeout() {
                          ThreatMetadata());
 }
 
-void BaseResourceThrottle::ResumeRequest() {
+void RequestChecker::ResumeRequest() {
   CHECK_EQ(state_, STATE_NONE);
   CHECK_NE(defer_state_, DEFERRED_NONE);
 
   bool resume = true;
   if (defer_state_ == DEFERRED_UNCHECKED_REDIRECT) {
+    DCHECK(unchecked_redirect_url_.is_valid());
+    GURL unchecked_redirect_url;
+    unchecked_redirect_url_.Swap(&unchecked_redirect_url);
     // Save the redirect urls for possible malware detail reporting later.
-    redirect_urls_.push_back(unchecked_redirect_url_);
-    if (!CheckUrl(unchecked_redirect_url_)) {
-      // We're now waiting for the unchecked_redirect_url_.
+    redirect_urls_.push_back(unchecked_redirect_url);
+    if (!CheckUrl(unchecked_redirect_url)) {
+      // 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",
+                       unchecked_redirect_url, "defer_reason",
                        "resumed_redirect");
     }
   }
 
   if (resume) {
     defer_state_ = DEFERRED_NONE;
-    Resume();
+    delegate_->ResumeResourceRequest();
   }
 }
 
+// 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 RequestChecker::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 RequestChecker::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());
+}
+
+ScopedTimeoutForTesting::ScopedTimeoutForTesting(int new_timeout_ms)
+    : original_timeout_ms_(g_check_url_timeout_ms) {
+  g_check_url_timeout_ms = new_timeout_ms;
+}
+
+ScopedTimeoutForTesting::ScopedTimeoutForTesting(ScopedTimeoutForTesting&& rhs)
+    : original_timeout_ms_(rhs.original_timeout_ms_) {
+  rhs.original_timeout_ms_ = -1;
+}
+
+ScopedTimeoutForTesting::~ScopedTimeoutForTesting() {
+  if (original_timeout_ms_ != -1)
+    g_check_url_timeout_ms = original_timeout_ms_;
+}
+
 }  // namespace safe_browsing