Remove Request from Map in OnNewSpdySessionReady().

Remove Request from Map in OnNewSpdySessionReady().

SpdySessionPool::OnNewSpdySessionReady() calls
HttpStreamFactoryImpl::Request::Complete() repeatedly while there are
any Requests in the map.  This relies on
HttpStreamRequest::Delegate::OnStreamReady() destroying the Request,
which in its destructor calls
HttpStreamFactoryImpl::JobController::OnRequestComplete(), which calls
HttpStreamFactoryImpl::JobController::CancelJobs(), which calls
HttpStreamFactoryImpl::JobController::RemoveRequestFromSpdySessionRequestMap(),
which calls SpdySessionPool::RemoveRequestFromSpdySessionRequestMap().
However, if HttpStreamRequest::Delegate::OnStreamReady() does not
destroy the Request, that results in DCHECK(!completed_) triggering in
HttpStreamFactoryImpl::Request::Complete() in a debug build, and an
infinite loop in production build.

This CL fixes this by explicitly calling
RemoveRequestFromSpdySessionRequestMap() in
SpdySessionPool::OnNewSpdySessionReady().

This revives https://crrev.com/2784143003, which was abandoned to do all
the necessary cleanup first.  Kudos to xunjieli@ for paving the way.

BUG=706974
Review-Url: https://codereview.chromium.org/2930323002
Cr-Commit-Position: refs/heads/master@{#478704}
Committed: https://chromium.googlesource.com/chromium/src/+/dcdaa5f6458c89fee3f6e2a0913707463c05171e

[Bence, 2017-06-12 15:23:59]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 15:24:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bence, 2017-06-12 16:49:40]

bnc@chromium.org changed reviewers:
+ xunjieli@chromium.org

[Bence, 2017-06-12 16:49:40]

xunjieli: PTAL.  Thanks.

[commit-bot: I haz the power, 2017-06-12 16:51:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 16:51:19]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-06-12 17:09:08]

This is great! Thank you.

https://codereview.chromium.org/2930323002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2930323002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_unittest.cc:2161: EXPECT_EQ(1,
GetSpdySessionCount(session.get()));
Could you add the following to make sure the read data are being used?

EXPECT_TRUE(data0.AllReadDataConsumed());
EXPECT_TRUE(data1.AllReadDataConsumed());

I think data1/read1 might not be used because these two requests were bound to
the first socket.

[Bence, 2017-06-12 17:33:19]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[Bence, 2017-06-12 17:33:47]

Thank you.  PTAL.

On 2017/06/12 17:09:08, xunjieli wrote:
> This is great! Thank you.
> 
>
https://codereview.chromium.org/2930323002/diff/1/net/http/http_stream_factor...
> File net/http/http_stream_factory_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2930323002/diff/1/net/http/http_stream_factor...
> net/http/http_stream_factory_impl_unittest.cc:2161: EXPECT_EQ(1,
> GetSpdySessionCount(session.get()));
> Could you add the following to make sure the read data are being used?
> 
> EXPECT_TRUE(data0.AllReadDataConsumed());
> EXPECT_TRUE(data1.AllReadDataConsumed());
> 
> I think data1/read1 might not be used because these two requests were bound to
> the first socket.

[commit-bot: I haz the power, 2017-06-12 17:33:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2017-06-12 17:50:15]

lgtm

[commit-bot: I haz the power, 2017-06-12 18:55:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 18:55:55]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-06-12 18:56:36]

Thank you.

https://codereview.chromium.org/2930323002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2930323002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_unittest.cc:2161: EXPECT_EQ(1,
GetSpdySessionCount(session.get()));
On 2017/06/12 17:09:07, xunjieli wrote:
> Could you add the following to make sure the read data are being used?
> 
> EXPECT_TRUE(data0.AllReadDataConsumed());
> EXPECT_TRUE(data1.AllReadDataConsumed());
> 
> I think data1/read1 might not be used because these two requests were bound to
> the first socket.

It seems like data1 is needed (otherwise the DCHECK fails in
SocketDataProviderArray<>::GetNext()), but read1 is in fact unnecessary.  Good
catch!

[Bence, 2017-06-12 18:56:48]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2017-06-12 18:57:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 19:02:49]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497293808422290,
"parent_rev": "293876a7b9dcaa1a8d74d63b0ab272c54f7a8dcc", "commit_rev":
"dcdaa5f6458c89fee3f6e2a0913707463c05171e"}

[commit-bot: I haz the power, 2017-06-12 19:03:03]

Description was changed from

==========
Remove Request from Map in OnNewSpdySessionReady().

SpdySessionPool::OnNewSpdySessionReady() calls
HttpStreamFactoryImpl::Request::Complete() repeatedly while there are
any Requests in the map.  This relies on
HttpStreamRequest::Delegate::OnStreamReady() destroying the Request,
which in its destructor calls
HttpStreamFactoryImpl::JobController::OnRequestComplete(), which calls
HttpStreamFactoryImpl::JobController::CancelJobs(), which calls
HttpStreamFactoryImpl::JobController::RemoveRequestFromSpdySessionRequestMap(),
which calls SpdySessionPool::RemoveRequestFromSpdySessionRequestMap().
However, if HttpStreamRequest::Delegate::OnStreamReady() does not
destroy the Request, that results in DCHECK(!completed_) triggering in
HttpStreamFactoryImpl::Request::Complete() in a debug build, and an
infinite loop in production build.

This CL fixes this by explicitly calling
RemoveRequestFromSpdySessionRequestMap() in
SpdySessionPool::OnNewSpdySessionReady().

This revives https://crrev.com/2784143003, which was abandoned to do all
the necessary cleanup first.  Kudos to xunjieli@ for paving the way.

BUG=706974
==========

to

==========
Remove Request from Map in OnNewSpdySessionReady().

SpdySessionPool::OnNewSpdySessionReady() calls
HttpStreamFactoryImpl::Request::Complete() repeatedly while there are
any Requests in the map.  This relies on
HttpStreamRequest::Delegate::OnStreamReady() destroying the Request,
which in its destructor calls
HttpStreamFactoryImpl::JobController::OnRequestComplete(), which calls
HttpStreamFactoryImpl::JobController::CancelJobs(), which calls
HttpStreamFactoryImpl::JobController::RemoveRequestFromSpdySessionRequestMap(),
which calls SpdySessionPool::RemoveRequestFromSpdySessionRequestMap().
However, if HttpStreamRequest::Delegate::OnStreamReady() does not
destroy the Request, that results in DCHECK(!completed_) triggering in
HttpStreamFactoryImpl::Request::Complete() in a debug build, and an
infinite loop in production build.

This CL fixes this by explicitly calling
RemoveRequestFromSpdySessionRequestMap() in
SpdySessionPool::OnNewSpdySessionReady().

This revives https://crrev.com/2784143003, which was abandoned to do all
the necessary cleanup first.  Kudos to xunjieli@ for paving the way.

BUG=706974

Review-Url: https://codereview.chromium.org/2930323002
Cr-Commit-Position: refs/heads/master@{#478704}
Committed:
https://chromium.googlesource.com/chromium/src/+/dcdaa5f6458c89fee3f6e2a09137...
==========

[commit-bot: I haz the power, 2017-06-12 19:03:05]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/dcdaa5f6458c89fee3f6e2a09137...