Cancel any pending ResumeMainJob task

When we restart jobs with a different proxy configuration, we might have
a delayed ResumeMainJob task in our task queue. When this ResumeMainJob() runs,
we will crash because the old main job is deleted and we have a new main
job.

This CL cancels the ResumeMainJob task and adds regression test.

Bug: 790776, 789560
Change-Id: Icd2bc9ec36c98c973f80e14541d9f68d0a046835
Reviewed-on: https://chromium-review.googlesource.com/805354
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521444}

net/http/http_stream_factory_impl_job_controller.cc

@@ -559,17 +559,19 @@ void HttpStreamFactoryImpl::JobController::ResumeMainJobLater(
     const base::TimeDelta& delay) {
   net_log_.AddEvent(NetLogEventType::HTTP_STREAM_JOB_DELAYED,
                     NetLog::Int64Callback("delay", delay.InMilliseconds()));
+  resume_main_job_callback_.Reset(
+      base::BindOnce(&HttpStreamFactoryImpl::JobController::ResumeMainJob,
+                     ptr_factory_.GetWeakPtr()));
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&HttpStreamFactoryImpl::JobController::ResumeMainJob,
-                 ptr_factory_.GetWeakPtr()),
-      delay);
+      FROM_HERE, resume_main_job_callback_.callback(), delay);
 }
 
 void HttpStreamFactoryImpl::JobController::ResumeMainJob() {
-  if (main_job_is_resumed_ || !main_job_)
-    return;
+  DCHECK(main_job_);
 
+  if (main_job_is_resumed_) {
+    return;
+  }
   main_job_is_resumed_ = true;
   main_job_->net_log().AddEvent(
       NetLogEventType::HTTP_STREAM_JOB_RESUMED,
@@ -1316,10 +1318,15 @@ int HttpStreamFactoryImpl::JobController::ReconsiderProxyAfterError(Job* job,
     // Abandon all Jobs and start over.
     job_bound_ = false;
     bound_job_ = nullptr;
-    main_job_is_resumed_ = false;
-    main_job_is_blocked_ = false;
     alternative_job_.reset();
     main_job_.reset();
+    // Also resets states that related to the old main job. In particular,
+    // cancels |resume_main_job_callback_| so there won't be any delayed
+    // ResumeMainJob() left in the task queue.
+    resume_main_job_callback_.Cancel();
+    main_job_is_resumed_ = false;
+    main_job_is_blocked_ = false;
+
     next_state_ = STATE_RESOLVE_PROXY_COMPLETE;
   } else {
     // If ReconsiderProxyAfterError() failed synchronously, it means

net/http/http_stream_factory_impl_job_controller.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/cancelable_callback.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/privacy_mode.h"
 #include "net/http/http_stream_factory_impl_job.h"
@@ -356,6 +357,8 @@ class HttpStreamFactoryImpl::JobController
   // job must not create a connection until it is resumed.
   bool main_job_is_blocked_;
 
+  // Handle for cancelling any posted delayed ResumeMainJob() task.
+  base::CancelableOnceClosure resume_main_job_callback_;
   // True if the main job was blocked and has been resumed in ResumeMainJob().
   bool main_job_is_resumed_;
 

net/http/http_stream_factory_impl_job_controller_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/test/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_mock_time_message_loop_task_runner.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/threading/platform_thread.h"
 #include "net/base/test_proxy_delegate.h"
 #include "net/dns/mock_host_resolver.h"
@@ -39,6 +40,7 @@
 #include "net/quic/test_tools/mock_random.h"
 #include "net/socket/socket_test_util.h"
 #include "net/spdy/chromium/spdy_test_util_common.h"
+#include "net/test/net_test_suite.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gmock_mutant.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -150,6 +152,10 @@ class HttpStreamFactoryImplJobPeer {
       const HttpStreamFactoryImpl::Job* job) {
     return job->spdy_session_key_;
   }
+
+  static void SetShouldReconsiderProxy(HttpStreamFactoryImpl::Job* job) {
+    job->should_reconsider_proxy_ = true;
+  }
 };
 
 class JobControllerPeer {
@@ -1427,6 +1433,82 @@ TEST_F(HttpStreamFactoryImplJobControllerTest, DelayedTCP) {
   EXPECT_FALSE(job_controller_->alternative_job());
 }
 
+// Regression test for crbug.com/789560.
+TEST_F(HttpStreamFactoryImplJobControllerTest, ResumeMainJobLaterCanceled) {
+  NetTestSuite::SetScopedTaskEnvironment(
+      base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
+  std::unique_ptr<ProxyService> proxy_service = ProxyService::CreateDirect();
+  ProxyService* proxy_service_raw = proxy_service.get();
+  session_deps_.proxy_service = std::move(proxy_service);
+
+  // Using hanging resolver will cause the alternative job to hang indefinitely.
+  session_deps_.host_resolver = std::make_unique<HangingResolver>();
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  Initialize(request_info);
+
+  // Enable delayed TCP and set time delay for waiting job.
+  QuicStreamFactory* quic_stream_factory = session_->quic_stream_factory();
+  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);
+
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(kProtoQUIC, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_ =
+      job_controller_->Start(&request_delegate_, nullptr, net_log_.bound(),
+                             HttpStreamRequest::HTTP_STREAM, DEFAULT_PRIORITY);
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+  EXPECT_TRUE(job_controller_->main_job()->is_waiting());
+
+  base::RunLoop run_loop;
+  // The main job should be resumed without delay when alt job fails.
+  EXPECT_CALL(*job_factory_.main_job(), Resume())
+      .Times(1)
+      .WillOnce(Invoke([&run_loop]() { run_loop.Quit(); }));
+  job_controller_->OnStreamFailed(job_factory_.alternative_job(),
+                                  ERR_QUIC_PROTOCOL_ERROR, SSLConfig());
+  NetTestSuite::GetScopedTaskEnvironment()->FastForwardBy(
+      base::TimeDelta::FromMicroseconds(0));
+  run_loop.Run();
+  EXPECT_FALSE(job_controller_->alternative_job());
+
+  // Calling ForceReloadProxyConfig will cause the proxy configuration to
+  // change. It will still be the direct connection but the configuration
+  // version will be bumped. That is enough for the job controller to restart
+  // the jobs.
+  proxy_service_raw->ForceReloadProxyConfig();
+  HttpStreamFactoryImplJobPeer::SetShouldReconsiderProxy(
+      job_factory_.main_job());
+  // Now the alt service is marked as broken (e.g. through a different request),
+  // so only non-alt job is restarted.
+  session_->http_server_properties()->MarkAlternativeServiceBroken(
+      alternative_service);
+
+  job_controller_->OnStreamFailed(job_factory_.main_job(), ERR_FAILED,
+                                  SSLConfig());
+  // Jobs are restarted.
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_FALSE(job_controller_->alternative_job());
+
+  // There shouldn't be any ResumeMainJobLater() delayed tasks.
+  // This EXPECT_CALL will fail before crbug.com/789560 fix.
+  EXPECT_CALL(*job_factory_.main_job(), Resume()).Times(0);
+  NetTestSuite::GetScopedTaskEnvironment()->FastForwardBy(
+      base::TimeDelta::FromMicroseconds(15));
+
+  EXPECT_TRUE(job_controller_->main_job());
+  request_.reset();
+}
+
 // Test that main job is blocked for kMaxDelayTimeForMainJob(3s) if
 // http_server_properties cached an inappropriate large srtt for the server,
 // which would potentially delay the main job for a extremely long time in

net/test/net_test_suite.cc

@@ -5,7 +5,6 @@
 #include "net/test/net_test_suite.h"
 
 #include "base/logging.h"
-#include "base/test/scoped_task_environment.h"
 #include "net/base/network_change_notifier.h"
 #include "net/http/http_stream_factory.h"
 #include "net/spdy/chromium/spdy_session.h"
@@ -52,6 +51,13 @@ base::test::ScopedTaskEnvironment* NetTestSuite::GetScopedTaskEnvironment() {
   return g_current_net_test_suite->scoped_task_environment_.get();
 }
 
+void NetTestSuite::SetScopedTaskEnvironment(
+    base::test::ScopedTaskEnvironment::MainThreadType type) {
+  g_current_net_test_suite->scoped_task_environment_ = nullptr;
+  g_current_net_test_suite->scoped_task_environment_ =
+      std::make_unique<base::test::ScopedTaskEnvironment>(type);
+}
+
 void NetTestSuite::InitializeTestThread() {
   network_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock());
 

net/test/net_test_suite.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/memory/ref_counted.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/test/test_suite.h"
 #include "build/build_config.h"
 #include "net/dns/mock_host_resolver.h"
@@ -35,6 +36,12 @@ class NetTestSuite : public base::TestSuite {
   // NetTestSuite.
   static base::test::ScopedTaskEnvironment* GetScopedTaskEnvironment();
 
+  // Sets the global ScopedTaskEnvironment with a new environment of main thread
+  // type, |type|. For example, one might want to use a MOCK_TIME
+  // ScopedTaskEnvironment.
+  static void SetScopedTaskEnvironment(
+      base::test::ScopedTaskEnvironment::MainThreadType type);
+
  protected:
   // Called from within Initialize(), but separate so that derived classes
   // can initialize the NetTestSuite instance only and not