Fix bug in SpdySession

Fix bug in SpdySession where map.erase() was called on an iterator when the underlying map had changed, making the iterator invalid.

BUG=139518
TEST=SpdySessionSpdy*Test.CloseSessionWithTwoCreatedStreams


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

[Ryan Sleevi, 2012-07-30 06:36:15]

drive by checks

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
File net/spdy/spdy_session_spdy2_unittest.cc (right):

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1389: namespace {
drive by nit:

According to
http://www.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces , unnamed
namespaces appear first in the file (eg: before code from the .h). Most code
I've seen follows this convention.

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1395: virtual bool
OnSendHeadersComplete(int status) {
comment nit:
// SpdyStream::Delegate implementation:

style nit: Make sure to annotate as OVERRIDE the methods you are overriding (
http://crbug.com/115047 )

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1420: TEST_F(SpdySessionSpdy2Test,
CloseSessionWithTwoCreatedStreams) {
minor nit: Comment about what the test tests ;-)

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1423: // Request 2, at LOWEST priority,
will be a full request and will be id 1.
BUG? I don't see where you test ID EQ 1 (see below)

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1490: EXPECT_EQ(0u,
spdy_stream2->stream_id());
BUG? - Comment suggests it should be EQ 1

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1520: EXPECT_EQ(0u,
spdy_stream2->stream_id());
Seems like the EXPECT_EQ here and on lines 1514-1515 are the important part of
the tests. Can you add some comments about the pre-conditions / post-conditions
you're expecting to test here?

[Ryan Hamilton, 2012-07-30 16:21:13]

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
File net/spdy/spdy_session_spdy2_unittest.cc (right):

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1389: namespace {
On 2012/07/30 06:36:15, Ryan Sleevi wrote:
> drive by nit:
> 
> According to
> http://www.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces ,
unnamed
> namespaces appear first in the file (eg: before code from the .h). Most code
> I've seen follows this convention.

Done.  I had thought it made more sense to put the helper near the code where it
is used, but I've moved it per the style guide.

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1395: virtual bool
OnSendHeadersComplete(int status) {
On 2012/07/30 06:36:15, Ryan Sleevi wrote:
> comment nit:
> // SpdyStream::Delegate implementation:
> 
> style nit: Make sure to annotate as OVERRIDE the methods you are overriding (
> http://crbug.com/115047 )

Done.

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1420: TEST_F(SpdySessionSpdy2Test,
CloseSessionWithTwoCreatedStreams) {
On 2012/07/30 06:36:15, Ryan Sleevi wrote:
> minor nit: Comment about what the test tests ;-)

Done.

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1423: // Request 2, at LOWEST priority,
will be a full request and will be id 1.
On 2012/07/30 06:36:15, Ryan Sleevi wrote:
> BUG? I don't see where you test ID EQ 1 (see below)

Sorry, I copied this test from the previous test and then modified it to
reproduce the crash.  I failed to clean it up correctly and this comment is
totally irrelevant to this test.

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1490: EXPECT_EQ(0u,
spdy_stream2->stream_id());
On 2012/07/30 06:36:15, Ryan Sleevi wrote:
> BUG? - Comment suggests it should be EQ 1

No, the comment was bogus :<

http://codereview.chromium.org/10821086/diff/6001/net/spdy/spdy_session_spdy2...
net/spdy/spdy_session_spdy2_unittest.cc:1520: EXPECT_EQ(0u,
spdy_stream2->stream_id());
On 2012/07/30 06:36:15, Ryan Sleevi wrote:
> Seems like the EXPECT_EQ here and on lines 1514-1515 are the important part of
> the tests. Can you add some comments about the pre-conditions /
post-conditions
> you're expecting to test here?

I've revised this and added a comment that explains what the test is testing at
the top.  The point, really, is that spdy session does not crash :>

[ramant (doing other things), 2012-07-30 21:20:31]

lgtm

[commit-bot: I haz the power, 2012-07-30 21:27:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10821086/11001

[commit-bot: I haz the power, 2012-07-30 23:18:33]

Change committed as 149071