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

Index: net/http/http_stream_factory_impl_job_controller.cc
diff --git a/net/http/http_stream_factory_impl_job_controller.cc b/net/http/http_stream_factory_impl_job_controller.cc
index c72fd3db7148254fa21964d411d9c5f363b138f9..34b76a77b6f46eebe008ecae06520804fa31c9cb 100644
--- a/net/http/http_stream_factory_impl_job_controller.cc
+++ b/net/http/http_stream_factory_impl_job_controller.cc
@@ -152,13 +152,6 @@ void HttpStreamFactoryImpl::JobController::OnRequestComplete() {
     } 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;
   }
@@ -743,7 +736,16 @@ void HttpStreamFactoryImpl::JobController::OrphanUnboundJob() {
   } 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...
 

+    }
   }
 }