Fix race between OnPipelineFeedback and

net/http/http_pipelined_host_impl.cc

 // Copyright (c) 2011 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_pipelined_host_impl.h"
 
 #include "base/stl_util.h"
 #include "net/http/http_pipelined_connection_impl.h"
 #include "net/http/http_pipelined_stream.h"
 
@@ skipping 51 matching lines @@
       was_npn_negotiated);
   PipelineInfo info;
   pipelines_.insert(std::make_pair(pipeline, info));
   return pipeline->CreateNewStream();
 }
 
 HttpPipelinedStream* HttpPipelinedHostImpl::CreateStreamOnExistingPipeline() {
   HttpPipelinedConnection* available_pipeline = NULL;
   for (PipelineInfoMap::iterator it = pipelines_.begin();
        it != pipelines_.end(); ++it) {
-    if (it->first->usable() &&
-        it->first->active() &&
-        it->first->depth() < GetPipelineCapacity() &&
+    if (CanPipelineAcceptRequests(it->first) &&
         (!available_pipeline ||
          it->first->depth() < available_pipeline->depth())) {
       available_pipeline = it->first;
     }
   }
   if (!available_pipeline) {
     return NULL;
   }
   return available_pipeline->CreateNewStream();
 }
 
 bool HttpPipelinedHostImpl::IsExistingPipelineAvailable() const {
   for (PipelineInfoMap::const_iterator it = pipelines_.begin();
        it != pipelines_.end(); ++it) {
-    if (it->first->usable() &&
-        it->first->active() &&
-        it->first->depth() < GetPipelineCapacity()) {
+    if (CanPipelineAcceptRequests(it->first)) {
       return true;
     }
   }
   return false;
 }
 
 const HostPortPair& HttpPipelinedHostImpl::origin() const {
   return origin_;
 }
 
 void HttpPipelinedHostImpl::OnPipelineEmpty(HttpPipelinedConnection* pipeline) {
   CHECK(ContainsKey(pipelines_, pipeline));
   pipelines_.erase(pipeline);
   delete pipeline;
   if (pipelines_.empty()) {
     delegate_->OnHostIdle(this);
     // WARNING: We'll probably be deleted here.
   }
 }
 
 void HttpPipelinedHostImpl::OnPipelineHasCapacity(
     HttpPipelinedConnection* pipeline) {
   CHECK(ContainsKey(pipelines_, pipeline));
-  if (pipeline->usable() &&
-      capability_ != INCAPABLE &&
-      pipeline->depth() < GetPipelineCapacity()) {
+  if (CanPipelineAcceptRequests(pipeline)) {
     delegate_->OnHostHasAdditionalCapacity(this);
   }
   if (!pipeline->depth()) {
     OnPipelineEmpty(pipeline);
     // WARNING: We might be deleted here.
   }
 }
 
 void HttpPipelinedHostImpl::OnPipelineFeedback(
     HttpPipelinedConnection* pipeline,
     HttpPipelinedConnection::Feedback feedback) {
   CHECK(ContainsKey(pipelines_, pipeline));
   switch (feedback) {
     case HttpPipelinedConnection::OK:
       ++pipelines_[pipeline].num_successes;
       if (capability_ == UNKNOWN) {
         capability_ = PROBABLY_CAPABLE;
-        for (PipelineInfoMap::iterator it = pipelines_.begin();
-             it != pipelines_.end(); ++it) {
-          OnPipelineHasCapacity(it->first);
-        }
+        NotifyAllPipelinesHaveCapacity();
       } else if (capability_ == PROBABLY_CAPABLE &&
                  pipelines_[pipeline].num_successes >=
                      kNumKnownSuccessesThreshold) {
         capability_ = CAPABLE;
         delegate_->OnHostDeterminedCapability(this, CAPABLE);
       }
       break;
 
     case HttpPipelinedConnection::PIPELINE_SOCKET_ERROR:
     case HttpPipelinedConnection::OLD_HTTP_VERSION:
@@ skipping 20 matching lines @@
     case UNKNOWN:
       capacity = 1;
       break;
 
     default:
       CHECK(false) << "Unkown pipeline capability: " << capability_;
   }
   return capacity;
 }
 
+bool HttpPipelinedHostImpl::CanPipelineAcceptRequests(
+    HttpPipelinedConnection* pipeline) const {
+  return pipeline->usable() &&
+      pipeline->active() &&
+      capability_ != INCAPABLE &&

[mmenke, 2011/12/06 15:29:03]

nit:  Seems a little odd to have this in the middle of the pipeline->whatever()
comparisons.  Doesn't really matter, but think having it first or last slightly
enhances readability.

[James Simonsen, 2011/12/06 19:41:35]

Done.

+      pipeline->depth() < GetPipelineCapacity();
+}
+
+void HttpPipelinedHostImpl::NotifyAllPipelinesHaveCapacity() {
+  // Calling OnPipelineHasCapacity() can have side effects that include
+  // deleting and removing entries from |pipelines_|.
+  PipelineInfoMap pipelines_to_notify = pipelines_;
+  for (PipelineInfoMap::iterator it = pipelines_to_notify.begin();
+       it != pipelines_to_notify.end(); ++it) {
+    if (pipelines_.find(it->first) != pipelines_.end()) {
+      OnPipelineHasCapacity(it->first);
+    }
+  }
+}
+
 HttpPipelinedHostImpl::PipelineInfo::PipelineInfo()
     : num_successes(0) {
 }
 
 }  // namespace net