Add a RenewStreamForAuth method to HttpStream, replacing DetachConnection

BUG=58192
TEST=Start chrome with --auth-schemes=NTLM and --proxy-server pointing to a Microsoft Forefront Threat Management Gateway proxy configurated for Integrated Authentication. Assuming the user is part of the same domain as the proxy, authentication should work transparently, and the user should not be presented with auth prompts. Also, net_unittests should pass.

Review URL: http://codereview.chromium.org/3676004

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

13 files changed, 52 insertions, 93 deletions

diff --git a/net/http/http_basic_stream.cc b/net/http/http_basic_stream.cc
index 4303291..ecd692b 100644
--- a/net/http/http_basic_stream.cc
+++ b/net/http/http_basic_stream.cc
@@ -19,7 +19,6 @@ HttpBasicStream::HttpBasicStream(ClientSocketHandle* connection)
 int HttpBasicStream::InitializeStream(const HttpRequestInfo* request_info,
                                       const BoundNetLog& net_log,
                                       CompletionCallback* callback) {
-  DCHECK(!IsDetached());
   parser_.reset(new HttpStreamParser(connection_.get(), request_info,
                                      read_buf_, net_log));
   return OK;
@@ -31,80 +30,66 @@ int HttpBasicStream::SendRequest(const std::string& headers,
                                  HttpResponseInfo* response,
                                  CompletionCallback* callback) {
   DCHECK(parser_.get());
-  DCHECK(!IsDetached());
   return parser_->SendRequest(headers, request_body, response, callback);
 }
 
 HttpBasicStream::~HttpBasicStream() {}
 
 uint64 HttpBasicStream::GetUploadProgress() const {
-  DCHECK(!IsDetached());
   return parser_->GetUploadProgress();
 }
 
 int HttpBasicStream::ReadResponseHeaders(CompletionCallback* callback) {
-  DCHECK(!IsDetached());
   return parser_->ReadResponseHeaders(callback);
 }
 
 const HttpResponseInfo* HttpBasicStream::GetResponseInfo() const {
-  DCHECK(!IsDetached());
   return parser_->GetResponseInfo();
 }
 
 int HttpBasicStream::ReadResponseBody(IOBuffer* buf, int buf_len,
                                       CompletionCallback* callback) {
-  DCHECK(!IsDetached());
   return parser_->ReadResponseBody(buf, buf_len, callback);
 }
 
 void HttpBasicStream::Close(bool not_reusable) {
-  DCHECK(!IsDetached());
   parser_->Close(not_reusable);
 }
 
+HttpStream* HttpBasicStream::RenewStreamForAuth() {
+  DCHECK(IsResponseBodyComplete());
+  DCHECK(!IsMoreDataBuffered());
+  parser_.reset();
+  return new HttpBasicStream(connection_.release());
+}
+
 bool HttpBasicStream::IsResponseBodyComplete() const {
-  DCHECK(!IsDetached());
   return parser_->IsResponseBodyComplete();
 }
 
 bool HttpBasicStream::CanFindEndOfResponse() const {
-  DCHECK(!IsDetached());
   return parser_->CanFindEndOfResponse();
 }
 
 bool HttpBasicStream::IsMoreDataBuffered() const {
-  DCHECK(!IsDetached());
   return parser_->IsMoreDataBuffered();
 }
 
 bool HttpBasicStream::IsConnectionReused() const {
-  DCHECK(!IsDetached());
   return parser_->IsConnectionReused();
 }
 
 void HttpBasicStream::SetConnectionReused() {
-  DCHECK(!IsDetached());
   parser_->SetConnectionReused();
 }
 
-ClientSocketHandle* HttpBasicStream::DetachConnection() {
-  return connection_.release();
-}
-
 void HttpBasicStream::GetSSLInfo(SSLInfo* ssl_info) {
-  DCHECK(!IsDetached());
   parser_->GetSSLInfo(ssl_info);
 }
 
 void HttpBasicStream::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
-  DCHECK(!IsDetached());
   parser_->GetSSLCertRequestInfo(cert_request_info);
 }
 
-bool HttpBasicStream::IsDetached() const {
-  return connection_.get() == NULL;
-}
-
 }  // namespace net
diff --git a/net/http/http_basic_stream.h b/net/http/http_basic_stream.h
index 2da2f4d..ffab967 100644
--- a/net/http/http_basic_stream.h
+++ b/net/http/http_basic_stream.h
@@ -53,6 +53,8 @@ class HttpBasicStream : public HttpStream {
 
   virtual void Close(bool not_reusable);
 
+  virtual HttpStream* RenewStreamForAuth();
+
   virtual bool IsResponseBodyComplete() const;
 
   virtual bool CanFindEndOfResponse() const;
@@ -63,15 +65,11 @@ class HttpBasicStream : public HttpStream {
 
   virtual void SetConnectionReused();
 
-  virtual ClientSocketHandle* DetachConnection();
-
   virtual void GetSSLInfo(SSLInfo* ssl_info);
 
   virtual void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info);
 
  private:
-  bool IsDetached() const;
-
   scoped_refptr<GrowableIOBuffer> read_buf_;
 
   scoped_ptr<HttpStreamParser> parser_;
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index bf1c0e9..b5a5707 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -326,20 +326,25 @@ void HttpNetworkTransaction::DidDrainBodyForAuthRestart(bool keep_alive) {
   DCHECK(!stream_request_.get());
 
   if (stream_.get()) {
+    HttpStream* new_stream = NULL;
     if (keep_alive) {
       // We should call connection_->set_idle_time(), but this doesn't occur
       // often enough to be worth the trouble.
       stream_->SetConnectionReused();
-      DCHECK(!auth_connection_.get());
-      auth_connection_.reset(stream_->DetachConnection());
+      new_stream = stream_->RenewStreamForAuth();
+    }
+
+    if (!new_stream) {
+      stream_->Close(!keep_alive);
+      next_state_ = STATE_CREATE_STREAM;
     } else {
-      stream_->Close(true);
+      next_state_ = STATE_INIT_STREAM;
     }
-    next_state_ = STATE_CREATE_STREAM;
+    stream_.reset(new_stream);
   }
 
   // Reset the other member variables.
-  ResetStateForRestart();
+  ResetStateForAuthRestart();
 }
 
 bool HttpNetworkTransaction::IsReadyToRestartForAuth() {
@@ -593,15 +598,13 @@ int HttpNetworkTransaction::DoLoop(int result) {
 int HttpNetworkTransaction::DoCreateStream() {
   next_state_ = STATE_CREATE_STREAM_COMPLETE;
 
-  session_->http_stream_factory()->RequestStream(
-      request_,
-      &ssl_config_,
-      &proxy_info_,
-      auth_connection_.release(),
-      this,
-      net_log_,
-      session_,
-      &stream_request_);
+  session_->http_stream_factory()->RequestStream(request_,
+                                                 &ssl_config_,
+                                                 &proxy_info_,
+                                                 this,
+                                                 net_log_,
+                                                 session_,
+                                                 &stream_request_);
   return ERR_IO_PENDING;
 }
 
@@ -1125,10 +1128,14 @@ int HttpNetworkTransaction::HandleIOError(int error) {
 }
 
 void HttpNetworkTransaction::ResetStateForRestart() {
+  ResetStateForAuthRestart();
+  stream_.reset();
+}
+
+void HttpNetworkTransaction::ResetStateForAuthRestart() {
   pending_auth_target_ = HttpAuth::AUTH_NONE;
   read_buf_ = NULL;
   read_buf_len_ = 0;
-  stream_.reset();
   headers_valid_ = false;
   request_headers_.clear();
   response_ = HttpResponseInfo();
diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
index b53363e..7ddee29 100644
--- a/net/http/http_network_transaction.h
+++ b/net/http/http_network_transaction.h
@@ -24,7 +24,6 @@
 
 namespace net {
 
-class ClientSocketHandle;
 class HttpAuthController;
 class HttpNetworkSession;
 class HttpStream;
@@ -169,6 +168,10 @@ class HttpNetworkTransaction : public HttpTransaction,
   // Resets the members of the transaction so it can be restarted.
   void ResetStateForRestart();
 
+  // Resets the members of the transaction, except |stream_|, which needs
+  // to be maintained for multi-round auth.
+  void ResetStateForAuthRestart();
+
   // Returns true if we should try to add a Proxy-Authorization header
   bool ShouldApplyProxyAuth() const;
 
@@ -211,10 +214,6 @@ class HttpNetworkTransaction : public HttpTransaction,
   scoped_refptr<StreamFactory::StreamRequestJob> stream_request_;
   scoped_ptr<HttpStream> stream_;
 
-  // Reuse the same connection for each round of a connection-based HTTP
-  // authentication scheme.
-  scoped_ptr<ClientSocketHandle> auth_connection_;
-
   // True if we've validated the headers that the stream parser has returned.
   bool headers_valid_;
 
diff --git a/net/http/http_response_body_drainer_unittest.cc b/net/http/http_response_body_drainer_unittest.cc
index 78168fd..119d8f7 100644
--- a/net/http/http_response_body_drainer_unittest.cc
+++ b/net/http/http_response_body_drainer_unittest.cc
@@ -94,8 +94,6 @@ class MockHttpStream : public HttpStream {
   virtual bool IsMoreDataBuffered() const { return false; }
   virtual bool IsConnectionReused() const { return false; }
   virtual void SetConnectionReused() {}
-  virtual ClientSocketHandle* DetachConnection() { return NULL; }
-
   virtual void GetSSLInfo(SSLInfo* ssl_info) {}
   virtual void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info) {}
 
@@ -107,6 +105,11 @@ class MockHttpStream : public HttpStream {
     closed_ = true;
     result_waiter_->set_result(not_reusable);
   }
+
+  virtual HttpStream* RenewStreamForAuth() {
+    return NULL;
+  }
+
   virtual bool IsResponseBodyComplete() const { return is_complete_; }
 
   // Methods to tweak/observer mock behavior:
diff --git a/net/http/http_stream.h b/net/http/http_stream.h
index 601d01b..5aa84e0 100644
--- a/net/http/http_stream.h
+++ b/net/http/http_stream.h
@@ -22,7 +22,6 @@
 namespace net {
 
 class BoundNetLog;
-class ClientSocketHandle;
 class HttpResponseInfo;
 class IOBuffer;
 class SSLCertRequestInfo;
@@ -88,6 +87,13 @@ class HttpStream {
   //                eliminate the SetConnectionReused() below.
   virtual void Close(bool not_reusable) = 0;
 
+  // Returns a new (not initialized) stream using the same underlying
+  // connection and invalidates the old stream - no further methods should be
+  // called on the old stream.  The caller should ensure that the response body
+  // from the previous request is drained before calling this method.  If the
+  // subclass does not support renewing the stream, NULL is returned.
+  virtual HttpStream* RenewStreamForAuth() = 0;
+
   // Indicates if the response body has been completely read.
   virtual bool IsResponseBodyComplete() const = 0;
 
@@ -109,17 +115,6 @@ class HttpStream {
   virtual bool IsConnectionReused() const = 0;
   virtual void SetConnectionReused() = 0;
 
-  // Detach the connection from this HttpStream. The caller is responsible
-  // for deleting the handle. After this is called, none of the other HttpStream
-  // methods should be called.
-  //
-  // The return value may be NULL. In that case, the underlying connection
-  // is either unavailable, or can be consistently rediscoverable.
-  //
-  // TODO(cbentzel): Consider ResetForAuth() approach instead.
-  // http://crbug.com/58192
-  virtual ClientSocketHandle* DetachConnection() = 0;
-
   // Get the SSLInfo associated with this stream's connection.  This should
   // only be called for streams over SSL sockets, otherwise the behavior is
   // undefined.
diff --git a/net/http/http_stream_factory.cc b/net/http/http_stream_factory.cc
index 9378357..7a26282 100644
--- a/net/http/http_stream_factory.cc
+++ b/net/http/http_stream_factory.cc
@@ -48,20 +48,13 @@ void HttpStreamFactory::RequestStream(
     const HttpRequestInfo* request_info,
     SSLConfig* ssl_config,
     ProxyInfo* proxy_info,
-    ClientSocketHandle* connection,
     StreamFactory::StreamRequestDelegate* delegate,
     const BoundNetLog& net_log,
     const scoped_refptr<HttpNetworkSession>& session,
     scoped_refptr<StreamRequestJob>* stream) {
   DCHECK(stream != NULL);
   *stream = new HttpStreamRequest(this, session);
-  (*stream)->Start(
-      request_info,
-      ssl_config,
-      proxy_info,
-      connection,
-      delegate,
-      net_log);
+  (*stream)->Start(request_info, ssl_config, proxy_info, delegate, net_log);
 }
 
 void HttpStreamFactory::AddTLSIntolerantServer(const GURL& url) {
diff --git a/net/http/http_stream_factory.h b/net/http/http_stream_factory.h
index db50038..08da415 100644
--- a/net/http/http_stream_factory.h
+++ b/net/http/http_stream_factory.h
@@ -33,7 +33,6 @@ class HttpStreamFactory : public StreamFactory,
   virtual void RequestStream(const HttpRequestInfo* info,
                              SSLConfig* ssl_config,
                              ProxyInfo* proxy_info,
-                             ClientSocketHandle* connection,
                              StreamRequestDelegate* delegate,
                              const BoundNetLog& net_log,
                              const scoped_refptr<HttpNetworkSession>& session,
diff --git a/net/http/http_stream_request.cc b/net/http/http_stream_request.cc
index f60a86c..13c30e6 100644
--- a/net/http/http_stream_request.cc
+++ b/net/http/http_stream_request.cc
@@ -54,6 +54,7 @@ HttpStreamRequest::HttpStreamRequest(
       session_(session),
       ALLOW_THIS_IN_INITIALIZER_LIST(
           io_callback_(this, &HttpStreamRequest::OnIOComplete)),
+      connection_(new ClientSocketHandle),
       factory_(factory),
       delegate_(NULL),
       next_state_(STATE_NONE),
@@ -90,7 +91,6 @@ HttpStreamRequest::~HttpStreamRequest() {
 void HttpStreamRequest::Start(const HttpRequestInfo* request_info,
                               SSLConfig* ssl_config,
                               ProxyInfo* proxy_info,
-                              ClientSocketHandle* connection,
                               StreamFactory::StreamRequestDelegate* delegate,
                               const BoundNetLog& net_log) {
   CHECK_EQ(STATE_NONE, next_state_);
@@ -101,15 +101,7 @@ void HttpStreamRequest::Start(const HttpRequestInfo* request_info,
   proxy_info_ = proxy_info;
   delegate_ = delegate;
   net_log_ = net_log;
-  if (connection) {
-    DCHECK(!using_spdy_);
-    DCHECK(connection->is_initialized());
-    connection_.reset(connection);
-    next_state_ = STATE_CREATE_STREAM;
-  } else {
-    connection_.reset(new ClientSocketHandle);
-    next_state_ = STATE_RESOLVE_PROXY;
-  }
+  next_state_ = STATE_RESOLVE_PROXY;
   int rv = RunLoop(OK);
   DCHECK_EQ(ERR_IO_PENDING, rv);
 }
diff --git a/net/http/http_stream_request.h b/net/http/http_stream_request.h
index cad0e59..dcec385 100644
--- a/net/http/http_stream_request.h
+++ b/net/http/http_stream_request.h
@@ -42,7 +42,6 @@ class HttpStreamRequest : public StreamFactory::StreamRequestJob {
   virtual void Start(const HttpRequestInfo* request_info,
                      SSLConfig* ssl_config,
                      ProxyInfo* proxy_info,
-                     ClientSocketHandle* connection,
                      StreamFactory::StreamRequestDelegate* delegate,
                      const BoundNetLog& net_log);
   virtual void Cancel();
diff --git a/net/http/stream_factory.h b/net/http/stream_factory.h
index 6e89c53..9ec85f1 100644
--- a/net/http/stream_factory.h
+++ b/net/http/stream_factory.h
@@ -13,7 +13,6 @@
 namespace net {
 
 class BoundNetLog;
-class ClientSocketHandle;
 class HostPortPair;
 class HttpAlternateProtocols;
 class HttpAuthController;
@@ -84,7 +83,6 @@ class StreamFactory {
     virtual void Start(const HttpRequestInfo* request_info,
                        SSLConfig* ssl_config,
                        ProxyInfo* proxy_info,
-                       ClientSocketHandle* connection,
                        StreamRequestDelegate* delegate,
                        const BoundNetLog& net_log) = 0;
 
@@ -130,7 +128,6 @@ class StreamFactory {
   virtual void RequestStream(const HttpRequestInfo* info,
                              SSLConfig* ssl_config,
                              ProxyInfo* proxy_info,
-                             ClientSocketHandle* connection,
                              StreamRequestDelegate* delegate,
                              const BoundNetLog& net_log,
                              const scoped_refptr<HttpNetworkSession>& session,
diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index aa86604..0e3962e 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -381,12 +381,4 @@ void SpdyHttpStream::GetSSLCertRequestInfo(
   stream_->GetSSLCertRequestInfo(cert_request_info);
 }
 
-ClientSocketHandle* SpdyHttpStream::DetachConnection() {
-  // DetachConnection is currently used to ensure that multi-round HTTP
-  // authentication takes place on the same connection. Since SpdyHttpStream's
-  // for the same domain will always map to the same SpdySession, NULL can
-  // be returned.
-  return NULL;
-}
-
 }  // namespace net
diff --git a/net/spdy/spdy_http_stream.h b/net/spdy/spdy_http_stream.h
index a8e1a2c..372db57 100644
--- a/net/spdy/spdy_http_stream.h
+++ b/net/spdy/spdy_http_stream.h
@@ -70,6 +70,8 @@ class SpdyHttpStream : public SpdyStream::Delegate, public HttpStream {
   // Closes the stream.
   virtual void Close(bool not_reusable);
 
+  virtual HttpStream* RenewStreamForAuth() { return NULL; }
+
   // Indicates if the response body has been completely read.
   virtual bool IsResponseBodyComplete() const {
     if (!stream_)
@@ -91,8 +93,6 @@ class SpdyHttpStream : public SpdyStream::Delegate, public HttpStream {
     // SPDY doesn't need an indicator here.
   }
 
-  virtual ClientSocketHandle* DetachConnection();
-
   virtual void GetSSLInfo(SSLInfo* ssl_info);
   virtual void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info);