Fix accounting and logging of HttpStream request and job bindings.

net/http/http_stream_factory_impl_request.cc

Index: net/http/http_stream_factory_impl_request.cc
diff --git a/net/http/http_stream_factory_impl_request.cc b/net/http/http_stream_factory_impl_request.cc
index 53548b1254fbf0706ca1c177bac9a57264f6bd17..c527657da3cee44a0a453bd5880c2ad585a2f8cc 100644
--- a/net/http/http_stream_factory_impl_request.cc
+++ b/net/http/http_stream_factory_impl_request.cc
@@ -68,22 +68,14 @@ void HttpStreamFactoryImpl::Request::AttachJob(Job* 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(
@@ -121,9 +113,9 @@ void HttpStreamFactoryImpl::Request::OnStreamFailed(
   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.
@@ -132,10 +124,7 @@ void HttpStreamFactoryImpl::Request::OnStreamFailed(
       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());
@@ -296,7 +285,7 @@ void HttpStreamFactoryImpl::Request::OnNewSpdySessionReady(
   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_;
@@ -335,6 +324,11 @@ void HttpStreamFactoryImpl::Request::OrphanJobsExcept(Job* 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();
 }