Move the logic to cancel main job in OrphanUnboundJob when the alt job succeed while main job is bl…

net/http/http_stream_factory_impl_job_controller.cc

 // Copyright (c) 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_stream_factory_impl_job_controller.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 
@@ skipping 134 matching lines @@
   if (bound_job_) {
     if (bound_job_->job_type() == MAIN) {
       main_job_.reset();
       // |alternative_job_| can be non-null if |main_job_| is resumed after
       // |main_job_wait_time_| has elapsed. Allow |alternative_job_| to run to
       // completion, rather than resetting it. OnOrphanedJobComplete() will
       // clean up |this| when the job completes.
     } else {
       DCHECK(bound_job_->job_type() == ALTERNATIVE);
       alternative_job_.reset();
-      // If ResumeMainJob() is not executed, reset |main_job_|. Otherwise,
-      // OnOrphanedJobComplete() will clean up |this| when the job completes.
-      // Use |main_job_is_blocked_| and |!main_job_wait_time_.is_zero()| instead
-      // of |main_job_|->is_waiting() because |main_job_| can be in proxy
-      // resolution step.
-      if (main_job_ && (main_job_is_blocked_ || !main_job_wait_time_.is_zero()))
-        main_job_.reset();
     }
     bound_job_ = nullptr;
   }
   MaybeNotifyFactoryOfCompletion();
 }
 
 int HttpStreamFactoryImpl::JobController::RestartTunnelWithProxyAuth(
     const AuthCredentials& credentials) {
   DCHECK(bound_job_);
   return bound_job_->RestartTunnelWithProxyAuth(credentials);
@@ skipping 564 matching lines @@
   DCHECK(request_);
   RemoveRequestFromSpdySessionRequestMap();
 
   DCHECK(bound_job_);
   if (bound_job_->job_type() == MAIN && alternative_job_) {
     factory_->request_map_.erase(alternative_job_.get());
     alternative_job_->Orphan();
   } else if (bound_job_->job_type() == ALTERNATIVE && main_job_) {
     // Orphan main job.
     factory_->request_map_.erase(main_job_.get());
-    main_job_->Orphan();
+    // If ResumeMainJob() is not executed, reset |main_job_|. Otherwise,
+    // OnOrphanedJobComplete() will clean up |this| when the job completes.
+    // Use |main_job_is_blocked_| and |!main_job_wait_time_.is_zero()| instead
+    // of |main_job_|->is_waiting() because |main_job_| can be in proxy
+    // resolution step.
+    if (main_job_ && (main_job_is_blocked_ || !main_job_wait_time_.is_zero())) {

[mmenke, 2017/01/12 22:46:48]

DCHECK(alternative_job_)?  (If alternate_job_ is removed before this call, we'd
need a MaybeNotifyFactoryOfCompletion(); call after destroying the job)

[Zhongyi Shi, 2017/01/12 23:17:50]

Done.
bound_job_ will only be reset to nullptr when the Job it's referencing is
deleted. Thus it should be guaranteed that the alternative_job_ is not removed.
But DCHECK here is a good idea!

+      main_job_.reset();
+    } else {
+      main_job_->Orphan();

[xunjieli, 2017/01/12 23:00:32]

Should HttpStreamFactoryImpl::Job::Orphan() also call OnOrphanedJobComplete() in
the non-websockets case?
If this |main_job_| does complete, how will it be removed if not by
MaybeNotifyFactoryOfCompletion through OnOrphanedJobComplete()?

[xunjieli, 2017/01/12 23:04:30]

Think that the old code calls OnOrphanedJobComplete() here directly.

[Zhongyi Shi, 2017/01/12 23:17:50]

For non-websockets case, the Job will continue connecting until it finishes.
When it's finishing, it will call one of the callbacks and notifying the
JobController of the Job's completion. The JobController will then check whether
this job has been orphaned, and calls OnOrphanedJobComplete.

https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job_co...
Nope. The old code only calls OnOrphanedJobComplete if there's a blocking job
(which means the main job is blocked) or the main job is for websocket. That is,
if the main job is not blocked, and the main job is for non-websocket, it won't
call OnOrphanedJobComplte().
https://codereview.chromium.org/1952423002/diff/460001/net/http/http_stream_f...
 

+    }
   }
 }
 
 void HttpStreamFactoryImpl::JobController::OnJobSucceeded(Job* job) {
   // |job| should only be nullptr if we're being serviced by a late bound
   // SpdySession (one that was not created by a job in our |jobs_| set).
   if (!job) {
     DCHECK(!bound_job_);
     // NOTE(willchan): We do *NOT* call OrphanUnboundJob() here. The reason is
     // because we *WANT* to cancel the unnecessary Jobs from other requests if
@@ skipping 337 matching lines @@
     return;
   DCHECK(alternative_job_->alternative_proxy_server().is_valid());
   alternative_job_->Start(request_->stream_type());
 }
 
 bool HttpStreamFactoryImpl::JobController::IsJobOrphaned(Job* job) const {
   return !request_ || (job_bound_ && bound_job_ != job);
 }
 
 }  // namespace net