Fix pipelining crash on canceled user callbacks.

Fix pipelining crash on canceled user callbacks.

User callbacks are run in separate tasks posted by the pipelining code. If the
user closes their stream before the task runs, we shouldn't try to call the
stale callback.

BUG=101936
TEST=net_unittests


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

[James Simonsen, 2011-10-28 00:22:34]

This fixes one of the two reported crashes.

[mmenke, 2011-10-28 00:54:09]

I thought we fixed this issue already.  Anyways, LGTM, modulo comment.

http://codereview.chromium.org/8414010/diff/1/net/http/http_pipelined_connect...
File net/http/http_pipelined_connection_impl_unittest.cc (right):

http://codereview.chromium.org/8414010/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl_unittest.cc:1047:
close_stream->Close(false);
Add "MessageLoop::current()->RunAllPending();" to the end of both these tests to
make sure that if things fail, they fail in the body of the test.  While it's
run during shutdown anyways, it's easier to diagnose issues when they're
encountered in the body of a test.

[commit-bot: I haz the power, 2011-10-28 20:50:26]

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

[mmenke, 2011-10-28 20:52:01]

On 2011/10/28 20:50:26, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/simonjam%40chromium.org/8414010/1005

Skimming over the CL again...  You probably need to delete the callback and the
close_stream for the bug to cause the unit test to crash.

[James Simonsen, 2011-10-28 20:54:14]

On 2011/10/28 20:52:01, Matt Menke wrote:
> On 2011/10/28 20:50:26, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://chromium-status.appspot.com/cq/simonjam%2540chromium.org/8414010/1005
> 
> Skimming over the CL again...  You probably need to delete the callback and
the
> close_stream for the bug to cause the unit test to crash.

Oh right, yeah we can't depend on everything getting destructed any more. I'll
upload a fixed one.

[commit-bot: I haz the power, 2011-10-28 21:17:04]

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

[commit-bot: I haz the power, 2011-10-28 22:33:39]

Change committed as 107808

[willchan no longer on Chromium, 2011-11-01 18:24:43]

Man, I really ought to re-read this code at some point to get the state machine
fully in my head. Anyway, here's my post-review comments. LGTM2.

http://codereview.chromium.org/8414010/diff/3002/net/http/http_pipelined_conn...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/8414010/diff/3002/net/http/http_pipelined_conn...
net/http/http_pipelined_connection_impl.cc:573:
CHECK(stream_info_map_[pipeline_id].pending_user_callback);
In the future, a CHECK like this is unnecessary. We'll crash on the Run() call
anyway. Don't get me wrong, I like CHECKs, but I don't use them in cases like
these where it doesn't improve the diagnostics for the crash, since it has some
non-trivial logging costs in terms of binary size.