[SPDY] Use WeakPtr<SpdySession> everywhere but SpdySessionPool

Make SpdyHttpStream cache it's SPDY stream's LoadTimingInfo
on close. Also, make SpdyHttpStream query SpdySession::IsReused()
when it's constructed and cache the value, as that's closer to the
intent of its use.

Avoid uses of SpdySession::IsClosed() in non-test code and add
TODO to remove uses from test code. This is more correct since
testing the WeakPtr is a stronger condition than testing openness
and SpdySession functions already do the right thing if the
SpdySession is already closed.

Tweak some tests that implicitly depended on
having refs to SpdySession.

BUG=255701
R=rch@chromium.org

Review URL: https://codereview.chromium.org/18546008

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

net/http/http_network_transaction_unittest.cc

@@ -63,6 +63,7 @@
 #include "net/test/cert_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
+#include "url/gurl.h"
 
 //-----------------------------------------------------------------------------
 
@@ -8855,7 +8856,7 @@ TEST_P(HttpNetworkTransactionTest,
   HostPortPair host_port_pair("www.google.com", 443);
   SpdySessionKey key(host_port_pair, ProxyServer::Direct(),
                      kPrivacyModeDisabled);
-  scoped_refptr<SpdySession> spdy_session =
+  base::WeakPtr<SpdySession> spdy_session =
       CreateSecureSpdySession(session, key, BoundNetLog());
 
   trans.reset(new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
@@ -9578,6 +9579,29 @@ TEST_P(HttpNetworkTransactionTest, SpdyPostNPNServerHangup) {
   EXPECT_EQ(ERR_CONNECTION_CLOSED, callback.WaitForResult());
 }
 
+// A subclass of HttpAuthHandlerMock that records the request URL when
+// it gets it. This is needed since the auth handler may get destroyed
+// before we get a chance to query it.
+class UrlRecordingHttpAuthHandlerMock : public HttpAuthHandlerMock {
+ public:
+  explicit UrlRecordingHttpAuthHandlerMock(GURL* url) : url_(url) {}
+
+  virtual ~UrlRecordingHttpAuthHandlerMock() {}
+
+ protected:
+  virtual int GenerateAuthTokenImpl(const AuthCredentials* credentials,
+                                    const HttpRequestInfo* request,
+                                    const CompletionCallback& callback,
+                                    std::string* auth_token) OVERRIDE {
+    *url_ = request->url;
+    return HttpAuthHandlerMock::GenerateAuthTokenImpl(
+        credentials, request, callback, auth_token);
+  }
+
+ private:
+  GURL* url_;
+};
+
 TEST_P(HttpNetworkTransactionTest, SpdyAlternateProtocolThroughProxy) {
   // This test ensures that the URL passed into the proxy is upgraded
   // to https when doing an Alternate Protocol upgrade.
@@ -9588,12 +9612,16 @@ TEST_P(HttpNetworkTransactionTest, SpdyAlternateProtocolThroughProxy) {
       ProxyService::CreateFixedFromPacResult("PROXY myproxy:70"));
   CapturingNetLog net_log;
   session_deps_.net_log = &net_log;
-  HttpAuthHandlerMock::Factory* auth_factory =
-      new HttpAuthHandlerMock::Factory();
-  HttpAuthHandlerMock* auth_handler = new HttpAuthHandlerMock();
-  auth_factory->AddMockHandler(auth_handler, HttpAuth::AUTH_PROXY);
-  auth_factory->set_do_init_from_challenge(true);
-  session_deps_.http_auth_handler_factory.reset(auth_factory);
+  GURL request_url;
+  {
+    HttpAuthHandlerMock::Factory* auth_factory =
+        new HttpAuthHandlerMock::Factory();
+    UrlRecordingHttpAuthHandlerMock* auth_handler =
+        new UrlRecordingHttpAuthHandlerMock(&request_url);
+    auth_factory->AddMockHandler(auth_handler, HttpAuth::AUTH_PROXY);
+    auth_factory->set_do_init_from_challenge(true);
+    session_deps_.http_auth_handler_factory.reset(auth_factory);
+  }
 
   HttpRequestInfo request;
   request.method = "GET";
@@ -9724,7 +9752,6 @@ TEST_P(HttpNetworkTransactionTest, SpdyAlternateProtocolThroughProxy) {
 
   // After all that work, these two lines (or actually, just the scheme) are
   // what this test is all about. Make sure it happens correctly.
-  const GURL& request_url = auth_handler->request_url();
   EXPECT_EQ("https", request_url.scheme());
   EXPECT_EQ("www.google.com", request_url.host());
 
@@ -9987,7 +10014,7 @@ TEST_P(HttpNetworkTransactionTest, PreconnectWithExistingSpdySession) {
   HostPortPair host_port_pair("www.google.com", 443);
   SpdySessionKey key(host_port_pair, ProxyServer::Direct(),
                      kPrivacyModeDisabled);
-  scoped_refptr<SpdySession> spdy_session =
+  base::WeakPtr<SpdySession> spdy_session =
       CreateInsecureSpdySession(session, key, BoundNetLog());
 
   HttpRequestInfo request;

net/http/http_proxy_client_socket_pool.cc

@@ -302,7 +302,7 @@ int HttpProxyConnectJob::DoSpdyProxyCreateStream() {
                      ProxyServer::Direct(),
                      kPrivacyModeDisabled);
   SpdySessionPool* spdy_pool = params_->spdy_session_pool();
-  scoped_refptr<SpdySession> spdy_session =
+  base::WeakPtr<SpdySession> spdy_session =
       spdy_pool->FindAvailableSession(key, net_log());
   // It's possible that a session to the proxy has recently been created
   if (spdy_session) {

net/http/http_stream_factory_impl.cc

@@ -263,23 +263,24 @@ void HttpStreamFactoryImpl::OrphanJob(Job* job, const Request* request) {
 }
 
 void HttpStreamFactoryImpl::OnNewSpdySessionReady(
-    scoped_refptr<SpdySession> spdy_session,
+    const base::WeakPtr<SpdySession>& spdy_session,
     bool direct,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     bool was_npn_negotiated,
     NextProto protocol_negotiated,
     bool using_spdy,
     const BoundNetLog& net_log) {
-  const SpdySessionKey& spdy_session_key =
-      spdy_session->spdy_session_key();
-  while (!spdy_session->IsClosed()) {
-    // Each iteration may empty out the RequestSet for |spdy_session_key_ in
+  while (true) {
+    if (!spdy_session)
+      break;
+    const SpdySessionKey& spdy_session_key = spdy_session->spdy_session_key();
+    // Each iteration may empty out the RequestSet for |spdy_session_key| in
     // |spdy_session_request_map_|. So each time, check for RequestSet and use
     // the first one.
     //
     // TODO(willchan): If it's important, switch RequestSet out for a FIFO
-    // pqueue (Order by priority first, then FIFO within same priority). Unclear
+    // queue (Order by priority first, then FIFO within same priority). Unclear
     // that it matters here.
     if (!ContainsKey(spdy_session_request_map_, spdy_session_key))
       break;
@@ -297,14 +298,14 @@ void HttpStreamFactoryImpl::OnNewSpdySessionReady(
           NULL,
           used_ssl_config,
           used_proxy_info,
-          factory->CreateSpdyStream(spdy_session.get(), use_relative_url));
+          factory->CreateSpdyStream(spdy_session, use_relative_url));
     } else {
       bool use_relative_url = direct || request->url().SchemeIs("https");
       request->OnStreamReady(
           NULL,
           used_ssl_config,
           used_proxy_info,
-          new SpdyHttpStream(spdy_session.get(), use_relative_url));
+          new SpdyHttpStream(spdy_session, use_relative_url));
     }
   }
   // TODO(mbelshe): Alert other valid requests.

net/http/http_stream_factory_impl.h

@@ -94,7 +94,7 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl :
   // Called when a SpdySession is ready. It will find appropriate Requests and
   // fulfill them. |direct| indicates whether or not |spdy_session| uses a
   // proxy.
-  void OnNewSpdySessionReady(scoped_refptr<SpdySession> spdy_session,
+  void OnNewSpdySessionReady(const base::WeakPtr<SpdySession>& spdy_session,
                              bool direct,
                              const SSLConfig& used_ssl_config,
                              const ProxyInfo& used_proxy_info,

net/http/http_stream_factory_impl_job.cc

@@ -4,6 +4,7 @@
 
 #include "net/http/http_stream_factory_impl_job.h"
 
+#include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ -330,9 +331,10 @@ void HttpStreamFactoryImpl::Job::OnNewSpdySessionReadyCallback() {
   DCHECK(!stream_.get());
   DCHECK(!IsPreconnecting());
   DCHECK(using_spdy());
-  DCHECK(new_spdy_session_.get());
-  scoped_refptr<SpdySession> spdy_session = new_spdy_session_;
-  new_spdy_session_ = NULL;
+  if (!new_spdy_session_)
+    return;
+  base::WeakPtr<SpdySession> spdy_session = new_spdy_session_;
+  new_spdy_session_.reset();
   if (IsOrphaned()) {
     stream_factory_->OnNewSpdySessionReady(
         spdy_session, spdy_session_direct_, server_ssl_config_, proxy_info_,
@@ -762,7 +764,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
   // Check first if we have a spdy session for this group.  If so, then go
   // straight to using that.
   SpdySessionKey spdy_session_key = GetSpdySessionKey();
-  scoped_refptr<SpdySession> spdy_session =
+  base::WeakPtr<SpdySession> spdy_session =
       session_->spdy_session_pool()->FindAvailableSession(
           spdy_session_key, net_log_);
   if (spdy_session && CanUseExistingSpdySession()) {
@@ -1097,13 +1099,13 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     direct = false;
   }
 
-  scoped_refptr<SpdySession> spdy_session;
+  base::WeakPtr<SpdySession> spdy_session;
   if (existing_spdy_session_.get()) {
     // We picked up an existing session, so we don't need our socket.
     if (connection_->socket())
       connection_->socket()->Disconnect();
     connection_->Reset();
-    spdy_session.swap(existing_spdy_session_);
+    std::swap(spdy_session, existing_spdy_session_);
   } else {
     SpdySessionPool* spdy_pool = session_->spdy_session_pool();
     spdy_session = spdy_pool->FindAvailableSession(spdy_session_key, net_log_);
@@ -1127,7 +1129,7 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     }
   }
 
-  if (spdy_session->IsClosed())
+  if (!spdy_session)
     return ERR_CONNECTION_CLOSED;
 
   // TODO(willchan): Delete this code, because eventually, the
@@ -1140,10 +1142,10 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     bool use_relative_url = direct || request_info_.url.SchemeIs("wss");
     websocket_stream_.reset(
         request_->websocket_stream_factory()->CreateSpdyStream(
-            spdy_session.get(), use_relative_url));
+            spdy_session, use_relative_url));
   } else {
     bool use_relative_url = direct || request_info_.url.SchemeIs("https");
-    stream_.reset(new SpdyHttpStream(spdy_session.get(), use_relative_url));
+    stream_.reset(new SpdyHttpStream(spdy_session, use_relative_url));
   }
   return OK;
 }

net/http/http_stream_factory_impl_job.h

@@ -307,10 +307,10 @@ class HttpStreamFactoryImpl::Job {
   int num_streams_;
 
   // Initialized when we create a new SpdySession.
-  scoped_refptr<SpdySession> new_spdy_session_;
+  base::WeakPtr<SpdySession> new_spdy_session_;
 
   // Initialized when we have an existing SpdySession.
-  scoped_refptr<SpdySession> existing_spdy_session_;
+  base::WeakPtr<SpdySession> existing_spdy_session_;
 
   // Only used if |new_spdy_session_| is non-NULL.
   bool spdy_session_direct_;

net/http/http_stream_factory_impl_request.cc

@@ -279,7 +279,7 @@ HttpStreamFactoryImpl::Request::RemoveRequestFromHttpPipeliningRequestMap() {
 
 void HttpStreamFactoryImpl::Request::OnNewSpdySessionReady(
     Job* job,
-    scoped_refptr<SpdySession> spdy_session,
+    const base::WeakPtr<SpdySession>& spdy_session,
     bool direct) {
   DCHECK(job);
   DCHECK(job->using_spdy());
@@ -311,17 +311,17 @@ void HttpStreamFactoryImpl::Request::OnNewSpdySessionReady(
     delegate_->OnWebSocketStreamReady(
         job->server_ssl_config(),
         job->proxy_info(),
-        websocket_stream_factory_->CreateSpdyStream(spdy_session.get(),
+        websocket_stream_factory_->CreateSpdyStream(spdy_session,
                                                     use_relative_url));
   } else {
     bool use_relative_url = direct || url().SchemeIs("https");
     delegate_->OnStreamReady(
         job->server_ssl_config(),
         job->proxy_info(),
-        new SpdyHttpStream(spdy_session.get(), use_relative_url));
+        new SpdyHttpStream(spdy_session, use_relative_url));
   }
   // |this| may be deleted after this point.
-  factory->OnNewSpdySessionReady(spdy_session.get(),
+  factory->OnNewSpdySessionReady(spdy_session,
                                  direct,
                                  used_ssl_config,
                                  used_proxy_info,

net/http/http_stream_factory_impl_request.h

@@ -63,7 +63,7 @@ class HttpStreamFactoryImpl::Request : public HttpStreamRequest {
 
   // Called by an attached Job if it sets up a SpdySession.
   void OnNewSpdySessionReady(Job* job,
-                             scoped_refptr<SpdySession> spdy_session,
+                             const base::WeakPtr<SpdySession>& spdy_session,
                              bool direct);
 
   WebSocketStreamBase::Factory* websocket_stream_factory() {

net/http/http_stream_factory_impl_unittest.cc

@@ -201,15 +201,15 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
 
 class WebSocketSpdyStream : public MockWebSocketStream {
  public:
-  explicit WebSocketSpdyStream(SpdySession* spdy_session)
+  explicit WebSocketSpdyStream(const base::WeakPtr<SpdySession>& spdy_session)
       : MockWebSocketStream(kStreamTypeSpdy), spdy_session_(spdy_session) {}
 
   virtual ~WebSocketSpdyStream() {}
 
   SpdySession* spdy_session() { return spdy_session_.get(); }
 
  private:
-  scoped_refptr<SpdySession> spdy_session_;
+  base::WeakPtr<SpdySession> spdy_session_;
 };
 
 class WebSocketBasicStream : public MockWebSocketStream {
@@ -237,7 +237,7 @@ class WebSocketStreamFactory : public WebSocketStreamBase::Factory {
   }
 
   virtual WebSocketStreamBase* CreateSpdyStream(
-      SpdySession* spdy_session,
+      const base::WeakPtr<SpdySession>& spdy_session,
       bool use_relative_url) OVERRIDE {
     return new WebSocketSpdyStream(spdy_session);
   }
@@ -1022,17 +1022,20 @@ TEST_P(HttpStreamFactoryTest, RequestSpdyHttpStream) {
                                        ProxyService::CreateDirect());
 
   MockRead mock_read(ASYNC, OK);
-  StaticSocketDataProvider socket_data(&mock_read, 1, NULL, 0);
+  DeterministicSocketData socket_data(&mock_read, 1, NULL, 0);
   socket_data.set_connect_data(MockConnect(ASYNC, OK));
-  session_deps.socket_factory->AddSocketDataProvider(&socket_data);
+  session_deps.deterministic_socket_factory->AddSocketDataProvider(
+      &socket_data);
 
   SSLSocketDataProvider ssl_socket_data(ASYNC, OK);
   ssl_socket_data.SetNextProto(GetParam());
-  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_socket_data);
+  session_deps.deterministic_socket_factory->AddSSLSocketDataProvider(
+      &ssl_socket_data);
 
   HostPortPair host_port_pair("www.google.com", 443);
   scoped_refptr<HttpNetworkSession>
-      session(SpdySessionDependencies::SpdyCreateSession(&session_deps));
+      session(SpdySessionDependencies::SpdyCreateSessionDeterministic(
+          &session_deps));
 
   // Now request a stream.
   HttpRequestInfo request_info;
@@ -1149,21 +1152,25 @@ TEST_P(HttpStreamFactoryTest, OrphanedWebSocketStream) {
                                        ProxyService::CreateDirect());
 
   MockRead mock_read(ASYNC, OK);
-  StaticSocketDataProvider socket_data(&mock_read, 1, NULL, 0);
+  DeterministicSocketData socket_data(&mock_read, 1, NULL, 0);
   socket_data.set_connect_data(MockConnect(ASYNC, OK));
-  session_deps.socket_factory->AddSocketDataProvider(&socket_data);
+  session_deps.deterministic_socket_factory->AddSocketDataProvider(
+      &socket_data);
 
   MockRead mock_read2(ASYNC, OK);
-  StaticSocketDataProvider socket_data2(&mock_read2, 1, NULL, 0);
+  DeterministicSocketData socket_data2(&mock_read2, 1, NULL, 0);
   socket_data2.set_connect_data(MockConnect(ASYNC, ERR_IO_PENDING));
-  session_deps.socket_factory->AddSocketDataProvider(&socket_data2);
+  session_deps.deterministic_socket_factory->AddSocketDataProvider(
+      &socket_data2);
 
   SSLSocketDataProvider ssl_socket_data(ASYNC, OK);
   ssl_socket_data.SetNextProto(GetParam());
-  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_socket_data);
+  session_deps.deterministic_socket_factory->AddSSLSocketDataProvider(
+      &ssl_socket_data);
 
   scoped_refptr<HttpNetworkSession>
-      session(SpdySessionDependencies::SpdyCreateSession(&session_deps));
+      session(SpdySessionDependencies::SpdyCreateSessionDeterministic(
+          &session_deps));
 
   // Now request a stream.
   HttpRequestInfo request_info;

net/socket/ssl_client_socket_pool_unittest.cc

@@ -759,7 +759,7 @@ TEST_F(SSLClientSocketPoolTest, IPPooling) {
   socket_factory_.AddSSLSocketDataProvider(&ssl);
 
   CreatePool(true /* tcp pool */, false, false);
-  scoped_refptr<SpdySession> spdy_session =
+  base::WeakPtr<SpdySession> spdy_session =
       CreateSecureSpdySession(session_, test_hosts[0].key, BoundNetLog());
 
   EXPECT_TRUE(
@@ -812,7 +812,7 @@ void SSLClientSocketPoolTest::TestIPPoolingDisabled(
   socket_factory_.AddSSLSocketDataProvider(ssl);
 
   CreatePool(true /* tcp pool */, false, false);
-  scoped_refptr<SpdySession> spdy_session =
+  base::WeakPtr<SpdySession> spdy_session =
       CreateSecureSpdySession(session_, test_hosts[0].key, BoundNetLog());
 
   EXPECT_TRUE(