Fix accounting and logging of HttpStream request and job bindings.

net/http/http_stream_factory_impl_request.cc

 // Copyright (c) 2012 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_request.h"
 
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "net/http/http_stream_factory_impl_job.h"
@@ skipping 50 matching lines @@
   DCHECK(!ContainsKey(request_set, this));
   request_set.insert(this);
 }
 
 void HttpStreamFactoryImpl::Request::AttachJob(Job* job) {
   DCHECK(job);
   jobs_.insert(job);
   factory_->request_map_[job] = this;
 }
 
-void HttpStreamFactoryImpl::Request::Complete(
-    bool was_npn_negotiated,
-    NextProto protocol_negotiated,
-    bool using_spdy,
-    const BoundNetLog& job_net_log) {
+void HttpStreamFactoryImpl::Request::Complete(bool was_npn_negotiated,
+                                              NextProto protocol_negotiated,
+                                              bool using_spdy) {
   DCHECK(!completed_);
   completed_ = true;
   was_npn_negotiated_ = was_npn_negotiated;
   protocol_negotiated_ = protocol_negotiated;
   using_spdy_ = using_spdy;
-  net_log_.AddEvent(
-      NetLog::TYPE_HTTP_STREAM_REQUEST_BOUND_TO_JOB,
-      job_net_log.source().ToEventParametersCallback());
-  job_net_log.AddEvent(
-      NetLog::TYPE_HTTP_STREAM_JOB_BOUND_TO_REQUEST,
-      net_log_.source().ToEventParametersCallback());
 }
 
 void HttpStreamFactoryImpl::Request::OnStreamReady(
     Job* job,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     HttpStream* stream) {
   DCHECK(!factory_->for_websockets_);
   DCHECK(stream);
   DCHECK(completed_);
@@ skipping 17 matching lines @@
 }
 
 void HttpStreamFactoryImpl::Request::OnStreamFailed(
     Job* job,
     int status,
     const SSLConfig& used_ssl_config,
     SSLFailureState ssl_failure_state) {
   DCHECK_NE(OK, status);
   DCHECK(job);
   if (!bound_job_.get()) {
-    // Hey, we've got other jobs! Maybe one of them will succeed, let's just
-    // ignore this failure.
     if (jobs_.size() > 1) {
+      // Hey, we've got other jobs! Maybe one of them will succeed, let's just
+      // ignore this failure.
       jobs_.erase(job);
       factory_->request_map_.erase(job);
       // Notify all the other jobs that this one failed.
       for (std::set<Job*>::iterator it = jobs_.begin(); it != jobs_.end(); ++it)
         (*it)->MarkOtherJobComplete(*job);
       delete job;
       return;
     } else {
-      bound_job_.reset(job);
-      jobs_.erase(job);
-      DCHECK(jobs_.empty());
-      factory_->request_map_.erase(job);
+      OrphanJobsExcept(job);

[mmenke, 2015/06/25 23:20:20]

This is kind of silly.  Can we rename this?  "BindJobAndOrphanOthers" or just
"BindJob"?

[davidben, 2015/06/26 19:16:30]

Done. Though in doing that and trying to figure out the responsibilities of that
method, it seems there's a few other variants of the bug here where we don't
disconnect from jobs and the SpdySessionRequest map when something resolves.
I've gone ahead and resolved them in this CL since the NetLog thing is pretty
tangential in comparison.

+rch to make sure I haven't done anything crazy with the orphaned/canceled job
tracking.

     }
   } else {
     DCHECK(jobs_.empty());
   }
   delegate_->OnStreamFailed(status, used_ssl_config, ssl_failure_state);
 }
 
 void HttpStreamFactoryImpl::Request::OnCertificateError(
     Job* job,
     int status,
@@ skipping 140 matching lines @@
 
   // Cache these values in case the job gets deleted.
   const SSLConfig used_ssl_config = job->server_ssl_config();
   const ProxyInfo used_proxy_info = job->proxy_info();
   const bool was_npn_negotiated = job->was_npn_negotiated();
   const NextProto protocol_negotiated =
       job->protocol_negotiated();
   const bool using_spdy = job->using_spdy();
   const BoundNetLog net_log = job->net_log();
 
-  Complete(was_npn_negotiated, protocol_negotiated, using_spdy, net_log);
+  Complete(was_npn_negotiated, protocol_negotiated, using_spdy);
 
   // Cache this so we can still use it if the request is deleted.
   HttpStreamFactoryImpl* factory = factory_;
   if (factory->for_websockets_) {
     // TODO(ricea): Re-instate this code when WebSockets over SPDY is
     // implemented.
     NOTREACHED();
   } else {
     delegate_->OnStreamReady(job->server_ssl_config(), job->proxy_info(),
                              stream.release());
@@ skipping 18 matching lines @@
 }
 
 void HttpStreamFactoryImpl::Request::OrphanJobsExcept(Job* job) {
   DCHECK(job);
   DCHECK(!bound_job_.get());
   DCHECK(ContainsKey(jobs_, job));
   bound_job_.reset(job);
   jobs_.erase(job);
   factory_->request_map_.erase(job);
 
+  net_log_.AddEvent(NetLog::TYPE_HTTP_STREAM_REQUEST_BOUND_TO_JOB,
+                    job->net_log().source().ToEventParametersCallback());
+  job->net_log().AddEvent(NetLog::TYPE_HTTP_STREAM_JOB_BOUND_TO_REQUEST,
+                          net_log_.source().ToEventParametersCallback());
+
   OrphanJobs();
 }
 
 void HttpStreamFactoryImpl::Request::OrphanJobs() {
   RemoveRequestFromSpdySessionRequestMap();
 
   std::set<Job*> tmp;
   tmp.swap(jobs_);
 
   for (std::set<Job*>::iterator it = tmp.begin(); it != tmp.end(); ++it)
@@ skipping 27 matching lines @@
     }
     // We may have other jobs in |jobs_|. For example, if we start multiple jobs
     // for Alternate-Protocol.
     OrphanJobsExcept(job);
     return;
   }
   DCHECK(jobs_.empty());
 }
 
 }  // namespace net