JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs.

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 bd94d7ddc3fa89cdb49f8f5847f58e79e3994577..5355a4b7431f042457d9de01d69a7e40d6306b5b 100644
--- a/net/http/http_stream_factory_impl_job_controller_unittest.cc
+++ b/net/http/http_stream_factory_impl_job_controller_unittest.cc
@@ -6,13 +6,19 @@
 
 #include <memory>
 
+#include "base/run_loop.cc"
+#include "net/dns/mock_host_resolver.h"
 #include "net/http/http_basic_stream.h"
 #include "net/http/http_stream_factory_impl_request.h"
 #include "net/http/http_stream_factory_test_util.h"
+#include "net/proxy/mock_proxy_resolver.h"
+#include "net/proxy/proxy_config_service_fixed.h"
 #include "net/proxy/proxy_info.h"
 #include "net/proxy/proxy_service.h"
+#include "net/quic/test_tools/quic_stream_factory_peer.h"
 #include "net/spdy/spdy_test_util_common.h"
 #include "net/ssl/ssl_failure_state.h"
+#include "testing/gmock_mutant.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
@@ -28,8 +34,117 @@ void DeleteHttpStreamPointer(const SSLConfig& used_ssl_config,
   delete stream;
 }
 
+class HangingProxyResolver : public ProxyResolver {
+ public:
+  HangingProxyResolver() {}
+  ~HangingProxyResolver() override {}
+
+  int GetProxyForURL(const GURL& url,
+                     ProxyInfo* results,
+                     const CompletionCallback& callback,
+                     RequestHandle* request,
+                     const BoundNetLog& net_log) override {
+    return ERR_IO_PENDING;
+  }
+
+  void CancelRequest(RequestHandle request) override { NOTREACHED(); }
+
+  LoadState GetLoadState(RequestHandle request) const override {
+    NOTREACHED();
+    return LOAD_STATE_IDLE;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(HangingProxyResolver);
+};
+
+class HangingProxyResolverFactory : public ProxyResolverFactory {
+ public:
+  explicit HangingProxyResolverFactory(HangingProxyResolver* resolver)
+      : ProxyResolverFactory(false), resolver_(resolver) {}
+
+  // ProxyResolverFactory override.
+  int CreateProxyResolver(
+      const scoped_refptr<ProxyResolverScriptData>& pac_script,
+      std::unique_ptr<ProxyResolver>* resolver,
+      const net::CompletionCallback& callback,
+      std::unique_ptr<Request>* request) override {
+    resolver->reset(new ForwardingProxyResolver(resolver_));
+    return OK;
+  }
+
+ private:
+  HangingProxyResolver* resolver_;
+};
+
+class FailingProxyResolverFactory : public ProxyResolverFactory {
+ public:
+  FailingProxyResolverFactory() : ProxyResolverFactory(false) {}
+
+  // ProxyResolverFactory override.
+  int CreateProxyResolver(
+      const scoped_refptr<ProxyResolverScriptData>& script_data,
+      std::unique_ptr<ProxyResolver>* result,
+      const CompletionCallback& callback,
+      std::unique_ptr<Request>* request) override {
+    return ERR_PAC_SCRIPT_FAILED;
+  }
+};
+
+class FailingHostResolver : public MockHostResolverBase {
+ public:
+  FailingHostResolver() : MockHostResolverBase(false /*use_caching*/) {}
+  ~FailingHostResolver() override {}
+
+  int Resolve(const RequestInfo& info,
+              RequestPriority priority,
+              AddressList* addresses,
+              const CompletionCallback& callback,
+              RequestHandle* out_req,
+              const BoundNetLog& net_log) override {
+    return ERR_NAME_NOT_RESOLVED;
+  }
+};
+
+class HangingResolver : public MockHostResolverBase {
+ public:
+  HangingResolver() : MockHostResolverBase(false /*use_caching*/) {}
+  ~HangingResolver() override {}
+
+  int Resolve(const RequestInfo& info,
+              RequestPriority priority,
+              AddressList* addresses,
+              const CompletionCallback& callback,
+              RequestHandle* out_req,
+              const BoundNetLog& net_log) override {
+    return ERR_IO_PENDING;
+    ;

[Ryan Hamilton, 2016/07/12 00:21:45]

nit: extra ;

[Zhongyi Shi, 2016/07/12 23:03:12]

whoops, sorry :(

+  }
+
+  void CancelRequest(RequestHandle req) override {}
+};
 }  // anonymous namespace
 
+class HttpStreamFactoryImplJobPeer {
+ public:
+  static void Start(HttpStreamFactoryImpl::Job* job,
+                    HttpStreamRequest::StreamType stream_type) {
+    // Start() is mocked for MockHttpStreamFactoryImplJob.
+    // This is the alternative method to invoke real Start() method on Job.
+    job->stream_type_ = stream_type;
+    job->StartInternal();
+  }
+};
+
+class JobControllerPeer {
+ public:
+  static void VerifyWaitingTimeForMainJob(
+      HttpStreamFactoryImpl::JobController* job_controller,
+      const base::TimeDelta& delay) {
+    EXPECT_EQ(delay, job_controller->main_job_wait_time_);
+  }
+};
+
 class HttpStreamFactoryImplJobControllerTest
     : public ::testing::Test,
       public ::testing::WithParamInterface<NextProto> {
@@ -37,6 +152,9 @@ class HttpStreamFactoryImplJobControllerTest
   HttpStreamFactoryImplJobControllerTest()
       : session_deps_(GetParam(), ProxyService::CreateDirect()) {
     session_deps_.enable_quic = true;
+  }
+
+  void Initialize() {
     session_ = SpdySessionDependencies::SpdyCreateSession(&session_deps_);
     factory_ =
         static_cast<HttpStreamFactoryImpl*>(session_->http_stream_factory());
@@ -73,6 +191,8 @@ INSTANTIATE_TEST_CASE_P(NextProto,
 
 TEST_P(HttpStreamFactoryImplJobControllerTest,
        OnStreamFailedWithNoAlternativeJob) {
+  Initialize();
+
   HttpRequestInfo request_info;
   request_info.method = "GET";
   request_info.url = GURL("http://www.google.com");
@@ -95,6 +215,8 @@ TEST_P(HttpStreamFactoryImplJobControllerTest,
 
 TEST_P(HttpStreamFactoryImplJobControllerTest,
        OnStreamReadyWithNoAlternativeJob) {
+  Initialize();
+
   HttpRequestInfo request_info;
   request_info.method = "GET";
   request_info.url = GURL("http://www.google.com");
@@ -120,6 +242,8 @@ TEST_P(HttpStreamFactoryImplJobControllerTest,
 // Test we cancel Jobs correctly when the Request is explicitly canceled
 // before any Job is bound to Request.
 TEST_P(HttpStreamFactoryImplJobControllerTest, CancelJobsBeforeBinding) {
+  Initialize();
+
   HttpRequestInfo request_info;
   request_info.method = "GET";
   request_info.url = GURL("https://www.google.com");
@@ -143,6 +267,8 @@ TEST_P(HttpStreamFactoryImplJobControllerTest, CancelJobsBeforeBinding) {
 }
 
 TEST_P(HttpStreamFactoryImplJobControllerTest, OnStreamFailedForBothJobs) {
+  Initialize();
+
   HttpRequestInfo request_info;
   request_info.method = "GET";
   request_info.url = GURL("https://www.google.com");
@@ -178,6 +304,8 @@ TEST_P(HttpStreamFactoryImplJobControllerTest, OnStreamFailedForBothJobs) {
 
 TEST_P(HttpStreamFactoryImplJobControllerTest,
        SecondJobFailsAfterFirstJobSucceeds) {
+  Initialize();
+
   HttpRequestInfo request_info;
   request_info.method = "GET";
   request_info.url = GURL("https://www.google.com");
@@ -220,6 +348,8 @@ TEST_P(HttpStreamFactoryImplJobControllerTest,
 
 TEST_P(HttpStreamFactoryImplJobControllerTest,
        SecondJobSucceedsAfterFirstJobFailed) {
+  Initialize();
+
   HttpRequestInfo request_info;
   request_info.method = "GET";
   request_info.url = GURL("https://www.google.com");
@@ -258,6 +388,8 @@ TEST_P(HttpStreamFactoryImplJobControllerTest,
 // Regression test for crbug/621069.
 // Get load state after main job fails and before alternative job succeeds.
 TEST_P(HttpStreamFactoryImplJobControllerTest, GetLoadStateAfterMainJobFailed) {
+  Initialize();
+
   HttpRequestInfo request_info;
   request_info.method = "GET";
   request_info.url = GURL("https://www.google.com");
@@ -296,4 +428,300 @@ TEST_P(HttpStreamFactoryImplJobControllerTest, GetLoadStateAfterMainJobFailed) {
                                  ProxyInfo());
 }
 
+TEST_P(HttpStreamFactoryImplJobControllerTest, DoNotResumeMainJobBeforeWait) {
+  // Use failing ProxyResolverFactory which is unable to create ProxyResolver
+  // to stall the alternative job and report to controller to resume the main
+  // job.
+  ProxyConfig proxy_config;
+  proxy_config.set_auto_detect(true);
+  proxy_config.set_pac_mandatory(true);
+  session_deps_.proxy_service.reset(new ProxyService(
+      base::WrapUnique(new ProxyConfigServiceFixed(proxy_config)),
+      base::WrapUnique(new FailingProxyResolverFactory), nullptr));
+
+  Initialize();
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(QUIC, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), 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.
+  // Job controller should not resume the main job as it's not in wait state.
+  EXPECT_CALL(*job_factory_.main_job(), Resume()).Times(0);
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.alternative_job(),
+                                      HttpStreamRequest::HTTP_STREAM);
+
+  // Wait until OnStreamFailedCallback is executed on the alternative job.
+  EXPECT_CALL(*job_factory_.main_job(), MarkOtherJobComplete(_)).Times(1);

[Ryan Hamilton, 2016/07/12 00:21:45]

Is MarkOtherJobComplete going away in a future CL?

[Zhongyi Shi, 2016/07/12 23:03:12]

Yeah, this one is more for resume logic. It should be a simple change if
job_status_ is well taken care of. 

+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(job_controller_->alternative_job());
+
+  // |main_job| succeeds and should report status to Request.
+  HttpStream* http_stream =
+      new HttpBasicStream(new ClientSocketHandle(), false);
+  job_factory_.main_job()->SetStream(http_stream);
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
+      .WillOnce(Invoke(DeleteHttpStreamPointer));
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
+                                 ProxyInfo());
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       DoNotResumeMainJobWhenAltJobPending) {
+  ProxyConfig proxy_config;
+  proxy_config.set_auto_detect(true);
+  proxy_config.set_pac_url(GURL("http://fooproxyurl"));
+  HangingProxyResolver hanging_proxy_resolver;
+  session_deps_.proxy_service.reset(new ProxyService(
+      base::WrapUnique(new ProxyConfigServiceFixed(proxy_config)),
+      base::WrapUnique(
+          new HangingProxyResolverFactory(&hanging_proxy_resolver)),
+      nullptr));
+
+  Initialize();
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+
+  // Set a SPDY alternative service for the server.
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(NPN_HTTP_2, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  // The alternative job stalls but not failing and controller should
+  // not resume the main job.
+  EXPECT_CALL(*job_factory_.main_job(), Resume()).Times(0);
+
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.alternative_job(),
+                                      HttpStreamRequest::HTTP_STREAM);

[Ryan Hamilton, 2016/07/12 00:21:45]

Why do we need to use a Peer here?

[Zhongyi Shi, 2016/07/12 23:03:12]

Job::Start() is called by JobController::CreateJobs(). We have to mock it for
testing purpose so that we can test step by step. 

Then here we want to call the real Start() to test more of the resume logic.

+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest, InvalidPortToResumeMainJob) {
+  Initialize();
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  url::SchemeHostPort server(request_info.url);
+  // Using a restricted port 101 for QUIC should fail.
+  AlternativeService alternative_service(QUIC, server.host(), 101);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.main_job(),
+                                      HttpStreamRequest::HTTP_STREAM);
+  EXPECT_TRUE(job_factory_.main_job()->is_waiting());
+
+  // |alternative_job| fails but should not report status to Request.
+  EXPECT_CALL(*job_factory_.main_job(), Resume()).Times(1);
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.alternative_job(),
+                                      HttpStreamRequest::HTTP_STREAM);
+
+  // Wait until OnStreamFailedCallback is executed on the alternative job.
+  EXPECT_CALL(*job_factory_.main_job(), MarkOtherJobComplete(_)).Times(1);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(job_controller_->alternative_job());
+
+  // |main_job| succeeds and should report status to Request.
+  HttpStream* http_stream =
+      new HttpBasicStream(new ClientSocketHandle(), false);
+  job_factory_.main_job()->SetStream(http_stream);
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
+      .WillOnce(Invoke(DeleteHttpStreamPointer));
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
+                                 ProxyInfo());
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       NoAvailableQuicSessionToResumeMainJob) {
+  // Use failing HostResolver which is unable to resolve the host name for QUIC.
+  // No QUIC session is created and thus should resume the main job.
+  FailingHostResolver* resolver = new FailingHostResolver();
+  session_deps_.host_resolver.reset(resolver);
+
+  Initialize();
+  QuicStreamFactory* quic_stream_factory = session_->quic_stream_factory();
+  DCHECK(quic_stream_factory);
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(QUIC, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.main_job(),
+                                      HttpStreamRequest::HTTP_STREAM);
+
+  // |alternative_job| fails as host resolution fails and should resume the
+  // main job.
+  EXPECT_CALL(*job_factory_.main_job(), Resume()).Times(1);
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.alternative_job(),
+                                      HttpStreamRequest::HTTP_STREAM);
+
+  // Wait until OnStreamFailedCallback is executed on the alternative job.
+  // Request shouldn't be notified as the main job is still pending status.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _, _)).Times(0);
+  EXPECT_CALL(*job_factory_.main_job(), MarkOtherJobComplete(_)).Times(1);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(job_controller_->alternative_job());
+
+  // |main_job| succeeds and should report status to Request.
+  HttpStream* http_stream =
+      new HttpBasicStream(new ClientSocketHandle(), false);
+  job_factory_.main_job()->SetStream(http_stream);
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
+      .WillOnce(Invoke(DeleteHttpStreamPointer));
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
+                                 ProxyInfo());
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       NoAvailableSpdySessionToResumeMainJob) {
+  // Disable QUIC.
+  session_deps_.enable_quic = false;
+  // Use failing HostResolver which is unable to resolve the host name.
+  // No SPDY session is created and thus should resume the main job.
+  FailingHostResolver* resolver = new FailingHostResolver();
+  session_deps_.host_resolver.reset(resolver);
+
+  Initialize();
+  QuicStreamFactory* quic_stream_factory = session_->quic_stream_factory();
+  DCHECK(quic_stream_factory);
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+
+  // Set a SPDY alternative service for the server.
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(NPN_HTTP_2, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.main_job(),
+                                      HttpStreamRequest::HTTP_STREAM);
+
+  // |alternative_job| fails as host resolution fails and should resume the
+  // main job.
+  EXPECT_CALL(*job_factory_.main_job(), Resume()).Times(1);
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.alternative_job(),
+                                      HttpStreamRequest::HTTP_STREAM);
+
+  // Wait until OnStreamFailedCallback is executed on the alternative job.
+  // Request shouldn't be notified as the main job is still pending status.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _, _)).Times(0);
+  EXPECT_CALL(*job_factory_.main_job(), MarkOtherJobComplete(_)).Times(1);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(job_controller_->alternative_job());
+
+  // |main_job| succeeds and should report status to Request.
+  HttpStream* http_stream =
+      new HttpBasicStream(new ClientSocketHandle(), false);
+  job_factory_.main_job()->SetStream(http_stream);
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
+      .WillOnce(Invoke(DeleteHttpStreamPointer));
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
+                                 ProxyInfo());
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest, DelayedTCP) {
+  session_deps_.proxy_service.reset(
+      ProxyService::CreateFixedFromPacResult("QUIC mail.example.org:70")
+          .release());
+  HangingResolver* resolver = new HangingResolver();
+  session_deps_.host_resolver.reset(resolver);
+
+  Initialize();
+
+  // 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://mail.example.org:70/")), stats1);
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+
+  // Set a SPDY alternative service for the server.
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(NPN_HTTP_2, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  // Start the main job then wait.
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.main_job(),
+                                      HttpStreamRequest::HTTP_STREAM);

[Ryan Hamilton, 2016/07/12 00:21:45]

Can we avoid the peer by making the PAC resolution asyn (or perhaps host
resolution async?)

[Zhongyi Shi, 2016/07/12 23:03:12]

I thought you were saying use the asyn PAC resolver to prevent calling the
alternative_job.Start() before main_job.Start(). 

But this won't help us avoid the peer.

+
+  // 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.
+  EXPECT_CALL(*job_factory_.main_job(), Resume())
+      .WillOnce(Invoke(testing::CreateFunctor(
+          &JobControllerPeer::VerifyWaitingTimeForMainJob, job_controller_,
+          base::TimeDelta::FromMicroseconds(15))));
+
+  HttpStreamFactoryImplJobPeer::Start(job_factory_.alternative_job(),
+                                      HttpStreamRequest::HTTP_STREAM);
+  base::RunLoop().RunUntilIdle();
+}
 }  // namespace net