Resume the main job immediately when QUIC job fails, not wait for the head start timer to exprie

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 577363bb2c64894eae0941a9a0e2c28fa5c870ad..d7a836e84c05adeb420281328d8440e0387b04b2 100644
--- a/net/http/http_stream_factory_impl_job_controller_unittest.cc
+++ b/net/http/http_stream_factory_impl_job_controller_unittest.cc
@@ -918,15 +918,82 @@ TEST_F(HttpStreamFactoryImplJobControllerTest, DelayedTCP) {
 
   // The alternative job stalls as host resolution hangs when creating the QUIC
   // request and controller should resume the main job after delay.
-  // Verify the waiting time for delayed main job.
+  base::RunLoop run_loop;
+  EXPECT_CALL(*job_factory_.main_job(), Resume())
+      .Times(1)
+      .WillOnce(testing::DoAll(
+          testing::Invoke(testing::CreateFunctor(
+              &JobControllerPeer::VerifyWaitingTimeForMainJob, job_controller_,
+              base::TimeDelta::FromMicroseconds(15))),
+          testing::Invoke([&run_loop]() { run_loop.Quit(); })));
+
+  // Wait for the main job to be resumed.
+  run_loop.Run();
+
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  // |alternative_job| fails but should not report status to Request.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _)).Times(0);
+
+  EXPECT_FALSE(JobControllerPeer::main_job_is_blocked(job_controller_));
+  // OnStreamFailed will not resume the main job again since it's been resumed
+  // already.
+  EXPECT_CALL(*job_factory_.main_job(), Resume()).Times(0);
+
+  job_controller_->OnStreamFailed(job_factory_.alternative_job(),
+                                  ERR_NETWORK_CHANGED, SSLConfig());
+}

[Ryan Hamilton, 2017/02/15 00:52:08]

Does this old test fail with the new code (And so you've needed to change the
test) or are these change to make the test better by testing more conditions?

[Zhongyi Shi, 2017/02/15 01:39:08]

The old test won't fail with the new code. I just added more testing to this
one, so in this test we'll have an extra call to OnStreamFailed when the main
job is actually resumed(Resume is verified to be invoked on line 923) should not
resume the job again(expectation on line 942).

+
+TEST_F(HttpStreamFactoryImplJobControllerTest,
+       ResumeMainJobImmediatelyOnStreamFailed) {

[Ryan Hamilton, 2017/02/15 00:52:08]

I assume this fails with the old code?

[Zhongyi Shi, 2017/02/15 01:39:09]

It'll, this one now expects the main job to be resumed with main_job_wait_time =
0.

+  HangingResolver* resolver = new HangingResolver();
+  session_deps_.host_resolver.reset(resolver);
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  Initialize(request_info, false, false);
+
+  // Enable delayed TCP and set time delay for waiting job.
+  QuicStreamFactory* quic_stream_factory = session_->quic_stream_factory();
+  test::QuicStreamFactoryPeer::SetDelayTcpRace(quic_stream_factory, true);
+  quic_stream_factory->set_require_confirmation(false);
+  ServerNetworkStats stats1;
+  stats1.srtt = base::TimeDelta::FromMicroseconds(10);
+  session_->http_server_properties()->SetServerNetworkStats(
+      url::SchemeHostPort(GURL("https://www.google.com")), stats1);
+
+  // Set a SPDY alternative service for the server.
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(kProtoQUIC, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             NetLogWithSource(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  // |alternative_job| fails but should not report status to Request.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _)).Times(0);
+
+  // The alternative job stalls as host resolution hangs when creating the QUIC
+  // request and controller should resume the main job with delay.
+  // OnStreamFailed should resume the main job immediately.
   EXPECT_CALL(*job_factory_.main_job(), Resume())
+      .Times(1)
       .WillOnce(Invoke(testing::CreateFunctor(
           &JobControllerPeer::VerifyWaitingTimeForMainJob, job_controller_,
-          base::TimeDelta::FromMicroseconds(15))));
+          base::TimeDelta::FromMicroseconds(0))));

[Ryan Hamilton, 2017/02/15 00:52:08]

How does this verify that the main job is started? (Does it check
main_job_wait_time_ or actually that the main job resumes?)

[Zhongyi Shi, 2017/02/15 01:39:06]

This method check Resume() is called on main_job. I have added expectations on
main job to check is_waiting() is true so that we could verify the main job
starts.

+
+  job_controller_->OnStreamFailed(job_factory_.alternative_job(),
+                                  ERR_NETWORK_CHANGED, SSLConfig());
 
   base::RunLoop().RunUntilIdle();
 }
-
 // Verifies that the alternative proxy server job is not created if the URL
 // scheme is HTTPS.
 TEST_F(HttpStreamFactoryImplJobControllerTest, HttpsURL) {