Rework SafeBrowsingResourceHandler.

Most notably, don't start the request until the URL has been verified.

The previous behavior was to overlap the retrieval of the request's headers with the URL check.
This meant that cookies from blocked pages got applied, and also that the renderer received the headers for blocked pages, and other awkwardness.

Blocking before the request has started also has the advantage of protecting against malware URLs that might exploit bugs in the HTTP stack itself (as the request is never started).

In terms of performance, overlapping had the benefit that the request gets a head start while the URL is being verified.
In practice I don't think this is actually significant, since we rely on low bloom filter false positives to avoid these extended checks in the first place. Hence optimizing for the uncommon case of extended checks isn't fruitful, especially when it comes at the cost of complexity.

I don't have unit-tests for this yet since there wasn't an existing framework to put them in (apparantly there are no safe browsing unit tests for ResourceDispatcherHost?).
I will follow up with another CL that does the necessary surgery to add such tests in resource_dispatcher_host_unittest.cc.

BUG=33572,36046
TEST=see bugs.
Review URL: http://codereview.chromium.org/661072

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@40184 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 331 insertions, 151 deletions

diff --git a/chrome/browser/renderer_host/resource_dispatcher_host.cc b/chrome/browser/renderer_host/resource_dispatcher_host.cc
index 9a245c7..d7985d7 100644
--- a/chrome/browser/renderer_host/resource_dispatcher_host.cc
+++ b/chrome/browser/renderer_host/resource_dispatcher_host.cc
@@ -435,21 +435,22 @@ void ResourceDispatcherHost::BeginRequest(
                                            this);
   }
 
+  // Insert a buffered event handler before the actual one.
+  handler = new BufferedResourceHandler(handler, this, request);
+
+  // Insert safe browsing at the front of the chain, so it gets to decide
+  // on policies first.
   if (safe_browsing_->enabled() &&
       safe_browsing_->CanCheckUrl(request_data.url)) {
     handler = new SafeBrowsingResourceHandler(handler,
                                               child_id,
                                               route_id,
-                                              request_data.url,
                                               request_data.resource_type,
                                               safe_browsing_,
                                               this,
                                               receiver_);
   }
 
-  // Insert a buffered event handler before the actual one.
-  handler = new BufferedResourceHandler(handler, this, request);
-
   // Make extra info and read footer (contains request ID).
   ResourceDispatcherHostRequestInfo* extra_info =
       new ResourceDispatcherHostRequestInfo(
@@ -605,7 +606,6 @@ void ResourceDispatcherHost::BeginDownload(
     handler = new SafeBrowsingResourceHandler(handler,
                                               child_id,
                                               route_id,
-                                              url,
                                               ResourceType::MAIN_FRAME,
                                               safe_browsing_,
                                               this,
@@ -728,6 +728,24 @@ void ResourceDispatcherHost::FollowDeferredRedirect(
   i->second->FollowDeferredRedirect();
 }
 
+void ResourceDispatcherHost::StartDeferredRequest(int process_unique_id,
+                                                  int request_id) {
+  GlobalRequestID global_id(process_unique_id, request_id);
+  PendingRequestList::iterator i = pending_requests_.find(global_id);
+  if (i == pending_requests_.end()) {
+    // The request may have been destroyed
+    LOG(WARNING) << "Trying to resume a non-existent request ("
+                 << process_unique_id << ", " << request_id << ")";
+    return;
+  }
+
+  // TODO(eroman): are there other considerations for paused or blocked
+  //               requests?
+
+  URLRequest* request = i->second;
+  InsertIntoResourceQueue(request, *InfoForRequest(request));
+}
+
 bool ResourceDispatcherHost::WillSendData(int child_id,
                                           int request_id) {
   PendingRequestList::iterator i = pending_requests_.find(
@@ -1187,7 +1205,35 @@ void ResourceDispatcherHost::BeginRequestInternal(URLRequest* request) {
 
   GlobalRequestID global_id(info->child_id(), info->request_id());
   pending_requests_[global_id] = request;
-  resource_queue_.AddRequest(request, *info);
+
+  // Give the resource handlers an opportunity to delay the URLRequest from
+  // being started.
+  //
+  // There are three cases:
+  //
+  //   (1) if OnWillStart() returns false, the request is cancelled (regardless
+  //       of whether |defer_start| was set).
+  //   (2) If |defer_start| was set to true, then the request is not added
+  //       into the resource queue, and will only be started in response to
+  //       calling StartDeferredRequest().
+  //   (3) If |defer_start| is not set, then the request is inserted into
+  //       the resource_queue_ (which may pause it further, or start it).
+  bool defer_start = false;
+  if (!info->resource_handler()->OnWillStart(
+          info->request_id(), request->url(),
+          &defer_start)) {
+    CancelRequest(info->child_id(), info->request_id(), false);
+    return;
+  }
+
+  if (!defer_start)
+    InsertIntoResourceQueue(request, *info);
+}
+
+void ResourceDispatcherHost::InsertIntoResourceQueue(
+    URLRequest* request,
+    const ResourceDispatcherHostRequestInfo& request_info) {
+  resource_queue_.AddRequest(request, request_info);
 
   // Make sure we have the load state monitor running
   if (!update_load_states_timer_.IsRunning()) {
diff --git a/chrome/browser/renderer_host/resource_dispatcher_host.h b/chrome/browser/renderer_host/resource_dispatcher_host.h
index c410f3a..121265f 100644
--- a/chrome/browser/renderer_host/resource_dispatcher_host.h
+++ b/chrome/browser/renderer_host/resource_dispatcher_host.h
@@ -139,6 +139,9 @@ class ResourceDispatcherHost : public URLRequest::Delegate {
                               bool has_new_first_party_for_cookies,
                               const GURL& new_first_party_for_cookies);
 
+  // Starts a request that was deferred during ResourceHandler::OnWillStart().
+  void StartDeferredRequest(int process_unique_id, int request_id);
+
   // Returns true if it's ok to send the data. If there are already too many
   // data messages pending, it pauses the request and returns false. In this
   // case the caller should not send the data.
@@ -318,6 +321,11 @@ class ResourceDispatcherHost : public URLRequest::Delegate {
   // Helper function for regular and download requests.
   void BeginRequestInternal(URLRequest* request);
 
+  // Helper function that inserts |request| into the resource queue.
+  void InsertIntoResourceQueue(
+      URLRequest* request,
+      const ResourceDispatcherHostRequestInfo& request_info);
+
   // Updates the "cost" of outstanding requests for |process_unique_id|.
   // The "cost" approximates how many bytes are consumed by all the in-memory
   // data structures supporting this request (URLRequest object,
diff --git a/chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc b/chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc
index 38f4dd4..1edc26b 100644
--- a/chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc
+++ b/chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc
@@ -20,6 +20,9 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "webkit/appcache/appcache_interfaces.h"
 
+// TODO(eroman): Write unit tests for SafeBrowsing that exercise
+//               SafeBrowsingResourceHandler.
+
 namespace {
 
 // Returns the resource response header structure for this request.
diff --git a/chrome/browser/renderer_host/resource_handler.h b/chrome/browser/renderer_host/resource_handler.h
index 526c6a2..2d6d797 100644
--- a/chrome/browser/renderer_host/resource_handler.h
+++ b/chrome/browser/renderer_host/resource_handler.h
@@ -49,6 +49,18 @@ class ResourceHandler
   virtual bool OnResponseStarted(int request_id,
                                  ResourceResponse* response) = 0;
 
+  // Called before the URLRequest for |request_id| (whose url is |url|) is to be
+  // started. If the handler returns false, then the request is cancelled.
+  // Otherwise if the return value is true, the ResourceHandler can delay the
+  // request from starting by setting |*defer = true|. A deferred request will
+  // not have called URLRequest::Start(), and will not resume until someone
+  // calls ResourceDispatcherHost::StartDeferredRequest().
+  virtual bool OnWillStart(int request_id, const GURL& url, bool* defer) {
+    // TODO(eroman): This should be a pure virtual method (no default
+    //               implementation).
+    return true;
+  }
+
   // Data will be read for the response.  Upon success, this method places the
   // size and address of the buffer where the data is to be written in its
   // out-params.  This call will be followed by either OnReadCompleted or
diff --git a/chrome/browser/renderer_host/safe_browsing_resource_handler.cc b/chrome/browser/renderer_host/safe_browsing_resource_handler.cc
index 68301e8..06beb0c 100644
--- a/chrome/browser/renderer_host/safe_browsing_resource_handler.cc
+++ b/chrome/browser/renderer_host/safe_browsing_resource_handler.cc
@@ -11,38 +11,31 @@
 #include "net/base/net_errors.h"
 #include "net/base/io_buffer.h"
 
-// Maximum time to wait for a gethash response from the Safe Browsing servers.
-static const int kMaxGetHashMs = 1000;
+// 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.
+static const int kCheckUrlTimeoutMs = 1000;
+
+// TODO(eroman): Downgrade these CHECK()s to DCHECKs once there is more
+//               unit test coverage.
 
 SafeBrowsingResourceHandler::SafeBrowsingResourceHandler(
     ResourceHandler* handler,
     int render_process_host_id,
     int render_view_id,
-    const GURL& url,
     ResourceType::Type resource_type,
     SafeBrowsingService* safe_browsing,
     ResourceDispatcherHost* resource_dispatcher_host,
     ResourceDispatcherHost::Receiver* receiver)
-    : next_handler_(handler),
+    : state_(STATE_NONE),
+      defer_state_(DEFERRED_NONE),
+      deferred_request_id_(-1),
+      next_handler_(handler),
       render_process_host_id_(render_process_host_id),
       render_view_id_(render_view_id),
-      paused_request_id_(-1),
-      in_safe_browsing_check_(false),
-      displaying_blocking_page_(false),
       safe_browsing_(safe_browsing),
-      queued_error_request_id_(-1),
       rdh_(resource_dispatcher_host),
-      resource_type_(resource_type),
-      redirect_id_(-1) {
-  if (safe_browsing_->CheckUrl(url, this)) {
-    safe_browsing_result_ = SafeBrowsingService::URL_SAFE;
-    safe_browsing_->LogPauseDelay(base::TimeDelta());  // No delay.
-  } else {
-    AddRef();
-    in_safe_browsing_check_ = true;
-    // Can't pause now because it's too early, so we'll do it in OnWillRead.
-  }
-
+      resource_type_(resource_type) {
   registrar_.Add(this, NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN,
                  Source<ResourceMessageFilter>(
                      static_cast<ResourceMessageFilter*>(receiver)));
@@ -54,6 +47,8 @@ SafeBrowsingResourceHandler::~SafeBrowsingResourceHandler() {
 bool SafeBrowsingResourceHandler::OnUploadProgress(int request_id,
                                                    uint64 position,
                                                    uint64 size) {
+  CHECK(state_ == STATE_NONE);
+  CHECK(defer_state_ = DEFERRED_NONE);
   return next_handler_->OnUploadProgress(request_id, position, size);
 }
 
@@ -62,59 +57,64 @@ bool SafeBrowsingResourceHandler::OnRequestRedirected(
     const GURL& new_url,
     ResourceResponse* response,
     bool* defer) {
-  if (in_safe_browsing_check_) {
-    // Defer following the redirect until the SafeBrowsing check is complete.
-    // Store the redirect context so we can pass it on to other handlers once we
-    // have completed our check.
-    redirect_response_ = response;
-    redirect_url_ = new_url;
-    redirect_id_ = request_id;
-    *defer = true;
-    return true;
-  }
+  CHECK(state_ == STATE_NONE);
+  CHECK(defer_state_ == DEFERRED_NONE);
 
-  if (safe_browsing_->CheckUrl(new_url, this)) {
-    safe_browsing_result_ = SafeBrowsingService::URL_SAFE;
-    safe_browsing_->LogPauseDelay(base::TimeDelta());  // No delay.
-  } else {
-    AddRef();
-    in_safe_browsing_check_ = true;
-    // Can't pause now because it's too early, so we'll do it in OnWillRead.
+  // We need to check the new URL before following the redirect.
+  if (CheckUrl(new_url)) {
+    return next_handler_->OnRequestRedirected(
+        request_id, new_url, response, defer);
   }
 
-  return next_handler_->OnRequestRedirected(
-      request_id, new_url, response, defer);
+  // If the URL couldn't be verified synchronously, defer following the
+  // redirect until the SafeBrowsing check is complete. Store the redirect
+  // context so we can pass it on to other handlers once we have completed
+  // our check.
+  defer_state_ = DEFERRED_REDIRECT;
+  deferred_request_id_ = request_id;
+  deferred_url_ = new_url;
+  deferred_redirect_response_ = response;
+  *defer = true;
+
+  return true;
 }
 
 bool SafeBrowsingResourceHandler::OnResponseStarted(
     int request_id, ResourceResponse* response) {
+  CHECK(state_ == STATE_NONE);
+  CHECK(defer_state_ == DEFERRED_NONE);
   return next_handler_->OnResponseStarted(request_id, response);
 }
 
-void SafeBrowsingResourceHandler::OnGetHashTimeout() {
-  if (!in_safe_browsing_check_)
-    return;
-
+void SafeBrowsingResourceHandler::OnCheckUrlTimeout() {
+  CHECK(state_ == STATE_CHECKING_URL);
+  CHECK(defer_state_ != DEFERRED_NONE);
   safe_browsing_->CancelCheck(this);
   OnUrlCheckResult(GURL(), SafeBrowsingService::URL_SAFE);
 }
 
+bool SafeBrowsingResourceHandler::OnWillStart(int request_id,
+                                              const GURL& url,
+                                              bool* defer) {
+  // We need to check the new URL before starting the request.
+  if (CheckUrl(url))
+    return next_handler_->OnWillStart(request_id, url, defer);
+
+  // If the URL couldn't be verified synchronously, defer starting the
+  // request until the check has completed.
+  defer_state_ = DEFERRED_START;
+  deferred_request_id_ = request_id;
+  deferred_url_ = url;
+  *defer = true;
+
+  return true;
+}
+
 bool SafeBrowsingResourceHandler::OnWillRead(int request_id,
                                              net::IOBuffer** buf, int* buf_size,
                                              int min_size) {
-  if (in_safe_browsing_check_ && pause_time_.is_null()) {
-    pause_time_ = base::Time::Now();
-    MessageLoop::current()->PostDelayedTask(
-        FROM_HERE,
-        NewRunnableMethod(this, &SafeBrowsingResourceHandler::OnGetHashTimeout),
-        kMaxGetHashMs);
-  }
-
-  if (in_safe_browsing_check_ || displaying_blocking_page_) {
-    rdh_->PauseRequest(render_process_host_id_, request_id, true);
-    paused_request_id_ = request_id;
-  }
-
+  CHECK(state_ == STATE_NONE);
+  CHECK(defer_state_ == DEFERRED_NONE);
   bool rv = next_handler_->OnWillRead(request_id, buf, buf_size, min_size);
   // TODO(willchan): Remove after debugging bug 16371.
   if (rv)
@@ -124,25 +124,16 @@ bool SafeBrowsingResourceHandler::OnWillRead(int request_id,
 
 bool SafeBrowsingResourceHandler::OnReadCompleted(int request_id,
                                                   int* bytes_read) {
+  CHECK(state_ == STATE_NONE);
+  CHECK(defer_state_ == DEFERRED_NONE);
   return next_handler_->OnReadCompleted(request_id, bytes_read);
 }
 
 bool SafeBrowsingResourceHandler::OnResponseCompleted(
     int request_id, const URLRequestStatus& status,
     const std::string& security_info) {
-  if ((in_safe_browsing_check_ ||
-       safe_browsing_result_ != SafeBrowsingService::URL_SAFE) &&
-      status.status() == URLRequestStatus::FAILED &&
-      status.os_error() == net::ERR_NAME_NOT_RESOLVED) {
-    // Got a DNS error while the safebrowsing check is in progress or we
-    // already know that the site is unsafe.  Don't show the the dns error
-    // page.
-    queued_error_.reset(new URLRequestStatus(status));
-    queued_error_request_id_ = request_id;
-    queued_security_info_ = security_info;
-    return true;
-  }
-
+  CHECK(state_ == STATE_NONE);
+  CHECK(defer_state_ == DEFERRED_NONE);
   return next_handler_->OnResponseCompleted(request_id, status, security_info);
 }
 
@@ -150,104 +141,158 @@ bool SafeBrowsingResourceHandler::OnResponseCompleted(
 // the URL has been classified.
 void SafeBrowsingResourceHandler::OnUrlCheckResult(
     const GURL& url, SafeBrowsingService::UrlCheckResult result) {
-  DCHECK(in_safe_browsing_check_);
-  DCHECK(!displaying_blocking_page_);
+  CHECK(state_ == STATE_CHECKING_URL);
+  CHECK(defer_state_ != DEFERRED_NONE);
+  CHECK(url == deferred_url_);
 
+  timer_.Stop();  // Cancel the timeout timer.
   safe_browsing_result_ = result;
-  in_safe_browsing_check_ = false;
+  state_ = STATE_NONE;
 
   if (result == SafeBrowsingService::URL_SAFE) {
-    // Resume following any redirect response we've deferred.
-    if (redirect_id_ != -1) {
-      ResumeRedirect();
-      return;
-    }
-
-    if (paused_request_id_ != -1) {
-      rdh_->PauseRequest(render_process_host_id_, paused_request_id_, false);
-      paused_request_id_ = -1;
-    }
-
+    // Log how much time the safe browsing check cost us.
     base::TimeDelta pause_delta;
-    if (!pause_time_.is_null())
-      pause_delta = base::Time::Now() - pause_time_;
+    pause_delta = base::TimeTicks::Now() - url_check_start_time_;
     safe_browsing_->LogPauseDelay(pause_delta);
 
-    if (queued_error_.get()) {
-      next_handler_->OnResponseCompleted(
-          queued_error_request_id_, *queued_error_.get(),
-          queued_security_info_);
-      queued_error_.reset();
-      queued_security_info_.clear();
-    }
-
-    Release();
+    // Continue the request.
+    ResumeRequest();
   } else {
-    displaying_blocking_page_ = true;
-    safe_browsing_->DisplayBlockingPage(
-        url, resource_type_, result, this, render_process_host_id_,
-        render_view_id_);
+    StartDisplayingBlockingPage(url, result);
   }
+
+  Release();  // Balances the AddRef() in CheckingUrl().
+}
+
+void SafeBrowsingResourceHandler::StartDisplayingBlockingPage(
+    const GURL& url,
+    SafeBrowsingService::UrlCheckResult result) {
+  CHECK(state_ == STATE_NONE);
+  CHECK(defer_state_ != DEFERRED_NONE);
+
+  state_ = STATE_DISPLAYING_BLOCKING_PAGE;
+  AddRef();  // Balanced in OnBlockingPageComplete().
+
+  safe_browsing_->DisplayBlockingPage(
+      url, resource_type_, result, this, render_process_host_id_,
+      render_view_id_);
 }
 
 // SafeBrowsingService::Client implementation, called on the IO thread when
 // the user has decided to proceed with the current request, or go back.
 void SafeBrowsingResourceHandler::OnBlockingPageComplete(bool proceed) {
-  DCHECK(displaying_blocking_page_);
-  displaying_blocking_page_ = false;
+  CHECK(state_ == STATE_DISPLAYING_BLOCKING_PAGE);
+  state_ = STATE_NONE;
 
   if (proceed) {
-    // Resume following any deferred redirect.
-    if (redirect_id_ != -1) {
-      ResumeRedirect();
-      return;
-    }
-
     safe_browsing_result_ = SafeBrowsingService::URL_SAFE;
-    if (paused_request_id_ != -1) {
-      rdh_->PauseRequest(render_process_host_id_, paused_request_id_, false);
-      paused_request_id_ = -1;
-    }
-
-    if (queued_error_.get()) {
-      next_handler_->OnResponseCompleted(
-          queued_error_request_id_, *queued_error_.get(),
-          queued_security_info_);
-      queued_error_.reset();
-      queued_security_info_.clear();
-    }
+    ResumeRequest();
   } else {
-    rdh_->CancelRequest(render_process_host_id_, paused_request_id_, false);
+    rdh_->CancelRequest(render_process_host_id_, deferred_request_id_, false);
   }
 
-  Release();
+  Release();  // Balances the AddRef() in StartDisplayingBlockingPage().
 }
 
 void SafeBrowsingResourceHandler::Observe(NotificationType type,
                                           const NotificationSource& source,
                                           const NotificationDetails& details) {
   DCHECK(type.value == NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN);
-  if (in_safe_browsing_check_) {
+  if (state_ == STATE_CHECKING_URL) {
     safe_browsing_->CancelCheck(this);
-    in_safe_browsing_check_ = false;
+    state_ = STATE_NONE;
+    // Balance the AddRef() from CheckUrl() which would ordinarily be
+    // balanced by OnUrlCheckResult().
     Release();
   }
 }
 
+bool SafeBrowsingResourceHandler::CheckUrl(const GURL& url) {
+  CHECK(state_ == STATE_NONE);
+  bool succeeded_synchronously = safe_browsing_->CheckUrl(url, this);
+  if (succeeded_synchronously) {
+    safe_browsing_result_ = SafeBrowsingService::URL_SAFE;
+    safe_browsing_->LogPauseDelay(base::TimeDelta());  // No delay.
+    return true;
+  }
+
+  AddRef();  // Balanced in OnUrlCheckResult().
+  state_ = STATE_CHECKING_URL;
+
+  // Record the start time of the check.
+  url_check_start_time_ = base::TimeTicks::Now();
+
+  // Start a timer to abort the check if it takes too long.
+  timer_.Start(base::TimeDelta::FromMilliseconds(kCheckUrlTimeoutMs),
+               this, &SafeBrowsingResourceHandler::OnCheckUrlTimeout);
+
+  return false;
+}
+
+void SafeBrowsingResourceHandler::ResumeRequest() {
+  CHECK(state_ == STATE_NONE);
+  CHECK(defer_state_ != DEFERRED_NONE);
+
+  // Resume whatever stage got paused by the safe browsing check.
+  switch (defer_state_) {
+    case DEFERRED_START:
+      ResumeStart();
+      break;
+    case DEFERRED_REDIRECT:
+      ResumeRedirect();
+      break;
+    case DEFERRED_NONE:
+      NOTREACHED();
+      break;
+  }
+}
+
+void SafeBrowsingResourceHandler::ResumeStart() {
+  CHECK(defer_state_ == DEFERRED_START);
+  CHECK(deferred_request_id_ != -1);
+  defer_state_ = DEFERRED_NONE;
+
+  // Retrieve the details for the paused OnWillStart().
+  int request_id = deferred_request_id_;
+  GURL url = deferred_url_;
+
+  ClearDeferredRequestInfo();
+
+  // Give the other resource handlers a chance to defer starting.
+  bool defer = false;
+  // TODO(eroman): the return value is being lost here. Should
+  // use it to cancel the request.
+  next_handler_->OnWillStart(request_id, url, &defer);
+  if (!defer)
+    rdh_->StartDeferredRequest(render_process_host_id_, request_id);
+}
+
 void SafeBrowsingResourceHandler::ResumeRedirect() {
-  DCHECK(redirect_id_ != -1);
+  CHECK(defer_state_ == DEFERRED_REDIRECT);
+  defer_state_ = DEFERRED_NONE;
+
+  // Retrieve the details for the paused OnReceivedRedirect().
+  int request_id = deferred_request_id_;
+  GURL redirect_url = deferred_url_;
+  scoped_refptr<ResourceResponse> redirect_response =
+      deferred_redirect_response_;
+
+  ClearDeferredRequestInfo();
 
   // Give the other resource handlers a chance to handle the redirect.
   bool defer = false;
-  next_handler_->OnRequestRedirected(redirect_id_, redirect_url_,
-                                     redirect_response_, &defer);
+  // TODO(eroman): the return value is being lost here. Should
+  // use it to cancel the request.
+  next_handler_->OnRequestRedirected(request_id, redirect_url,
+                                     redirect_response, &defer);
   if (!defer) {
-    // TODO(wtc): should we pass a new first party for cookies URL?
-    rdh_->FollowDeferredRedirect(render_process_host_id_, redirect_id_,
+    rdh_->FollowDeferredRedirect(render_process_host_id_, request_id,
                                  false, GURL());
   }
+}
 
-  redirect_response_ = NULL;
-  redirect_id_ = -1;
-  Release();
+void SafeBrowsingResourceHandler::ClearDeferredRequestInfo() {
+  deferred_request_id_ = -1;
+  deferred_url_ = GURL();
+  deferred_redirect_response_ = NULL;
 }
diff --git a/chrome/browser/renderer_host/safe_browsing_resource_handler.h b/chrome/browser/renderer_host/safe_browsing_resource_handler.h
index f7b75be..e1105b3 100644
--- a/chrome/browser/renderer_host/safe_browsing_resource_handler.h
+++ b/chrome/browser/renderer_host/safe_browsing_resource_handler.h
@@ -9,12 +9,34 @@
 
 #include "base/ref_counted.h"
 #include "base/time.h"
+#include "base/timer.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "chrome/browser/renderer_host/resource_handler.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/common/notification_registrar.h"
 
-// Checks that a url is safe.
+// SafeBrowsingResourceHandler checks that URLs are "safe" before navigating
+// to them. To be considered "safe", a URL must not appear in the
+// malware/phishing blacklists (see SafeBrowsingService for details).
+//
+// This check is done before requesting the original URL, and additionally
+// before following any subsequent redirect.
+//
+// In the common case, the check completes synchronously (no match in the bloom
+// filter), so the request's flow is un-interrupted.
+//
+// However if the URL fails this quick check, it has the possibility of being
+// on the blacklist. Now the request is suspended (prevented from starting),
+// and a more expensive safe browsing check is begun (fetches the full hashes).
+//
+// Note that the safe browsing check takes at most kCheckUrlTimeoutMs
+// milliseconds. If it takes longer than this, then the system defaults to
+// treating the URL as safe.
+//
+// Once the safe browsing check has completed, if the URL was decided to be
+// dangerous, a warning page is thrown up and the request remains suspended.
+// If on the other hand the URL was decided to be safe, the request is
+// resumed.
 class SafeBrowsingResourceHandler : public ResourceHandler,
                                     public SafeBrowsingService::Client,
                                     public NotificationObserver {
@@ -22,7 +44,6 @@ class SafeBrowsingResourceHandler : public ResourceHandler,
   SafeBrowsingResourceHandler(ResourceHandler* handler,
                               int render_process_host_id,
                               int render_view_id,
-                              const GURL& url,
                               ResourceType::Type resource_type,
                               SafeBrowsingService* safe_browsing,
                               ResourceDispatcherHost* resource_dispatcher_host,
@@ -33,7 +54,7 @@ class SafeBrowsingResourceHandler : public ResourceHandler,
   bool OnRequestRedirected(int request_id, const GURL& new_url,
                            ResourceResponse* response, bool* defer);
   bool OnResponseStarted(int request_id, ResourceResponse* response);
-  void OnGetHashTimeout();
+  bool OnWillStart(int request_id, const GURL& url, bool* defer);
   bool OnWillRead(int request_id, net::IOBuffer** buf, int* buf_size,
                   int min_size);
   bool OnReadCompleted(int request_id, int* bytes_read);
@@ -56,30 +77,75 @@ class SafeBrowsingResourceHandler : public ResourceHandler,
                const NotificationDetails& details);
 
  private:
+  // Describes what phase of the check a handler is in.
+  enum State {
+    STATE_NONE,
+    STATE_CHECKING_URL,
+    STATE_DISPLAYING_BLOCKING_PAGE,
+  };
+
+  // Describes what stage of the request got paused by the check.
+  enum DeferState {
+    DEFERRED_NONE,
+    DEFERRED_START,
+    DEFERRED_REDIRECT,
+  };
+
   ~SafeBrowsingResourceHandler();
 
-  // Helper function for resuming the following of a redirect response.
+  // Starts running |url| through the safe browsing check. Returns true if the
+  // URL is safe to visit. Otherwise returns false and will call
+  // OnUrlCheckResult() when the check has completed.
+  bool CheckUrl(const GURL& url);
+
+  // Callback for when the safe browsing check (which was initiated by
+  // StartCheckingUrl()) has taken longer than kCheckUrlTimeoutMs.
+  void OnCheckUrlTimeout();
+
+  // Starts displaying the safe browsing interstitial page.
+  void StartDisplayingBlockingPage(const GURL& url,
+                                   SafeBrowsingService::UrlCheckResult result);
+
+  // Resumes the request, by continuing the deferred action (either starting the
+  // request, or following a redirect).
+  void ResumeRequest();
+
+  // Resumes the deferred "start".
+  void ResumeStart();
+
+  // Resumes the deferred redirect.
   void ResumeRedirect();
 
+  // Erases the state associated with a deferred "start" or redirect
+  // (i.e. the deferred URL and request ID).
+  void ClearDeferredRequestInfo();
+
+  State state_;
+  DeferState defer_state_;
+
+  // The result of the most recent safe browsing check. Only valid to read this
+  // when state_ != STATE_CHECKING_URL.
+  SafeBrowsingService::UrlCheckResult safe_browsing_result_;
+
+  // The time when the outstanding safe browsing check was started.
+  base::TimeTicks url_check_start_time_;
+
+  // Timer to abort the safe browsing check if it takes too long.
+  base::OneShotTimer<SafeBrowsingResourceHandler> timer_;
+
+  // Details on the deferred request (either a start or redirect). It is only
+  // valid to access these members when defer_state_ != DEFERRED_NONE.
+  GURL deferred_url_;
+  int deferred_request_id_;
+  scoped_refptr<ResourceResponse> deferred_redirect_response_;
+
   NotificationRegistrar registrar_;
   scoped_refptr<ResourceHandler> next_handler_;
   int render_process_host_id_;
   int render_view_id_;
-  int paused_request_id_;  // -1 if not paused
-  bool in_safe_browsing_check_;
-  bool displaying_blocking_page_;
-  SafeBrowsingService::UrlCheckResult safe_browsing_result_;
   scoped_refptr<SafeBrowsingService> safe_browsing_;
-  scoped_ptr<URLRequestStatus> queued_error_;
-  std::string queued_security_info_;
-  int queued_error_request_id_;
   ResourceDispatcherHost* rdh_;
-  base::Time pause_time_;
   ResourceType::Type resource_type_;
-  // Context for handling deferred redirects.
-  scoped_refptr<ResourceResponse> redirect_response_;
-  GURL redirect_url_;
-  int redirect_id_;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingResourceHandler);
 };