Fix crash in handling PushStreams

Fix crash in handling PushStreams

"pushed_it" created early can point to expired entry.
DeleteExpiredPushedStreams will remove actual entry invalidating "pushed_it"
iterator. std::map::insert will crash when invalid iterator passed as hint.

Swap inserting new element and removing expired ones so iterator will stay
valid until late.

BUG=443490
R=bnc@chromium.org, rch@chromium.org

Committed: https://crrev.com/3bf914c745aa34ba3bce47ec6e1d0b4aee7f0eb0
Cr-Commit-Position: refs/heads/master@{#311345}

[szym, 2014-12-18 06:25:00]

-szym
Leaving this one to Johnny.

[szym, 2014-12-18 06:25:13]

szym@chromium.org changed reviewers:
- szym@chromium.org

[szym, 2014-12-18 06:27:18]

szym@chromium.org changed reviewers:
+ szym@chromium.org

[szym, 2014-12-18 06:34:38]

https://codereview.chromium.org/813993002/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (left):

https://codereview.chromium.org/813993002/diff/1/net/spdy/spdy_session.cc#old...
net/spdy/spdy_session.cc:2706: DeleteExpiredPushedStreams();
This is definitely a bug and the solution makes sense to me.

The only thing that I'd ask for is a unit test to cover this situation.

[baranovich, 2014-12-18 20:25:32]

I would suggest moving DeleteExpiredPushedStreams() before we check for
duplicate. Now code may end up finding expired duplicated pushed stream.

[bacek_yandex, 2014-12-18 23:46:31]

bacek@yandex-team.ru changed reviewers:
+ bnc@chromium.org, rch@chromium.org
- jgraettinger@chromium.org, szym@chromium.org

[cbentzel, 2014-12-19 01:54:48]

cbentzel@chromium.org changed required reviewers:
+ bnc@chromium.org

[Ryan Hamilton, 2015-01-07 19:32:14]

https://codereview.chromium.org/813993002/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (left):

https://codereview.chromium.org/813993002/diff/1/net/spdy/spdy_session.cc#old...
net/spdy/spdy_session.cc:2706: DeleteExpiredPushedStreams();
On 2014/12/18 06:34:38, szym wrote:
> This is definitely a bug and the solution makes sense to me.
> 
> The only thing that I'd ask for is a unit test to cover this situation.

+1 for a test.

[bacek_yandex, 2015-01-07 23:24:36]

On 2015/01/07 19:32:14, Ryan Hamilton wrote:
> > The only thing that I'd ask for is a unit test to cover this situation.
> 
> +1 for a test.

It's almost impossible to write test for incorrect usage of std::map...

I can try though.

[bacek_yandex, 2015-01-09 00:13:36]

On 2015/01/07 23:24:36, bacek_yandex wrote:
> On 2015/01/07 19:32:14, Ryan Hamilton wrote:
> > > The only thing that I'd ask for is a unit test to cover this situation.
> > 
> > +1 for a test.
> 
> It's almost impossible to write test for incorrect usage of std::map...
> 
> I can try though.

Ok, it wasn't _so_ hard actually.

[bacek_yandex, 2015-01-09 00:14:21]

Unittest added.

[bacek_yandex, 2015-01-09 00:55:35]

The CQ bit was checked by bacek@yandex-team.ru

[bacek_yandex, 2015-01-09 00:55:35]

The CQ bit was unchecked by bacek@yandex-team.ru

[Ryan Hamilton, 2015-01-09 19:00:05]

On 2015/01/09 00:14:21, bacek_yandex wrote:
> Unittest added.

To confirm, this test fails (crashes?) w/o your fix? If so, LGTM

[bacek_yandex, 2015-01-11 08:00:18]

On 2015/01/09 19:00:05, Ryan Hamilton wrote:
> On 2015/01/09 00:14:21, bacek_yandex wrote:
> > Unittest added.
> 
> To confirm, this test fails (crashes?) w/o your fix? If so, LGTM

It is indeed.

~/src/chromium/src$ ninja -C out/Debug net_unittests &&
./out/Debug/net_unittests
--gtest_filter=NextProto/SpdySessionTest.DeleteExpiredPushStreams/0
ninja: Entering directory `out/Debug'
ninja: no work to do.
[0111/185945:ERROR:nss_util.cc(819)] After loading Root Certs, loaded==false:
NSS error code: -8018
IMPORTANT DEBUGGING NOTE: batches of tests are run inside their
own process. For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with
--single-process-tests.
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
[0111/185946:ERROR:nss_util.cc(819)] After loading Root Certs, loaded==false:
NSS error code: -8018
Note: Google Test filter = NextProto/SpdySessionTest.DeleteExpiredPushStreams/0
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from NextProto/SpdySessionTest
[ RUN      ] NextProto/SpdySessionTest.DeleteExpiredPushStreams/0
/usr/lib/gcc/x86_64-linux-gnu/4.9/../../../../include/c++/4.9/debug/safe_iterator.h:207:
    error: attempt to construct a constant iterator from a singular mutable 
    iterator.

Objects involved in the operation:
iterator "this" @ 0x0x7ffff0642ae8 {
  state = dereferenceable;
  references sequence @ 0x0xfded5687140
}
iterator "other" @ 0x0x7ffff0642ba8 {
  state = singular;
  references sequence @ 0x0xfded5687140
}
Received signal 6
#0 0x7f27bac1349e base::debug::StackTrace::StackTrace()
#1 0x7f27bac12fd3 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f27b796f8d0 <unknown>
#3 0x7f27b75ec107 gsignal
#4 0x7f27b75ed4e8 abort
#5 0x7f27b8148655 __gnu_debug::_Error_formatter::_M_error()
#6 0x7f27ba44208f
_ZN11__gnu_debug14_Safe_iteratorISt23_Rb_tree_const_iteratorISt4pairIK4GURLN3net11SpdySession16PushedStreamInfoEEENSt7__debug3mapIS3_S7_St4lessIS3_ESaIS8_EEEEC2ISt17_Rb_tree_iteratorIS8_EEERKNS0_IT_N9__gnu_cxx11__enable_ifIXsr3std10__are_sameISK_SJ_EE7__valueESF_E6__typeEEE
#7 0x7f27ba43654c net::SpdySession::TryCreatePushStream()
#8 0x7f27ba4353c0 net::SpdySession::OnSynStream()
#9 0x7f27ba3c1f43 net::BufferedSpdyFramer::OnControlFrameHeaderData()
#10 0x7f27ba3fb903 net::SpdyFramer::ProcessControlFrameHeaderBlock()
#11 0x7f27ba3f739c net::SpdyFramer::ProcessInput()
#12 0x7f27ba3c29f7 net::BufferedSpdyFramer::ProcessInput()
#13 0x7f27ba42e8e0 net::SpdySession::DoReadComplete()
#14 0x7f27ba42ddf8 net::SpdySession::DoReadLoop()
#15 0x7f27ba427eea net::SpdySession::PumpReadLoop()
#16 0x7f27ba44d0ae base::internal::RunnableAdapter<>::Run()
#17 0x7f27ba44cfed base::internal::InvokeHelper<>::MakeItSo()
#18 0x7f27ba44cf64 base::internal::Invoker<>::Run()
#19 0x0000005891c6 base::Callback<>::Run()
#20 0x0000018db985 net::DeterministicSocketHelper::CompleteRead()
#21 0x0000018dce8c net::DeterministicMockTCPClientSocket::CompleteRead()
#22 0x0000018dcebc net::DeterministicMockTCPClientSocket::CompleteRead()
#23 0x0000018d814d net::DeterministicSocketData::InvokeCallbacks()
#24 0x0000018d7ea1 net::DeterministicSocketData::Run()
#25 0x0000018d818c net::DeterministicSocketData::RunFor()
#26 0x0000014b0868
net::SpdySessionTest_DeleteExpiredPushStreams_Test::TestBody()
#27 0x00000183ba33 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#28 0x00000182992e testing::internal::HandleExceptionsInMethodIfSupported<>()
#29 0x00000181dac5 testing::Test::Run()
#30 0x00000181e1bb testing::TestInfo::Run()
#31 0x00000181e76a testing::TestCase::Run()
#32 0x000001823c83 testing::internal::UnitTestImpl::RunAllTests()
#33 0x000001835ab3 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#34 0x00000182b85e testing::internal::HandleExceptionsInMethodIfSupported<>()
#35 0x000001823921 testing::UnitTest::Run()
#36 0x0000019a4781 RUN_ALL_TESTS()
#37 0x0000019a3667 base::TestSuite::Run()
#38 0x000001567dd2 base::internal::RunnableAdapter<>::Run()
#39 0x000001567d3f base::internal::InvokeHelper<>::MakeItSo()
#40 0x000001567cea base::internal::Invoker<>::Run()
#41 0x00000199979e base::Callback<>::Run()
#42 0x000001995a2a base::(anonymous namespace)::LaunchUnitTestsInternal()
#43 0x0000019956d7 base::LaunchUnitTests()
#44 0x000001567a65 main
#45 0x7f27b75d8b45 __libc_start_main
#46 0x000000537bd4 <unknown>
  r8: 00007f27b795c790  r9: 00007f27b7959460 r10: 0000000000000008 r11:
0000000000000206
 r12: 0000000000000002 r13: 00007ffff0649190 r14: 8b9e000f26b71b00 r15:
0000000000000000
  di: 0000000000003baf  si: 0000000000003baf  bp: 00007ffff0642778  bx:
00007ffff06426f8
  dx: 0000000000000006  ax: 0000000000000000  cx: ffffffffffffffff  sp:
00007ffff06424d8
  ip: 00007f27b75ec107 efl: 0000000000000206 cgf: 0000000000000033 erf:
0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[0111/185946:ERROR:kill_posix.cc(194)] Unable to terminate process group 15279:
No such process
[1/1] NextProto/SpdySessionTest.DeleteExpiredPushStreams/0 (CRASHED)
1 test crashed:
    NextProto/SpdySessionTest.DeleteExpiredPushStreams/0
Tests took 0 seconds.

[Ryan Hamilton, 2015-01-12 01:01:42]

LGTM

[bacek_yandex, 2015-01-13 03:45:35]

On 2015/01/12 01:01:42, Ryan Hamilton wrote:
> LGTM

Should I wait for Bence? Or find another reviewer? Or commit it straight away?

[Bence, 2015-01-13 15:34:30]

bnc@chromium.org changed reviewers:
+ bacek@yandex-team.ru
- Bacek@yandex-team.ru

[Bence, 2015-01-13 15:34:31]

LGTM, sorry for the delay.

[bacek_yandex, 2015-01-13 21:23:01]

The CQ bit was checked by bacek@yandex-team.ru

[commit-bot: I haz the power, 2015-01-13 21:23:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/813993002/20001

[commit-bot: I haz the power, 2015-01-13 22:20:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-13 22:20:47]

Patchset 2 (id:??) landed as
https://crrev.com/3bf914c745aa34ba3bce47ec6e1d0b4aee7f0eb0
Cr-Commit-Position: refs/heads/master@{#311345}