Revert "Allow extensions to redirect requests in onBeforeRequest."

The change introduced a regression in the WebRequestEvents api test.
TBR=mpcomplete

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

9 files changed, 54 insertions, 174 deletions

diff --git a/net/base/network_delegate.cc b/net/base/network_delegate.cc
index d707c44..2ed1ea5 100644
--- a/net/base/network_delegate.cc
+++ b/net/base/network_delegate.cc
@@ -9,21 +9,20 @@
 namespace net {
 
 int NetworkDelegate::NotifyBeforeURLRequest(URLRequest* request,
-                                            CompletionCallback* callback,
-                                            GURL* new_url) {
+                                            CompletionCallback* callback) {
   DCHECK(CalledOnValidThread());
   DCHECK(request);
   DCHECK(callback);
-  return OnBeforeURLRequest(request, callback, new_url);
+  return OnBeforeURLRequest(request, callback);
 }
 
 int NetworkDelegate::NotifyBeforeSendHeaders(uint64 request_id,
-                                             CompletionCallback* callback,
-                                             HttpRequestHeaders* headers) {
+                                             HttpRequestHeaders* headers,
+                                             CompletionCallback* callback) {
   DCHECK(CalledOnValidThread());
   DCHECK(headers);
   DCHECK(callback);
-  return OnBeforeSendHeaders(request_id, callback, headers);
+  return OnBeforeSendHeaders(request_id, headers, callback);
 }
 
 void NetworkDelegate::NotifyResponseStarted(URLRequest* request) {
diff --git a/net/base/network_delegate.h b/net/base/network_delegate.h
index eafb212..effc699 100644
--- a/net/base/network_delegate.h
+++ b/net/base/network_delegate.h
@@ -9,8 +9,6 @@
 #include "base/threading/non_thread_safe.h"
 #include "net/base/completion_callback.h"
 
-class GURL;
-
 namespace net {
 
 // NOTE: Layering violations!
@@ -36,11 +34,10 @@ class NetworkDelegate : public base::NonThreadSafe {
   // checking on parameters. See the corresponding virtuals for explanations of
   // the methods and their arguments.
   int NotifyBeforeURLRequest(URLRequest* request,
-                             CompletionCallback* callback,
-                             GURL* new_url);
+                             CompletionCallback* callback);
   int NotifyBeforeSendHeaders(uint64 request_id,
-                              CompletionCallback* callback,
-                              HttpRequestHeaders* headers);
+                              HttpRequestHeaders* headers,
+                              CompletionCallback* callback);
   void NotifyResponseStarted(URLRequest* request);
   void NotifyReadCompleted(URLRequest* request, int bytes_read);
   void NotifyURLRequestDestroyed(URLRequest* request);
@@ -58,22 +55,20 @@ class NetworkDelegate : public base::NonThreadSafe {
   // member functions will be called by the respective public notification
   // member function, which will perform basic sanity checking.
 
-  // Called before a request is sent. Allows the delegate to rewrite the URL
-  // being fetched by modifying |new_url|. The callback can be called at any
-  // time, but will have no effect if the request has already been cancelled or
+  // Called before a request is sent. The callback can be called at any time,
+  // but will have no effect if the request has already been cancelled or
   // deleted. Returns a net status code, generally either OK to continue with
   // the request or ERR_IO_PENDING if the result is not ready yet.
   virtual int OnBeforeURLRequest(URLRequest* request,
-                                 CompletionCallback* callback,
-                                 GURL* new_url) = 0;
+                                 CompletionCallback* callback) = 0;
 
   // Called right before the HTTP headers are sent. Allows the delegate to
   // read/write |headers| before they get sent out. The callback can be called
   // at any time, but will have no effect if the transaction handling this
   // request has been cancelled. Returns a net status code.
   virtual int OnBeforeSendHeaders(uint64 request_id,
-                                  CompletionCallback* callback,
-                                  HttpRequestHeaders* headers) = 0;
+                                  HttpRequestHeaders* headers,
+                                  CompletionCallback* callback) = 0;
 
   // This corresponds to URLRequestDelegate::OnResponseStarted.
   virtual void OnResponseStarted(URLRequest* request) = 0;
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index a12a64f..c4293d9 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -908,7 +908,7 @@ int HttpCache::Transaction::DoNotifyBeforeSendHeaders() {
     // TODO(mpcomplete): need to be able to modify these headers.
     HttpRequestHeaders headers = request_->extra_headers;
     return cache_->GetSession()->network_delegate()->NotifyBeforeSendHeaders(
-        request_->request_id, cache_callback_, &headers);
+        request_->request_id, &headers, cache_callback_);
   }
 
   return OK;
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 5651a41..f7d3acc 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -771,7 +771,7 @@ int HttpNetworkTransaction::DoBuildRequest() {
 
   if (session_->network_delegate()) {
     return session_->network_delegate()->NotifyBeforeSendHeaders(
-        request_->request_id, delegate_callback_, &request_headers_);
+        request_->request_id, &request_headers_, delegate_callback_);
   }
 
   return OK;
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index 3d60137..5c437c2 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -23,7 +23,6 @@
 #include "net/url_request/url_request_job.h"
 #include "net/url_request/url_request_job_manager.h"
 #include "net/url_request/url_request_netlog_params.h"
-#include "net/url_request/url_request_redirect_job.h"
 
 using base::Time;
 using std::string;
@@ -121,9 +120,7 @@ URLRequest::URLRequest(const GURL& url, Delegate* delegate)
       redirect_limit_(kMaxRedirects),
       final_upload_progress_(0),
       priority_(LOWEST),
-      identifier_(GenerateURLRequestIdentifier()),
-      ALLOW_THIS_IN_INITIALIZER_LIST(
-          before_request_callback_(this, &URLRequest::BeforeRequestComplete)) {
+      identifier_(GenerateURLRequestIdentifier()) {
   SIMPLE_STATS_COUNTER("URLRequestCount");
 
   // Sanity check out environment.
@@ -137,6 +134,9 @@ URLRequest::~URLRequest() {
   if (context_ && context_->network_delegate())
     context_->network_delegate()->NotifyURLRequestDestroyed(this);
 
+  if (before_request_callback_)
+    before_request_callback_->Cancel();
+
   Cancel();
 
   if (job_)
@@ -362,41 +362,22 @@ GURL URLRequest::GetSanitizedReferrer() const {
 void URLRequest::Start() {
   response_info_.request_time = Time::Now();
 
-  // Only notify the delegate for the initial request.
   if (context_ && context_->network_delegate()) {
+    before_request_callback_ = new CancelableCompletionCallback<URLRequest>(
+        this, &URLRequest::BeforeRequestComplete);
     if (context_->network_delegate()->NotifyBeforeURLRequest(
-            this, &before_request_callback_, &delegate_redirect_url_) ==
-            net::ERR_IO_PENDING) {
+            this, before_request_callback_) == net::ERR_IO_PENDING) {
+      before_request_callback_->AddRef();  // balanced in BeforeRequestComplete
       net_log_.BeginEvent(NetLog::TYPE_URL_REQUEST_BLOCKED_ON_EXTENSION, NULL);
       return;  // paused
     }
   }
 
-  StartInternal();
+  StartJob(URLRequestJobManager::GetInstance()->CreateJob(this));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
-void URLRequest::BeforeRequestComplete(int error) {
-  DCHECK(!job_);
-  DCHECK_NE(ERR_IO_PENDING, error);
-
-  net_log_.EndEvent(NetLog::TYPE_URL_REQUEST_BLOCKED_ON_EXTENSION, NULL);
-  if (error != OK) {
-    StartJob(new URLRequestErrorJob(this, error));
-  } else if (!delegate_redirect_url_.is_empty()) {
-    GURL new_url;
-    new_url.Swap(&delegate_redirect_url_);
-    StartJob(new URLRequestRedirectJob(this, new_url));
-  } else {
-    StartInternal();
-  }
-}
-
-void URLRequest::StartInternal() {
-  StartJob(URLRequestJobManager::GetInstance()->CreateJob(this));
-}
-
 void URLRequest::StartJob(URLRequestJob* job) {
   DCHECK(!is_pending_);
   DCHECK(!job_);
@@ -423,6 +404,18 @@ void URLRequest::StartJob(URLRequestJob* job) {
   job_->Start();
 }
 
+void URLRequest::BeforeRequestComplete(int error) {
+  DCHECK(!job_);
+
+  net_log_.EndEvent(NetLog::TYPE_URL_REQUEST_BLOCKED_ON_EXTENSION, NULL);
+  before_request_callback_->Release();  // balanced in Start
+  if (error != OK) {
+    StartJob(new URLRequestErrorJob(this, error));
+  } else {
+    StartJob(URLRequestJobManager::GetInstance()->CreateJob(this));
+  }
+}
+
 void URLRequest::Restart() {
   // Should only be called if the original job didn't make any progress.
   DCHECK(job_ && !job_->has_response_started());
@@ -635,7 +628,7 @@ int URLRequest::Redirect(const GURL& location, int http_status_code) {
     final_upload_progress_ = job_->GetUploadProgress();
 
   PrepareToRestart();
-  StartInternal();
+  Start();
   return OK;
 }
 
diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h
index b25b842..40f7d8a 100644
--- a/net/url_request/url_request.h
+++ b/net/url_request/url_request.h
@@ -584,15 +584,6 @@ class URLRequest : public base::NonThreadSafe {
   friend class URLRequestJob;
   typedef std::map<const void*, linked_ptr<UserData> > UserDataMap;
 
-  void StartInternal();
-
-  // Resumes or blocks a request paused by the NetworkDelegate::OnBeforeRequest
-  // handler. If |blocked| is true, the request is blocked and an error page is
-  // returned indicating so. This should only be called after Start is called
-  // and OnBeforeRequest returns true (signalling that the request should be
-  // paused).
-  void BeforeRequestComplete(int error);
-
   void StartJob(URLRequestJob* job);
 
   // Restarting involves replacing the current job with a new one such as what
@@ -609,6 +600,13 @@ class URLRequest : public base::NonThreadSafe {
   // passed values.
   void DoCancel(int os_error, const SSLInfo& ssl_info);
 
+  // Resumes or blocks a request paused by the NetworkDelegate::OnBeforeRequest
+  // handler. If |blocked| is true, the request is blocked and an error page is
+  // returned indicating so. This should only be called after Start is called
+  // and OnBeforeRequest returns true (signalling that the request should be
+  // paused).
+  void BeforeRequestComplete(int error);
+
   // Contextual information used for this request (can be NULL). This contains
   // most of the dependencies which are shared between requests (disk cache,
   // cookie store, socket pool, etc.)
@@ -622,7 +620,6 @@ class URLRequest : public base::NonThreadSafe {
   GURL url_;
   GURL original_url_;
   GURL first_party_for_cookies_;
-  GURL delegate_redirect_url_;
   std::string method_;  // "GET", "POST", etc. Should be all uppercase.
   std::string referrer_;
   HttpRequestHeaders extra_request_headers_;
@@ -666,7 +663,8 @@ class URLRequest : public base::NonThreadSafe {
 
   // Callback passed to the network delegate to notify us when a blocked request
   // is ready to be resumed or canceled.
-  CompletionCallbackImpl<URLRequest> before_request_callback_;
+  scoped_refptr< CancelableCompletionCallback<URLRequest> >
+      before_request_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequest);
 };
diff --git a/net/url_request/url_request_test_util.cc b/net/url_request/url_request_test_util.cc
index a91460a..aedda63 100644
--- a/net/url_request/url_request_test_util.cc
+++ b/net/url_request/url_request_test_util.cc
@@ -248,25 +248,20 @@ void TestDelegate::OnResponseCompleted(net::URLRequest* request) {
 
 TestNetworkDelegate::TestNetworkDelegate()
   : last_os_error_(0),
-    error_count_(0),
-    created_requests_(0),
-    destroyed_requests_(0) {
+    error_count_(0) {
 }
 
 TestNetworkDelegate::~TestNetworkDelegate() {}
 
 int TestNetworkDelegate::OnBeforeURLRequest(
-    net::URLRequest* request,
-    net::CompletionCallback* callback,
-    GURL* new_url) {
-  created_requests_++;
+    net::URLRequest* request, net::CompletionCallback* callback) {
   return net::OK;
 }
 
 int TestNetworkDelegate::OnBeforeSendHeaders(
     uint64 request_id,
-    net::CompletionCallback* callback,
-    net::HttpRequestHeaders* headers) {
+    net::HttpRequestHeaders* headers,
+    net::CompletionCallback* callback) {
   return net::OK;
 }
 
@@ -286,7 +281,6 @@ void TestNetworkDelegate::OnReadCompleted(net::URLRequest* request,
 }
 
 void TestNetworkDelegate::OnURLRequestDestroyed(net::URLRequest* request) {
-  destroyed_requests_++;
 }
 
 net::URLRequestJob* TestNetworkDelegate::OnMaybeCreateURLRequestJob(
diff --git a/net/url_request/url_request_test_util.h b/net/url_request/url_request_test_util.h
index cef5449..4d77d80 100644
--- a/net/url_request/url_request_test_util.h
+++ b/net/url_request/url_request_test_util.h
@@ -190,17 +190,14 @@ class TestNetworkDelegate : public net::NetworkDelegate {
 
   int last_os_error() const { return last_os_error_; }
   int error_count() const { return error_count_; }
-  int created_requests() const { return created_requests_; }
-  int destroyed_requests() const { return destroyed_requests_; }
 
- protected:
+ private:
   // net::NetworkDelegate:
   virtual int OnBeforeURLRequest(net::URLRequest* request,
-                                 net::CompletionCallback* callback,
-                                 GURL* new_url);
+                                 net::CompletionCallback* callback);
   virtual int OnBeforeSendHeaders(uint64 request_id,
-                                  net::CompletionCallback* callback,
-                                  net::HttpRequestHeaders* headers);
+                                  net::HttpRequestHeaders* headers,
+                                  net::CompletionCallback* callback);
   virtual void OnResponseStarted(net::URLRequest* request);
   virtual void OnReadCompleted(net::URLRequest* request, int bytes_read);
   virtual void OnURLRequestDestroyed(net::URLRequest* request);
@@ -209,8 +206,6 @@ class TestNetworkDelegate : public net::NetworkDelegate {
 
   int last_os_error_;
   int error_count_;
-  int created_requests_;
-  int destroyed_requests_;
 };
 
 #endif  // NET_URL_REQUEST_URL_REQUEST_TEST_UTIL_H_
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 7ecc847..394f813 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -53,13 +53,6 @@
 
 using base::Time;
 
-// We don't need a refcount because we are guaranteed the test will not proceed
-// until its task is run.
-namespace net {
-class BlockingNetworkDelegate;
-}  // namespace net
-DISABLE_RUNNABLE_METHOD_REFCOUNT(net::BlockingNetworkDelegate);
-
 namespace net {
 
 namespace {
@@ -125,38 +118,6 @@ void CheckSSLInfo(const SSLInfo& ssl_info) {
 
 }  // namespace
 
-// A network delegate that blocks requests, optionally cancelling or redirecting
-// them.
-class BlockingNetworkDelegate : public TestNetworkDelegate {
- public:
-  BlockingNetworkDelegate() : callback_retval_(net::OK) {}
-
-  void set_callback_retval(int retval) { callback_retval_ = retval; }
-  void set_redirect_url(const GURL& url) { redirect_url_ = url; }
-
- private:
-  // TestNetworkDelegate:
-  virtual int OnBeforeURLRequest(net::URLRequest* request,
-                                 net::CompletionCallback* callback,
-                                 GURL* new_url) {
-    TestNetworkDelegate::OnBeforeURLRequest(request, callback, new_url);
-
-    if (!redirect_url_.is_empty())
-      *new_url = redirect_url_;
-    MessageLoop::current()->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &BlockingNetworkDelegate::DoCallback,
-                          callback));
-    return net::ERR_IO_PENDING;
-  }
-
-  void DoCallback(net::CompletionCallback* callback) {
-    callback->Run(callback_retval_);
-  }
-
-  int callback_retval_;
-  GURL redirect_url_;
-};
-
 // Inherit PlatformTest since we require the autorelease pool on Mac OS X.f
 class URLRequestTest : public PlatformTest {
  public:
@@ -302,61 +263,6 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateTunnelConnectionFailed) {
   }
 }
 
-// Tests that the network delegate can block and cancel a request.
-TEST_F(URLRequestTestHTTP, NetworkDelegateCancelRequest) {
-  ASSERT_TRUE(test_server_.Start());
-
-  TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
-  network_delegate.set_callback_retval(ERR_EMPTY_RESPONSE);
-
-  {
-    TestURLRequest r(test_server_.GetURL(""), &d);
-    scoped_refptr<TestURLRequestContext> context(
-        new TestURLRequestContext(test_server_.host_port_pair().ToString()));
-    context->set_network_delegate(&network_delegate);
-    r.set_context(context);
-
-    r.Start();
-    MessageLoop::current()->Run();
-
-    EXPECT_EQ(URLRequestStatus::FAILED, r.status().status());
-    EXPECT_EQ(ERR_EMPTY_RESPONSE, r.status().os_error());
-    EXPECT_EQ(1, network_delegate.created_requests());
-    EXPECT_EQ(0, network_delegate.destroyed_requests());
-  }
-  EXPECT_EQ(1, network_delegate.destroyed_requests());
-}
-
-// Tests that the network delegate can block and redirect a request to a new
-// URL.
-TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequest) {
-  ASSERT_TRUE(test_server_.Start());
-
-  TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
-  GURL redirect_url(test_server_.GetURL("simple.html"));
-  network_delegate.set_redirect_url(redirect_url);
-
-  {
-    TestURLRequest r(test_server_.GetURL("empty.html"), &d);
-    scoped_refptr<TestURLRequestContext> context(
-        new TestURLRequestContext(test_server_.host_port_pair().ToString()));
-    context->set_network_delegate(&network_delegate);
-    r.set_context(context);
-
-    r.Start();
-    MessageLoop::current()->Run();
-
-    EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
-    EXPECT_EQ(0, r.status().os_error());
-    EXPECT_EQ(redirect_url, r.url());
-    EXPECT_EQ(1, network_delegate.created_requests());
-    EXPECT_EQ(0, network_delegate.destroyed_requests());
-  }
-  EXPECT_EQ(1, network_delegate.destroyed_requests());
-}
-
 // In this unit test, we're using the HTTPTestServer as a proxy server and
 // issuing a CONNECT request with the magic host name "www.server-auth.com".
 // The HTTPTestServer will return a 401 response, which we should balk at.