Throttle HttpStreamFactoryImpl::Job to HTTP/2-supported servers

net/http/http_stream_factory_impl_job_controller_unittest.cc

Index: net/http/http_stream_factory_impl_job_controller_unittest.cc
diff --git a/net/http/http_stream_factory_impl_job_controller_unittest.cc b/net/http/http_stream_factory_impl_job_controller_unittest.cc
index cf7e526a5b3887d6ba52a9caa4155609cd75340b..9e186661441088313c7803060db4c7ffe4dc4af8 100644
--- a/net/http/http_stream_factory_impl_job_controller_unittest.cc
+++ b/net/http/http_stream_factory_impl_job_controller_unittest.cc
@@ -167,12 +167,7 @@ class HttpStreamFactoryImplJobControllerTest : public ::testing::Test {
                       0,
                       &clock_,
                       kServerHostname,
-                      Perspective::IS_CLIENT),
-        use_alternative_proxy_(false),
-        is_preconnect_(false),
-        enable_ip_based_pooling_(true),
-        enable_alternative_services_(true),
-        test_proxy_delegate_(nullptr) {
+                      Perspective::IS_CLIENT) {
     session_deps_.enable_quic = true;
   }
 
@@ -196,6 +191,11 @@ class HttpStreamFactoryImplJobControllerTest : public ::testing::Test {
     enable_alternative_services_ = false;
   }
 
+  void SetSkipCreateJobController() {
+    ASSERT_FALSE(job_controller_);
+    skip_create_job_controller_ = true;
+  }
+
   void Initialize(const HttpRequestInfo& request_info) {
     ASSERT_FALSE(test_proxy_delegate_);
     auto test_proxy_delegate = base::MakeUnique<TestProxyDelegate>();
@@ -227,11 +227,13 @@ class HttpStreamFactoryImplJobControllerTest : public ::testing::Test {
     session_ = base::MakeUnique<HttpNetworkSession>(params, session_context);
     factory_ =
         static_cast<HttpStreamFactoryImpl*>(session_->http_stream_factory());
-    job_controller_ = new HttpStreamFactoryImpl::JobController(
-        factory_, &request_delegate_, session_.get(), &job_factory_,
-        request_info, is_preconnect_, enable_ip_based_pooling_,
-        enable_alternative_services_, SSLConfig(), SSLConfig());
-    HttpStreamFactoryImplPeer::AddJobController(factory_, job_controller_);
+    if (!skip_create_job_controller_) {

[Bence, 2017/06/09 19:17:39]

This feels like a double negation.  It might be less confusing to invert value
and rename to |create_job_controller_|, true by default.  (I prefer to avoid
|disable|, |do_not|, and similar negations in variable names for this reason.) 
Then the setter could be called SkipCreatingJobController() or
DoNotCreateJobController().

[xunjieli, 2017/06/13 15:23:11]

Done. Good idea.

+      job_controller_ = new HttpStreamFactoryImpl::JobController(
+          factory_, &request_delegate_, session_.get(), &job_factory_,
+          request_info, is_preconnect_, enable_ip_based_pooling_,
+          enable_alternative_services_, SSLConfig(), SSLConfig());
+      HttpStreamFactoryImplPeer::AddJobController(factory_, job_controller_);
+    }
   }
 
   TestProxyDelegate* test_proxy_delegate() const {
@@ -273,8 +275,8 @@ class HttpStreamFactoryImplJobControllerTest : public ::testing::Test {
   MockHttpStreamRequestDelegate request_delegate_;
   SpdySessionDependencies session_deps_;
   std::unique_ptr<HttpNetworkSession> session_;
-  HttpStreamFactoryImpl* factory_;
-  HttpStreamFactoryImpl::JobController* job_controller_;
+  HttpStreamFactoryImpl* factory_ = nullptr;
+  HttpStreamFactoryImpl::JobController* job_controller_ = nullptr;
   std::unique_ptr<HttpStreamFactoryImpl::Request> request_;
   std::unique_ptr<SequencedSocketData> tcp_data_;
   std::unique_ptr<test::MockQuicData> quic_data_;
@@ -284,14 +286,15 @@ class HttpStreamFactoryImplJobControllerTest : public ::testing::Test {
   test::QuicTestPacketMaker client_maker_;
 
  protected:
-  bool use_alternative_proxy_;
-  bool is_preconnect_;
-  bool enable_ip_based_pooling_;
-  bool enable_alternative_services_;
+  bool use_alternative_proxy_ = false;
+  bool is_preconnect_ = false;
+  bool enable_ip_based_pooling_ = true;
+  bool enable_alternative_services_ = true;
+  bool skip_create_job_controller_ = false;
 
  private:
   // Not owned by |this|.
-  TestProxyDelegate* test_proxy_delegate_;
+  TestProxyDelegate* test_proxy_delegate_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(HttpStreamFactoryImplJobControllerTest);
 };
@@ -1563,9 +1566,8 @@ TEST_F(HttpStreamFactoryImplJobControllerTest,
   request_info.url = GURL("http://www.example.com");
   Initialize(request_info);
 
-  url::SchemeHostPort server(request_info.url);
-
   // Sets server support Http/2.
+  url::SchemeHostPort server(request_info.url);
   session_->http_server_properties()->SetSupportsSpdy(server, true);
 
   job_controller_->Preconnect(/*num_streams=*/5);
@@ -1582,6 +1584,233 @@ TEST_F(HttpStreamFactoryImplJobControllerTest,
   EXPECT_TRUE(HttpStreamFactoryImplPeer::IsJobControllerDeleted(factory_));
 }
 
+class LimitMultipleH2Requests : public HttpStreamFactoryImplJobControllerTest {

[Bence, 2017/06/09 23:14:02]

Optional: add a test case where the server is thought to support HTTP/2 but in
fact negotiates HTTP/1.1, so the second request does not bind to the first
socket when resumed.

[xunjieli, 2017/06/13 15:23:11]

Good idea. Done.

+ protected:
+  const int kNumRequests = 5;
+  void SetUp() override { SetSkipCreateJobController(); }

[Bence, 2017/06/09 19:17:39]

Why not set |skip_create_job_controller_| directly, since it's protected anyway?
 Or if going down this route, maybe make |skip_create_job_controller_| private.

[xunjieli, 2017/06/13 15:23:11]

Done.

+};
+
+TEST_F(LimitMultipleH2Requests, MultipleRequests) {
+  // Make sure there are only one socket connect.

[Bence, 2017/06/09 19:17:39]

s/are/is/

[xunjieli, 2017/06/13 15:23:10]

Done.

+  MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING)};
+  tcp_data_ = base::MakeUnique<SequencedSocketData>(reads, arraysize(reads),
+                                                    nullptr, 0);
+  tcp_data_->set_connect_data(MockConnect(ASYNC, OK));
+  auto ssl_data = base::MakeUnique<SSLSocketDataProvider>(ASYNC, OK);
+  ssl_data->next_proto = kProtoHTTP2;
+  session_deps_.socket_factory->AddSSLSocketDataProvider(ssl_data.get());
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.example.com");
+  Initialize(request_info);
+  SpdySessionPoolPeer pool_peer(session_->spdy_session_pool());
+  pool_peer.SetEnableSendingInitialData(false);
+
+  // Sets server support Http/2.
+  url::SchemeHostPort server(request_info.url);
+  session_->http_server_properties()->SetSupportsSpdy(server, true);
+
+  std::vector<std::unique_ptr<MockHttpStreamRequestDelegate>> request_delegates;

[Bence, 2017/06/09 19:17:39]

Could you use std::vector<MockHttpStreamRequestDelegate> here?  emplace_back()
with no arguments below.

Same in other tests below.

[xunjieli, 2017/06/13 15:23:11]

Acknowledged. MockHttpStreamRequestDelegate has no copy constructor. std::vector
requires type T to have a copy constructor.
https://stackoverflow.com/questions/40457302/c-vector-emplace-back-calls-copy...

+  std::vector<std::unique_ptr<HttpStreamRequest>> requests;

[Bence, 2017/06/09 19:17:39]

I understand that this needs to use unique_ptr because that's what
JobController::Start returns.

[xunjieli, 2017/06/13 15:23:11]

Acknowledged.

+  for (int i = 0; i < kNumRequests; ++i) {
+    request_delegates.emplace_back(

[Bence, 2017/06/09 19:17:39]

I think a simple push_back() would be enough here.  MakeUnique already
constructs the unique_ptr instance, so a move is required even when using
emplace_back() (it will be the move constructor).  I don't think there is a
performance benefit.  See also comments at https://chromium-cpp.appspot.com/.

[xunjieli, 2017/06/13 15:23:10]

Done.

+        base::MakeUnique<MockHttpStreamRequestDelegate>());
+    HttpStreamFactoryImpl::JobController* job_controller =
+        new HttpStreamFactoryImpl::JobController(
+            factory_, request_delegates[i].get(), session_.get(), &job_factory_,
+            request_info, is_preconnect_, enable_ip_based_pooling_,
+            enable_alternative_services_, SSLConfig(), SSLConfig());
+    HttpStreamFactoryImplPeer::AddJobController(factory_, job_controller);
+    auto request = job_controller->Start(
+        request_delegates[i].get(), nullptr, NetLogWithSource(),
+        HttpStreamRequest::HTTP_STREAM, DEFAULT_PRIORITY);
+    EXPECT_TRUE(job_controller->main_job());
+    EXPECT_FALSE(job_controller->alternative_job());
+    requests.push_back(std::move(request));
+  }
+
+  for (int i = 0; i < kNumRequests; ++i) {
+    EXPECT_CALL(*request_delegates[i].get(), OnStreamReadyImpl(_, _, _));
+  }
+
+  base::RunLoop().RunUntilIdle();
+  requests.clear();
+  EXPECT_TRUE(HttpStreamFactoryImplPeer::IsJobControllerDeleted(factory_));
+}
+
+TEST_F(LimitMultipleH2Requests, MultipleRequestsFirstRequestHang) {
+  base::ScopedMockTimeMessageLoopTaskRunner test_task_runner;
+  // First socket connect hang.
+  auto hangdata = base::MakeUnique<SequencedSocketData>(nullptr, 0, nullptr, 0);
+  hangdata->set_connect_data(MockConnect(SYNCHRONOUS, ERR_IO_PENDING));
+  session_deps_.socket_factory->AddSocketDataProvider(hangdata.get());
+  MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING)};
+  std::vector<std::unique_ptr<SequencedSocketData>> socket_data;

[Bence, 2017/06/09 19:17:39]

Hm, could you not just use std::vector<SequencedSocketData>, emplace_back()
below, and access it through vector::back()?  Not sure if that would hurt
readability, but I usually try to avoid extra allocations and indirections.

[xunjieli, 2017/06/13 15:23:11]

Same here. SequencedSocketData has DISALLOW_COPY_AND_ASSIGN so the copy
constructor is deleted. std::vector<> requires the type T to have a copy
constructor.

+  std::vector<std::unique_ptr<SSLSocketDataProvider>> ssl_socket_data;

[Bence, 2017/06/09 19:17:39]

Same here.

[xunjieli, 2017/06/13 15:23:11]

Acknowledged.

+  // kNumRequests - 1 will resume themselves after a delay. There will be
+  // kNumRequests - 1 sockets opened.
+  for (int i = 0; i < kNumRequests - 1; i++) {
+    // Only the first one needs a MockRead because subsequent sockets are
+    // not used to establish a SpdySession.
+    auto tcp_data =
+        i == 0 ? base::MakeUnique<SequencedSocketData>(reads, arraysize(reads),
+                                                       nullptr, 0)
+               : base::MakeUnique<SequencedSocketData>(nullptr, 0, nullptr, 0);
+    tcp_data->set_connect_data(MockConnect(ASYNC, OK));
+    auto ssl_data = base::MakeUnique<SSLSocketDataProvider>(ASYNC, OK);
+    ssl_data->next_proto = kProtoHTTP2;
+    session_deps_.socket_factory->AddSocketDataProvider(tcp_data.get());
+    session_deps_.socket_factory->AddSSLSocketDataProvider(ssl_data.get());
+    socket_data.push_back(std::move(tcp_data));
+    ssl_socket_data.push_back(std::move(ssl_data));
+  }
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.example.com");
+  SetSkipCreateJobController();

[Bence, 2017/06/09 19:17:39]

Please remove this call, as SetUp() already takes care of it.

[xunjieli, 2017/06/13 15:23:10]

Done.

+  Initialize(request_info);
+  SpdySessionPoolPeer pool_peer(session_->spdy_session_pool());
+  pool_peer.SetEnableSendingInitialData(false);
+
+  // Sets server support Http/2.
+  url::SchemeHostPort server(request_info.url);
+  session_->http_server_properties()->SetSupportsSpdy(server, true);
+
+  std::vector<std::unique_ptr<MockHttpStreamRequestDelegate>> request_delegates;
+  std::vector<std::unique_ptr<HttpStreamRequest>> requests;
+  for (int i = 0; i < kNumRequests; ++i) {
+    request_delegates.emplace_back(
+        base::MakeUnique<MockHttpStreamRequestDelegate>());
+    HttpStreamFactoryImpl::JobController* job_controller =
+        new HttpStreamFactoryImpl::JobController(
+            factory_, request_delegates[i].get(), session_.get(), &job_factory_,
+            request_info, is_preconnect_, enable_ip_based_pooling_,
+            enable_alternative_services_, SSLConfig(), SSLConfig());
+    HttpStreamFactoryImplPeer::AddJobController(factory_, job_controller);
+    auto request = job_controller->Start(
+        request_delegates[i].get(), nullptr, NetLogWithSource(),
+        HttpStreamRequest::HTTP_STREAM, DEFAULT_PRIORITY);
+    EXPECT_TRUE(job_controller->main_job());
+    EXPECT_FALSE(job_controller->alternative_job());
+    requests.push_back(std::move(request));
+  }
+
+  for (int i = 0; i < kNumRequests; ++i) {
+    // Delete |request[i] when request completes is needed because

[Bence, 2017/06/09 19:17:39]

s/request/requests/

[Bence, 2017/06/09 19:17:39]

Nit: unmatched '|'.

[xunjieli, 2017/06/13 15:23:10]

Done.

[xunjieli, 2017/06/13 15:23:11]

Done.

+    // otherwise request will be completed twice. See crbug.com/706974.
+    EXPECT_CALL(*request_delegates[i].get(), OnStreamReadyImpl(_, _, _))
+        .WillOnce(testing::InvokeWithoutArgs(
+            [this, &requests, i]() { requests[i].reset(); }));
+  }
+
+  EXPECT_TRUE(test_task_runner->HasPendingTask());
+  test_task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(300));

[Bence, 2017/06/09 19:17:39]

Do you not need to use a larger time here to make sure callbacks are actually
completed?  I am worried about off-by-one errors due to rounding of internal
representations, or due to inconsistent use of < vs. <= in internal
implementation.

[xunjieli, 2017/06/13 15:23:11]

Probably not. The delayed task is posted when |job_controller| is started, so we
are sure that the delayed task is run if we fast forward the test task runner by
300ms. 

[Bence, 2017/06/13 16:04:51]

Acknowledged.

+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(HttpStreamFactoryImplPeer::IsJobControllerDeleted(factory_));
+  for (const auto& data : socket_data) {
+    EXPECT_TRUE(data->AllReadDataConsumed());
+    EXPECT_TRUE(data->AllWriteDataConsumed());
+  }
+}
+
+TEST_F(LimitMultipleH2Requests, MultipleRequestsFirstRequestCanceled) {
+  MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING)};
+  auto first_socket = base::MakeUnique<SequencedSocketData>(
+      reads, arraysize(reads), nullptr, 0);
+  first_socket->set_connect_data(MockConnect(ASYNC, OK));
+  auto ssl_data = base::MakeUnique<SSLSocketDataProvider>(ASYNC, OK);
+  ssl_data->next_proto = kProtoHTTP2;
+  session_deps_.socket_factory->AddSocketDataProvider(first_socket.get());
+  session_deps_.socket_factory->AddSSLSocketDataProvider(ssl_data.get());
+  auto second_socket =
+      base::MakeUnique<SequencedSocketData>(nullptr, 0, nullptr, 0);
+  second_socket->set_connect_data(MockConnect(ASYNC, OK));
+  session_deps_.socket_factory->AddSocketDataProvider(second_socket.get());
+  session_deps_.socket_factory->AddSSLSocketDataProvider(ssl_data.get());
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.example.com");
+  SetSkipCreateJobController();

[Bence, 2017/06/09 19:17:39]

Please remove this call, as SetUp() already takes care of it.

[xunjieli, 2017/06/13 15:23:11]

Done.

+  Initialize(request_info);
+  SpdySessionPoolPeer pool_peer(session_->spdy_session_pool());
+  pool_peer.SetEnableSendingInitialData(false);
+
+  // Sets server support Http/2.
+  url::SchemeHostPort server(request_info.url);
+  session_->http_server_properties()->SetSupportsSpdy(server, true);
+
+  std::vector<std::unique_ptr<MockHttpStreamRequestDelegate>> request_delegates;
+  std::vector<std::unique_ptr<HttpStreamRequest>> requests;
+  for (int i = 0; i < kNumRequests; ++i) {
+    request_delegates.emplace_back(
+        base::MakeUnique<MockHttpStreamRequestDelegate>());
+    HttpStreamFactoryImpl::JobController* job_controller =
+        new HttpStreamFactoryImpl::JobController(
+            factory_, request_delegates[i].get(), session_.get(), &job_factory_,
+            request_info, is_preconnect_, enable_ip_based_pooling_,
+            enable_alternative_services_, SSLConfig(), SSLConfig());
+    HttpStreamFactoryImplPeer::AddJobController(factory_, job_controller);
+    auto request = job_controller->Start(
+        request_delegates[i].get(), nullptr, NetLogWithSource(),
+        HttpStreamRequest::HTTP_STREAM, DEFAULT_PRIORITY);
+    EXPECT_TRUE(job_controller->main_job());
+    EXPECT_FALSE(job_controller->alternative_job());
+    requests.push_back(std::move(request));
+  }
+  // Cancel the first one.
+  requests[0].reset();
+
+  for (int i = 1; i < kNumRequests; ++i) {
+    EXPECT_CALL(*request_delegates[i].get(), OnStreamReadyImpl(_, _, _));
+  }
+  base::RunLoop().RunUntilIdle();
+  requests.clear();
+  EXPECT_TRUE(HttpStreamFactoryImplPeer::IsJobControllerDeleted(factory_));
+  EXPECT_TRUE(first_socket->AllReadDataConsumed());
+  EXPECT_TRUE(second_socket->AllReadDataConsumed());
+}
+
+TEST_F(LimitMultipleH2Requests, MultiplePreconnects) {
+  // Make sure there are only one socket connect.

[Bence, 2017/06/09 19:17:38]

s/are/is.

[xunjieli, 2017/06/13 15:23:11]

Done.

+  tcp_data_ = base::MakeUnique<SequencedSocketData>(nullptr, 0, nullptr, 0);
+  tcp_data_->set_connect_data(MockConnect(ASYNC, OK));
+  auto ssl_data = base::MakeUnique<SSLSocketDataProvider>(ASYNC, OK);
+  ssl_data->next_proto = kProtoHTTP2;
+  session_deps_.socket_factory->AddSSLSocketDataProvider(ssl_data.get());
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.example.com");
+  SetPreconnect();
+  Initialize(request_info);
+  SpdySessionPoolPeer pool_peer(session_->spdy_session_pool());
+  pool_peer.SetEnableSendingInitialData(false);

[Bence, 2017/06/09 19:17:39]

Turns out these two lines are not necessary for this test (but it is for the
other ones), please remove them.

[xunjieli, 2017/06/13 15:23:10]

Done. Good catch.

+
+  // Sets server support Http/2.
+  url::SchemeHostPort server(request_info.url);
+  session_->http_server_properties()->SetSupportsSpdy(server, true);
+
+  std::vector<std::unique_ptr<MockHttpStreamRequestDelegate>> request_delegates;
+  std::vector<std::unique_ptr<HttpStreamRequest>> requests;
+  for (int i = 0; i < kNumRequests; ++i) {
+    request_delegates.emplace_back(
+        base::MakeUnique<MockHttpStreamRequestDelegate>());
+    HttpStreamFactoryImpl::JobController* job_controller =
+        new HttpStreamFactoryImpl::JobController(
+            factory_, request_delegates[i].get(), session_.get(), &job_factory_,
+            request_info, is_preconnect_, enable_ip_based_pooling_,
+            enable_alternative_services_, SSLConfig(), SSLConfig());
+    HttpStreamFactoryImplPeer::AddJobController(factory_, job_controller);
+    job_controller->Preconnect(1);
+    EXPECT_TRUE(job_controller->main_job());
+    EXPECT_FALSE(job_controller->alternative_job());
+  }
+  base::RunLoop().RunUntilIdle();
+  requests.clear();
+  EXPECT_TRUE(HttpStreamFactoryImplPeer::IsJobControllerDeleted(factory_));
+}
+
 class HttpStreamFactoryImplJobControllerMisdirectedRequestRetry
     : public HttpStreamFactoryImplJobControllerTest,
       public ::testing::WithParamInterface<::testing::tuple<bool, bool>> {};