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 21 matching lines @@
       using_spdy_(false) {
   DCHECK(factory_);
   DCHECK(delegate_);
 
   net_log_.BeginEvent(NetLog::TYPE_HTTP_STREAM_REQUEST);
 }
 
 HttpStreamFactoryImpl::Request::~Request() {
   if (bound_job_.get())
     DCHECK(jobs_.empty());
-  else
-    DCHECK(!jobs_.empty());
 
   net_log_.EndEvent(NetLog::TYPE_HTTP_STREAM_REQUEST);
 
-  for (std::set<Job*>::iterator it = jobs_.begin(); it != jobs_.end(); ++it)
-    factory_->request_map_.erase(*it);
-
-  RemoveRequestFromSpdySessionRequestMap();
-
-  STLDeleteElements(&jobs_);
+  CancelJobs();
 }
 
 void HttpStreamFactoryImpl::Request::SetSpdySessionKey(
     const SpdySessionKey& spdy_session_key) {
   CHECK(!spdy_session_key_.get());
   spdy_session_key_.reset(new SpdySessionKey(spdy_session_key));
   RequestSet& request_set =
       factory_->spdy_session_request_map_[spdy_session_key];
   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);
+      BindJob(job);
     }
   } else {
     DCHECK(jobs_.empty());
   }
   delegate_->OnStreamFailed(status, used_ssl_config, ssl_failure_state);
 }
 
 void HttpStreamFactoryImpl::Request::OnCertificateError(
     Job* job,
     int status,
     const SSLConfig& used_ssl_config,
     const SSLInfo& ssl_info) {
   DCHECK_NE(OK, status);
   if (!bound_job_.get())
-    OrphanJobsExcept(job);
+    BindJob(job);
   else
     DCHECK(jobs_.empty());
   delegate_->OnCertificateError(status, used_ssl_config, ssl_info);
 }
 
 void HttpStreamFactoryImpl::Request::OnNeedsProxyAuth(
     Job* job,
     const HttpResponseInfo& proxy_response,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     HttpAuthController* auth_controller) {
   if (!bound_job_.get())
-    OrphanJobsExcept(job);
+    BindJob(job);
   else
     DCHECK(jobs_.empty());
   delegate_->OnNeedsProxyAuth(
       proxy_response, used_ssl_config, used_proxy_info, auth_controller);
 }
 
 void HttpStreamFactoryImpl::Request::OnNeedsClientAuth(
     Job* job,
     const SSLConfig& used_ssl_config,
     SSLCertRequestInfo* cert_info) {
   if (!bound_job_.get())
-    OrphanJobsExcept(job);
+    BindJob(job);
   else
     DCHECK(jobs_.empty());
   delegate_->OnNeedsClientAuth(used_ssl_config, cert_info);
 }
 
 void HttpStreamFactoryImpl::Request::OnHttpsProxyTunnelResponse(
     Job *job,
     const HttpResponseInfo& response_info,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     HttpStream* stream) {
   if (!bound_job_.get())
-    OrphanJobsExcept(job);
+    BindJob(job);
   else
     DCHECK(jobs_.empty());
   delegate_->OnHttpsProxyTunnelResponse(
       response_info, used_ssl_config, used_proxy_info, stream);
 }
 
 int HttpStreamFactoryImpl::Request::RestartTunnelWithProxyAuth(
     const AuthCredentials& credentials) {
   DCHECK(bound_job_.get());
   return bound_job_->RestartTunnelWithProxyAuth(credentials);
@@ skipping 72 matching lines @@
     bool direct) {
   DCHECK(job);
   DCHECK(job->using_spdy());
 
   // Note: |spdy_session| may be NULL. In that case, |delegate_| should still
   // receive |stream| so the error propogates up correctly, however there is no
   // point in broadcasting |spdy_session| to other requests.
 
   // The first case is the usual case.
   if (!bound_job_.get()) {
-    OrphanJobsExcept(job);
+    BindJob(job);
   } else {  // This is the case for HTTPS proxy tunneling.
     DCHECK_EQ(bound_job_.get(), job);
     DCHECK(jobs_.empty());
   }
 
   // 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 10 matching lines @@
                                    net_log);
   }
 }
 
 void HttpStreamFactoryImpl::Request::AddConnectionAttempts(
     const ConnectionAttempts& attempts) {
   for (const auto& attempt : attempts)
     connection_attempts_.push_back(attempt);
 }
 
-void HttpStreamFactoryImpl::Request::OrphanJobsExcept(Job* job) {
+void HttpStreamFactoryImpl::Request::BindJob(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)
-    factory_->OrphanJob(*it, this);
+  for (Job* job : tmp)
+    factory_->OrphanJob(job, this);
+}
+
+void HttpStreamFactoryImpl::Request::CancelJobs() {
+  RemoveRequestFromSpdySessionRequestMap();
+
+  std::set<Job*> tmp;
+  tmp.swap(jobs_);
+
+  for (Job* job : tmp) {
+    factory_->request_map_.erase(job);
+    delete job;
+  }
 }
 
 void HttpStreamFactoryImpl::Request::OnJobSucceeded(Job* job) {
   // |job| should only be NULL 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_.get());
     DCHECK(!jobs_.empty());
     // NOTE(willchan): We do *NOT* call OrphanJobs() here. The reason is because
     // we *WANT* to cancel the unnecessary Jobs from other requests if another
     // Job completes first.
     // TODO(mbelshe): Revisit this when we implement ip connection pooling of
     // SpdySessions. Do we want to orphan the jobs for a different hostname so
     // they complete? Or do we want to prevent connecting a new SpdySession if
     // we've already got one available for a different hostname where the ip
     // address matches up?
+    CancelJobs();
     return;
   }
   if (!bound_job_.get()) {
     if (jobs_.size() > 1)
       job->ReportJobSucceededForRequest();
     // Notify all the other jobs that this one succeeded.
     for (std::set<Job*>::iterator it = jobs_.begin(); it != jobs_.end(); ++it) {
       if (*it != job) {
         (*it)->MarkOtherJobComplete(*job);
       }
     }
     // We may have other jobs in |jobs_|. For example, if we start multiple jobs
     // for Alternate-Protocol.
-    OrphanJobsExcept(job);
+    BindJob(job);
     return;
   }
   DCHECK(jobs_.empty());
 }
 
 }  // namespace net