Fix race between OnPipelineFeedback and

net/http/http_pipelined_host_impl.cc

Index: net/http/http_pipelined_host_impl.cc
diff --git a/net/http/http_pipelined_host_impl.cc b/net/http/http_pipelined_host_impl.cc
index 2501476387f31c8a8e7c8a731b4a24aedd3fcd96..b9caad6d701d0d50695310cf72d07f024c75e047 100644
--- a/net/http/http_pipelined_host_impl.cc
+++ b/net/http/http_pipelined_host_impl.cc
@@ -69,9 +69,7 @@ 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;
@@ -86,9 +84,7 @@ HttpPipelinedStream* HttpPipelinedHostImpl::CreateStreamOnExistingPipeline() {
 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;
     }
   }
@@ -112,9 +108,7 @@ void HttpPipelinedHostImpl::OnPipelineEmpty(HttpPipelinedConnection* pipeline) {
 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()) {
@@ -132,10 +126,7 @@ void HttpPipelinedHostImpl::OnPipelineFeedback(
       ++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) {
@@ -176,6 +167,26 @@ int HttpPipelinedHostImpl::GetPipelineCapacity() const {
   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) {
 }