JobController 2: Remove reference between HttpStreamFactoryImpl::Jobs.

net/http/http_stream_factory_impl_job.cc

Index: net/http/http_stream_factory_impl_job.cc
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index 6c2a9018b942e82433c2867d73f5befb4ef9cf8b..fa9d4ea97d79f47a307ee1f0cd59d5575e75e54b 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -156,6 +156,7 @@ std::unique_ptr<base::Value> NetLogHttpStreamProtoCallback(
 }
 
 HttpStreamFactoryImpl::Job::Job(JobController* job_controller,
+                                JobType job_type,
                                 HttpNetworkSession* session,
                                 const HttpRequestInfo& request_info,
                                 RequestPriority priority,
@@ -165,6 +166,7 @@ HttpStreamFactoryImpl::Job::Job(JobController* job_controller,
                                 GURL origin_url,
                                 NetLog* net_log)
     : Job(job_controller,
+          job_type,
           session,
           request_info,
           priority,
@@ -176,6 +178,7 @@ HttpStreamFactoryImpl::Job::Job(JobController* job_controller,
           net_log) {}
 
 HttpStreamFactoryImpl::Job::Job(JobController* job_controller,
+                                JobType job_type,
                                 HttpNetworkSession* session,
                                 const HttpRequestInfo& request_info,
                                 RequestPriority priority,
@@ -199,8 +202,7 @@ HttpStreamFactoryImpl::Job::Job(JobController* job_controller,
       origin_url_(origin_url),
       alternative_service_(alternative_service),
       job_controller_(job_controller),
-      blocking_job_(NULL),
-      waiting_job_(NULL),
+      job_type_(job_type),
       orphaned_(false),
       using_ssl_(false),
       using_spdy_(false),
@@ -226,6 +228,7 @@ HttpStreamFactoryImpl::Job::Job(JobController* job_controller,
 
 HttpStreamFactoryImpl::Job::~Job() {
   net_log_.EndEvent(NetLog::TYPE_HTTP_STREAM_JOB);
+  job_controller_->OnJobDeletion(this);

[Ryan Hamilton, 2016/05/06 21:33:14]

The controller owns the job, right? That means the controller is deleting the
job. So that seems to suggest that the controller already knows that the job is
being deleted.

[Zhongyi Shi, 2016/05/13 00:31:25]

The intention was to remove references to the deleted Job. But since we had
fewer ref (just bound_job_), yeah, we don't need this anymore. 

 
   // When we're in a partially constructed state, waiting for the user to
   // provide certificate handling information or authentication, we can't reuse
@@ -283,21 +286,8 @@ LoadState HttpStreamFactoryImpl::Job::GetLoadState() const {
   }
 }
 
-void HttpStreamFactoryImpl::Job::WaitFor(Job* job) {
-  DCHECK_EQ(STATE_NONE, next_state_);
-  DCHECK_EQ(STATE_NONE, job->next_state_);
-  DCHECK(!blocking_job_);
-  DCHECK(!job->waiting_job_);
-
-  // Never share connection with other jobs for FTP requests.
-  DCHECK(!request_info_.url.SchemeIs("ftp"));
-
-  blocking_job_ = job;
-  job->waiting_job_ = this;
-}
-
 void HttpStreamFactoryImpl::Job::ResumeAfterDelay() {
-  DCHECK(!blocking_job_);
+  DCHECK((job_type_ == ALTERNATIVE || !job_controller_->racing()));
   DCHECK_EQ(STATE_WAIT_FOR_JOB_COMPLETE, next_state_);
 
   net_log_.AddEvent(NetLog::TYPE_HTTP_STREAM_JOB_DELAYED,
@@ -308,11 +298,7 @@ void HttpStreamFactoryImpl::Job::ResumeAfterDelay() {
       wait_time_);
 }
 
-void HttpStreamFactoryImpl::Job::Resume(Job* job,
-                                        const base::TimeDelta& delay) {
-  DCHECK_EQ(blocking_job_, job);
-  blocking_job_ = NULL;
-
+void HttpStreamFactoryImpl::Job::Resume(const base::TimeDelta& delay) {
   // If |this| job is not past STATE_WAIT_FOR_JOB_COMPLETE state, then it will
   // be delayed by the |wait_time_| when it resumes.
   if (next_state_ == STATE_NONE || next_state_ <= STATE_WAIT_FOR_JOB_COMPLETE)
@@ -329,18 +315,9 @@ void HttpStreamFactoryImpl::Job::Orphan(const Request* request) {
   if (!IsPreconnecting())
     orphaned_ = true;
   net_log_.AddEvent(NetLog::TYPE_HTTP_STREAM_JOB_ORPHANED);
-  if (blocking_job_) {
-    // We've been orphaned, but there's a job we're blocked on. Don't bother
-    // racing, just cancel ourself.
-    DCHECK(blocking_job_->waiting_job_);
-    blocking_job_->waiting_job_ = NULL;
-    blocking_job_ = NULL;
-    if (job_controller_->for_websockets() && connection_ &&
-        connection_->socket()) {
-      connection_->socket()->Disconnect();
-    }
-    job_controller_->OnOrphanedJobComplete(this);
-  } else if (job_controller_->for_websockets()) {
+
+  job_controller_->MaybeResumeOtherJob(this, base::TimeDelta());
+  if (job_controller_->for_websockets()) {
     // We cancel this job because a WebSocketHandshakeStream can't be created
     // without a WebSocketHandshakeStreamBase::CreateHelper which is stored in
     // the Request class and isn't accessible from this job.
@@ -610,9 +587,10 @@ int HttpStreamFactoryImpl::Job::RunLoop(int result) {
   if (result == ERR_IO_PENDING)
     return result;
 
-  // If there was an error, we should have already resumed the |waiting_job_|,
-  // if there was one.
-  DCHECK(result == OK || waiting_job_ == NULL);
+  // If there was an error, we should have already resumed the other waiting
+  // job, if there was one.
+  DCHECK(result == OK || job_type_ == NON_ALTERNATIVE ||
+         !job_controller_->racing());
 
   if (IsPreconnecting()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -816,10 +794,7 @@ int HttpStreamFactoryImpl::Job::DoStart() {
   // Don't connect to restricted ports.
   if (!IsPortAllowedForScheme(destination_.port(),
                               request_info_.url.scheme())) {
-    if (waiting_job_) {
-      waiting_job_->Resume(this, base::TimeDelta());
-      waiting_job_ = NULL;
-    }
+    job_controller_->MaybeResumeOtherJob(this, base::TimeDelta());
     return ERR_UNSAFE_PORT;
   }
 
@@ -892,10 +867,7 @@ int HttpStreamFactoryImpl::Job::DoResolveProxyComplete(int result) {
   }
 
   if (result != OK) {
-    if (waiting_job_) {
-      waiting_job_->Resume(this, base::TimeDelta());
-      waiting_job_ = NULL;
-    }
+    job_controller_->MaybeResumeOtherJob(this, base::TimeDelta());
     return result;
   }
 
@@ -911,8 +883,9 @@ bool HttpStreamFactoryImpl::Job::ShouldForceQuic() const {
 }
 
 int HttpStreamFactoryImpl::Job::DoWaitForJob() {
-  if (!blocking_job_ && wait_time_.is_zero()) {
-    // There is no |blocking_job_| and there is no |wait_time_|.
+  if ((job_type_ == ALTERNATIVE || !job_controller_->racing()) &&
+      wait_time_.is_zero()) {
+    // There is no blocking job and there is no |wait_time_|.
     next_state_ = STATE_INIT_CONNECTION;
     return OK;
   }
@@ -920,7 +893,7 @@ int HttpStreamFactoryImpl::Job::DoWaitForJob() {
   next_state_ = STATE_WAIT_FOR_JOB_COMPLETE;
   if (!wait_time_.is_zero()) {
     // If there is a waiting_time, then resume the job after the wait_time_.
-    DCHECK(!blocking_job_);
+    DCHECK((job_type_ == ALTERNATIVE || !job_controller_->racing()));

[Ryan Hamilton, 2016/05/06 21:33:14]

it's not clear to me why the job needs to perform this check since this seems
like the responsibility of the controller.

[Zhongyi Shi, 2016/05/13 00:31:24]

Moved to JobController!

     ResumeAfterDelay();
   }
 
@@ -928,7 +901,7 @@ int HttpStreamFactoryImpl::Job::DoWaitForJob() {
 }
 
 int HttpStreamFactoryImpl::Job::DoWaitForJobComplete(int result) {
-  DCHECK(!blocking_job_);
+  DCHECK((job_type_ == ALTERNATIVE || !job_controller_->racing()));
   DCHECK_EQ(OK, result);
   wait_time_ = base::TimeDelta();
   next_state_ = STATE_INIT_CONNECTION;
@@ -940,7 +913,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "462812 HttpStreamFactoryImpl::Job::DoInitConnection"));
-  DCHECK(!blocking_job_);
+  DCHECK((job_type_ == ALTERNATIVE || !job_controller_->racing()));
   DCHECK(!connection_->is_initialized());
   DCHECK(proxy_info_.proxy_server().is_valid());
   next_state_ = STATE_INIT_CONNECTION_COMPLETE;
@@ -1012,22 +985,18 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
     if (rv == OK) {
       using_existing_quic_session_ = true;
     } else {
-      // OK, there's no available QUIC session. Let |waiting_job_| resume
-      // if it's paused.
-      if (waiting_job_) {
-        if (rv == ERR_IO_PENDING) {
-          // Start the |waiting_job_| after the delay returned by
-          // GetTimeDelayForWaitingJob().
-          //
-          // If QUIC request fails during handshake, then
-          // DoInitConnectionComplete() will start the |waiting_job_|.
-          waiting_job_->Resume(this, quic_request_.GetTimeDelayForWaitingJob());
-        } else {
-          // QUIC request has failed, resume the |waiting_job_|.
-          waiting_job_->Resume(this, base::TimeDelta());
-        }
-        waiting_job_ = NULL;
+      base::TimeDelta delay = base::TimeDelta();

[Ryan Hamilton, 2016/05/06 21:33:14]

nit: I think you can remove the = ...;

[Zhongyi Shi, 2016/05/13 00:31:24]

Done.

+      // OK, there's no available QUIC session. Let JobController to resume
+      // the waiting job if it's paused.
+      if (rv == ERR_IO_PENDING) {
+        // Start the waiting job after the delay returned by
+        // GetTimeDelayForWaitingJob().
+        //
+        // If QUIC request fails during handshake, then
+        // DoInitConnectionComplete() will start the waiting job.
+        delay = quic_request_.GetTimeDelayForWaitingJob();
       }
+      job_controller_->MaybeResumeOtherJob(this, delay);
     }
     return rv;
   }
@@ -1059,12 +1028,9 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
     job_controller_->SetSpdySessionKey(spdy_session_key);
   }
 
-  // OK, there's no available SPDY session. Let |waiting_job_| resume if it's
-  // paused.
-  if (waiting_job_) {
-    waiting_job_->Resume(this, base::TimeDelta());
-    waiting_job_ = NULL;
-  }
+  // OK, there's no available SPDY session. Let JobController to resume the
+  // non-alternative job if it's paused.
+  job_controller_->MaybeResumeOtherJob(this, base::TimeDelta());
 
   if (proxy_info_.is_http() || proxy_info_.is_https())
     establishing_tunnel_ = using_ssl_;
@@ -1118,9 +1084,8 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
 }
 
 int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
-  if (using_quic_ && result < 0 && waiting_job_) {
-    waiting_job_->Resume(this, base::TimeDelta());
-    waiting_job_ = NULL;
+  if (using_quic_ && result < 0) {
+    job_controller_->MaybeResumeOtherJob(this, base::TimeDelta());
   }
   if (IsPreconnecting()) {
     if (using_quic_)
@@ -1158,9 +1123,8 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
 
   // TODO(willchan): Make this a bit more exact. Maybe there are recoverable
   // errors, such as ignoring certificate errors for Alternate-Protocol.
-  if (result < 0 && waiting_job_) {
-    waiting_job_->Resume(this, base::TimeDelta());
-    waiting_job_ = NULL;
+  if (result < 0) {
+    job_controller_->MaybeResumeOtherJob(this, base::TimeDelta());
   }
 
   // |result| may be the result of any of the stacked pools. The following