Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established

net/http/http_stream_factory_impl_unittest.cc

Index: net/http/http_stream_factory_impl_unittest.cc
diff --git a/net/http/http_stream_factory_impl_unittest.cc b/net/http/http_stream_factory_impl_unittest.cc
index 7a3b9f50f3f9b670cf38cf2d9ac91b142b8a65b9..bf2af49e28a1c220eb5dcb6f70d6d59d21e10d44 100644
--- a/net/http/http_stream_factory_impl_unittest.cc
+++ b/net/http/http_stream_factory_impl_unittest.cc
@@ -179,8 +179,7 @@ class MockHttpStreamFactoryImplForPreconnect : public HttpStreamFactoryImpl {
 
 class StreamRequestWaiter : public HttpStreamRequest::Delegate {
  public:
-  StreamRequestWaiter()
-      : waiting_for_stream_(false), stream_done_(false), error_status_(OK) {}
+  StreamRequestWaiter() : stream_done_(false), error_status_(OK) {}
 
   // HttpStreamRequest::Delegate
 
@@ -188,8 +187,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
                      const ProxyInfo& used_proxy_info,
                      HttpStream* stream) override {
     stream_done_ = true;
-    if (waiting_for_stream_)
-      loop_.Quit();
+    loop_.Quit();
     stream_.reset(stream);
     used_ssl_config_ = used_ssl_config;
     used_proxy_info_ = used_proxy_info;
@@ -200,8 +198,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
       const ProxyInfo& used_proxy_info,
       WebSocketHandshakeStreamBase* stream) override {
     stream_done_ = true;
-    if (waiting_for_stream_)
-      loop_.Quit();
+    loop_.Quit();
     websocket_stream_.reset(stream);
     used_ssl_config_ = used_ssl_config;
     used_proxy_info_ = used_proxy_info;
@@ -212,8 +209,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
       const ProxyInfo& used_proxy_info,
       BidirectionalStreamImpl* stream) override {
     stream_done_ = true;
-    if (waiting_for_stream_)
-      loop_.Quit();
+    loop_.Quit();
     bidirectional_stream_impl_.reset(stream);
     used_ssl_config_ = used_ssl_config;
     used_proxy_info_ = used_proxy_info;
@@ -221,8 +217,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
 
   void OnStreamFailed(int status, const SSLConfig& used_ssl_config) override {
     stream_done_ = true;
-    if (waiting_for_stream_)
-      loop_.Quit();
+    loop_.Quit();
     used_ssl_config_ = used_ssl_config;
     error_status_ = status;
   }
@@ -246,13 +241,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
 
   void OnQuicBroken() override {}
 
-  void WaitForStream() {
-    while (!stream_done_) {
-      waiting_for_stream_ = true;
-      loop_.Run();
-      waiting_for_stream_ = false;
-    }
-  }
+  void WaitForStream() { loop_.Run(); }
 
   const SSLConfig& used_ssl_config() const {
     return used_ssl_config_;
@@ -278,7 +267,6 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
   int error_status() const { return error_status_; }
 
  private:
-  bool waiting_for_stream_;
   bool stream_done_;
   base::RunLoop loop_;
   std::unique_ptr<HttpStream> stream_;
@@ -966,7 +954,6 @@ TEST_F(HttpStreamFactoryTest, WithQUICAlternativeProxyMarkedAsBad) {
       request_info.url = GURL("http://www.google.com");
 
       SSLConfig ssl_config;
-      StreamRequestWaiter waiter;
 
       EXPECT_EQ(set_alternative_proxy_server,
                 test_proxy_delegate.alternative_proxy_server().is_quic());
@@ -977,6 +964,7 @@ TEST_F(HttpStreamFactoryTest, WithQUICAlternativeProxyMarkedAsBad) {
       // |socket_data_direct_first_request|. The second request should consume
       // data from |socket_data_direct_second_request|.
       for (size_t i = 0; i < 2; ++i) {
+        StreamRequestWaiter waiter;
         std::unique_ptr<HttpStreamRequest> request(
             session->http_stream_factory()->RequestStream(
                 request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
@@ -1072,7 +1060,6 @@ TEST_F(HttpStreamFactoryTest, WithQUICAlternativeProxyNotMarkedAsBad) {
     request_info.url = GURL("http://www.google.com");
 
     SSLConfig ssl_config;
-    StreamRequestWaiter waiter;
 
     EXPECT_TRUE(test_proxy_delegate.alternative_proxy_server().is_quic());
 
@@ -1080,6 +1067,7 @@ TEST_F(HttpStreamFactoryTest, WithQUICAlternativeProxyNotMarkedAsBad) {
     // |socket_data_proxy_main_job| and |socket_data_https_first|.
     // The second request should consume data from |socket_data_https_second|.
     for (size_t i = 0; i < 2; ++i) {
+      StreamRequestWaiter waiter;
       std::unique_ptr<HttpStreamRequest> request(
           session->http_stream_factory()->RequestStream(
               request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
@@ -2143,6 +2131,59 @@ TEST_F(HttpStreamFactoryTest, NewSpdySessionCloseIdleH2Sockets) {
   EXPECT_EQ(1, GetSpdySessionCount(session.get()));
 }
 
+// Regression test for crbug.com/706974.
+TEST_F(HttpStreamFactoryTest, TwoSpdyConnects) {
+  SpdySessionDependencies session_deps(ProxyService::CreateDirect());
+
+  MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING)};
+  std::vector<std::unique_ptr<SequencedSocketData>> providers;
+  SSLSocketDataProvider ssl_socket_data(ASYNC, OK);
+  ssl_socket_data.next_proto = kProtoHTTP2;
+  for (int i = 0; i < 2; i++) {
+    auto provider = base::MakeUnique<SequencedSocketData>(
+        reads, arraysize(reads), nullptr, 0);
+    provider->set_connect_data(MockConnect(ASYNC, OK));
+    session_deps.socket_factory->AddSocketDataProvider(provider.get());
+    providers.push_back(std::move(provider));
+    session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_socket_data);
+  }
+
+  std::unique_ptr<HttpNetworkSession> session(
+      SpdySessionDependencies::SpdyCreateSession(&session_deps));
+
+  HostPortPair host_port_pair("www.google.com", 443);
+
+  // Request two streams at once and make sure they use the same connection.

[Bence, 2017/03/31 16:21:27]

It's not obvious to me where you make sure that they use the same connection.

[xunjieli, 2017/03/31 17:45:26]

I asserted on the number of spdy session established
EXPECT_EQ(1, GetSpdySessionCount(session.get()));

Will that be enough?

+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+  request_info.load_flags = 0;
+
+  SSLConfig ssl_config;
+  StreamRequestWaiter waiter1;
+  StreamRequestWaiter waiter2;
+  std::unique_ptr<HttpStreamRequest> request1(
+      session->http_stream_factory()->RequestStream(
+          request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter1,
+          /* enable_ip_based_pooling = */ true, NetLogWithSource()));
+  std::unique_ptr<HttpStreamRequest> request2(
+      session->http_stream_factory()->RequestStream(
+          request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter2,
+          /* enable_ip_based_pooling = */ true, NetLogWithSource()));
+  waiter1.WaitForStream();
+  waiter2.WaitForStream();
+  EXPECT_TRUE(waiter1.stream_done());
+  EXPECT_TRUE(waiter2.stream_done());
+  ASSERT_NE(nullptr, waiter1.stream());
+  ASSERT_NE(nullptr, waiter2.stream());
+  ASSERT_NE(waiter1.stream(), waiter2.stream());
+
+  // Establishing the SpdySession will close the extra H2 socket.
+  EXPECT_EQ(0, session->GetSSLSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL)
+                   ->IdleSocketCount());
+  EXPECT_EQ(1, GetSpdySessionCount(session.get()));
+}
+
 TEST_F(HttpStreamFactoryTest, RequestBidirectionalStreamImpl) {
   SpdySessionDependencies session_deps(ProxyService::CreateDirect());