Fix race between OnPipelineFeedback and

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


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113287

[mmenke, 2011-12-06 15:29:02]

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_host_im...
File net/http/http_pipelined_host_impl.cc (right):

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_host_im...
net/http/http_pipelined_host_impl.cc:174: capability_ != INCAPABLE &&
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.

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_network...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_network...
net/http/http_pipelined_network_transaction_unittest.cc:722: DataRunnerObserver
observer(data_vector_[0].get(), 3);
Why is this needed, exactly?

[willchan no longer on Chromium, 2011-12-06 17:49:21]

Looked at the change which seems fine to me. Deferring to Matt still. Thanks for
keeping me in the loop, and keep up the good work :)

On 2011/12/06 15:29:02, Matt Menke wrote:
>
http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_host_im...
> File net/http/http_pipelined_host_impl.cc (right):
> 
>
http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_host_im...
> net/http/http_pipelined_host_impl.cc:174: capability_ != INCAPABLE &&
> 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.
> 
>
http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_network...
> File net/http/http_pipelined_network_transaction_unittest.cc (right):
> 
>
http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_network...
> net/http/http_pipelined_network_transaction_unittest.cc:722:
DataRunnerObserver
> observer(data_vector_[0].get(), 3);
> Why is this needed, exactly?

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

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_host_im...
File net/http/http_pipelined_host_impl.cc (right):

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_host_im...
net/http/http_pipelined_host_impl.cc:174: capability_ != INCAPABLE &&
On 2011/12/06 15:29:03, Matt Menke wrote:
> 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.

Done.

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_network...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_network...
net/http/http_pipelined_network_transaction_unittest.cc:722: DataRunnerObserver
observer(data_vector_[0].get(), 3);
On 2011/12/06 15:29:03, Matt Menke wrote:
> Why is this needed, exactly?

Added a comment.

This was just the easiest way I found to make sure that OnPipelineFeedback() is
called in between when HttpStreamFactoryImpl::Job::OnStreamReadyCallback is
posted and when it is run.

[mmenke, 2011-12-06 19:45:37]

LGTM

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_network...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8820005/diff/1/net/http/http_pipelined_network...
net/http/http_pipelined_network_transaction_unittest.cc:722: DataRunnerObserver
observer(data_vector_[0].get(), 3);
On 2011/12/06 19:41:35, James Simonsen wrote:
> On 2011/12/06 15:29:03, Matt Menke wrote:
> > Why is this needed, exactly?
> 
> Added a comment.
> 
> This was just the easiest way I found to make sure that OnPipelineFeedback()
is
> called in between when HttpStreamFactoryImpl::Job::OnStreamReadyCallback is
> posted and when it is run.

Ahh, right.

[commit-bot: I haz the power, 2011-12-06 21:55:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/8820005/10001

[commit-bot: I haz the power, 2011-12-06 23:41:13]

Change committed as 113287