Clean up HttpStreamFactoryImpl::JobController when Impl::Jobs complete

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 7e76ce5ce3436767ba5f47262d75ec4dbcaf16b8..44ab1966568a0bc176c031db6803afb611f43bd5 100644
--- a/net/http/http_stream_factory_impl_job_controller.cc
+++ b/net/http/http_stream_factory_impl_job_controller.cc
@@ -145,9 +145,19 @@ void HttpStreamFactoryImpl::JobController::OnRequestComplete() {
   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.
+      // Do not reset |alternative_job_| here and let it run for completion.
+      // OnOrphanedJobComplete() will clean up |this| when the job completes.
     } else {
       DCHECK(bound_job_->job_type() == ALTERNATIVE);
       alternative_job_.reset();
+      // If |main_job_| hasn't started connecting, reset it. Otherwise,
+      // OnOrphanedJobComplete() will clean up |this| when the job completes.
+      // Use |main_job_is_blocked_| instead of |main_job_|->is_waiting() because
+      // |main_job_| can be in proxy resolution step.
+      if (main_job_ && main_job_is_blocked_)

[mmenke, 2017/01/09 18:37:20]

Should this be:

if (main_job_ && (main_job_is_blocked_ || !main_job_wait_time_.is_zero()))?

If main_job_wait_time_ is not zero, it means the job hasn't been started yet
(It's either waiting to be resumed, or still in the proxy resolution
phase)...Assuming there's no case where we set it to some non-zero value after a
job's been started.

[xunjieli, 2017/01/09 18:52:31]

In HttpStreamFactoryImpl::Job::DoInitConnectionImpl(), it's possible that we set
main_job_wait_time_ to zero or some non-zero value. Is |main_job_is_blocked_|
check not enough? It translates to the same idea right (job not started or
waiting for resumed)?

[mmenke, 2017/01/09 18:56:19]

It's not the same thing.  We set main_job_is_blocked_ to false when we start the
timer to actually start the blocked job.  It seems weird to delay the start of a
Job in favor of just letting the QUIC job complete, and when the QUIC job
completes, starting the other job...  If we're going to start the other job,
regardless, why bother delaying it?

It is enough to fix the leak, but I think the behavior is very weird and
unexpected.

[xunjieli, 2017/01/09 19:18:21]

Done. Got it, Thanks!

[Zhongyi Shi, 2017/01/09 20:09:45]

Umm, main_job_wait_time is only used to figure out how much time the main job
should wait so that when resuming the main job, the timer is set appropriately,
the wait time is usually set when QUIC job figures it out in InitConnection.
|main_job_is_blocked_| is what we are looking at when we decide whether or not
to block the main job if you refer to JobController::ShouldWait(). 

The case where |main_job_is_blocked_| = false, while we have non-zero wait time
is that the JobController::ResumeMainJob() is posted but not executed due to the
 delayed task. I don't think we should reset main_job here as when ResumeMainJob
is executed, the main_job_->Resume() will fire unless you canceled the posted
task when resetting the main_job_.

[mmenke, 2017/01/09 20:12:27]

That strikes me as unexpected - why are we delaying starting the job, if not
because we won't need the job if the QUIC job succeeds?

[Zhongyi Shi, 2017/01/09 23:18:49]

We are delaying the tcp job because we want to give priority to QUIC job in the
race so that QUIC job can finish the handshake so that QUIC could succeed. 

https://cs.chromium.org/chromium/src/net/quic/chromium/quic_stream_factory.cc...

xunjie@: reading at the code, OnOrphanedJobComplete is called when the orphaned
job completes. In the case where we set the main_job_is_blocked_ to false and
post ResumeMainJob but executing yet, I am more concerned that if we reset the
main_job_, when ResumeMainJob is executed, main_job_->Resume() will fire since
the main_job is destroyed.

[mmenke, 2017/01/09 23:25:12]

So what is fundamentally different between cases where we've started the timer
and cases we have not?

[Zhongyi Shi, 2017/01/09 23:39:46]

My understanding is: if we've started the timer, then the main job will start to
connect if QUIC job doesn't complete in the grace period, and it becomes racing
then. If we never started the timer, the QUIC job can run forward until it fails
or pending and won't be interrupted. 

[Zhongyi Shi, 2017/01/10 01:11:32]

Looks like both jobs and request will be reset here which means the
JobController will then be destroyed. Since we used WeakPtr when post
ResumeMainJob. This should have no problem. Sorry for introducing confusions. 

+        main_job_.reset();
     }
     bound_job_ = nullptr;
   }
@@ -177,7 +187,7 @@ void HttpStreamFactoryImpl::JobController::OnStreamReady(
 
   factory_->OnStreamReady(job->proxy_info());
 
-  if (job_bound_ && bound_job_ != job) {
+  if (!request_ || (job_bound_ && bound_job_ != job)) {
     // We have bound a job to the associated Request, |job| has been orphaned.
     OnOrphanedJobComplete(job);
     return;
@@ -202,7 +212,7 @@ void HttpStreamFactoryImpl::JobController::OnBidirectionalStreamImplReady(
     const ProxyInfo& used_proxy_info) {
   DCHECK(job);
 
-  if (job_bound_ && bound_job_ != job) {
+  if (!request_ || (job_bound_ && bound_job_ != job)) {
     // We have bound a job to the associated Request, |job| has been orphaned.
     OnOrphanedJobComplete(job);
     return;
@@ -253,7 +263,7 @@ void HttpStreamFactoryImpl::JobController::OnStreamFailed(
 
   MaybeResumeMainJob(job, base::TimeDelta());
 
-  if (job_bound_ && bound_job_ != job) {
+  if (!request_ || (job_bound_ && bound_job_ != job)) {
     // We have bound a job to the associated Request, |job| has been orphaned.
     OnOrphanedJobComplete(job);
     return;
@@ -291,7 +301,7 @@ void HttpStreamFactoryImpl::JobController::OnCertificateError(
     const SSLInfo& ssl_info) {
   MaybeResumeMainJob(job, base::TimeDelta());
 
-  if (job_bound_ && bound_job_ != job) {
+  if (!request_ || (job_bound_ && bound_job_ != job)) {
     // We have bound a job to the associated Request, |job| has been orphaned.
     OnOrphanedJobComplete(job);
     return;
@@ -314,7 +324,7 @@ void HttpStreamFactoryImpl::JobController::OnHttpsProxyTunnelResponse(
     HttpStream* stream) {
   MaybeResumeMainJob(job, base::TimeDelta());
 
-  if (job_bound_ && bound_job_ != job) {
+  if (!request_ || (job_bound_ && bound_job_ != job)) {
     // We have bound a job to the associated Request, |job| has been orphaned.
     OnOrphanedJobComplete(job);
     return;
@@ -334,7 +344,7 @@ void HttpStreamFactoryImpl::JobController::OnNeedsClientAuth(
     SSLCertRequestInfo* cert_info) {
   MaybeResumeMainJob(job, base::TimeDelta());
 
-  if (job_bound_ && bound_job_ != job) {
+  if (!request_ || (job_bound_ && bound_job_ != job)) {
     // We have bound a job to the associated Request, |job| has been orphaned.
     OnOrphanedJobComplete(job);
     return;
@@ -355,7 +365,7 @@ void HttpStreamFactoryImpl::JobController::OnNeedsProxyAuth(
     HttpAuthController* auth_controller) {
   MaybeResumeMainJob(job, base::TimeDelta());
 
-  if (job_bound_ && bound_job_ != job) {
+  if (!request_ || (job_bound_ && bound_job_ != job)) {

[mmenke, 2017/01/09 18:37:20]

Suggest a helper method here, IsJobOrphaned(job) const, and using it in place of
all these checks.

[xunjieli, 2017/01/09 19:18:21]

Done.

     // We have bound a job to the associated Request, |job| has been orphaned.
     OnOrphanedJobComplete(job);
     return;
@@ -422,7 +432,7 @@ void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady(
   DCHECK(job->using_spdy());
   DCHECK(!is_preconnect_);
 
-  bool is_job_orphaned = job_bound_ && bound_job_ != job;
+  bool is_job_orphaned = !request_ || (job_bound_ && bound_job_ != job);
 
   // Cache these values in case the job gets deleted.
   const SSLConfig used_ssl_config = job->server_ssl_config();