Fix race between OnPipelineFeedback and

HttpStreamFactoryImpl::Job::OnStreamReadyCallback.

As a side-effect of OnPipelineFeedback() calling OnPipelineHasCapacity() on all
of the pipelines, some requests are late-bound to existing pipelines with
capacity. However, those requests may have been in the process of establishing
new pipelines. By stealing those requests, the new pipelines become empty and
are deleted. Later iterations of the loop in OnPipelineFeedback() weren't
careful about checking to see if the pipeline was deleted before calling
OnPipelineHasCapacity() on it.

BUG=106313
TEST=net_unittests under valgrind


Review URL: http://codereview.chromium.org/8820005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@113287 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 24 insertions, 13 deletions

diff --git a/net/http/http_pipelined_host_impl.cc b/net/http/http_pipelined_host_impl.cc
index 25014763..d554fe1 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 capability_ != INCAPABLE &&
+      pipeline->usable() &&
+      pipeline->active() &&
+      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) {
 }