[Domain Reliabiliy: net stack] Plumb received go away from server due to connection migration.

Plumb received go away from server due to connection migration.

BUG=593163
Committed: https://crrev.com/6b5a389af3231391673ca3654034571a17a53034
Cr-Commit-Position: refs/heads/master@{#380849}

[Zhongyi Shi, 2016-03-09 02:26:37]

Description was changed from

==========
[Domain Reliabiliy: net stack] Plumb received go away from server due to
connection migration.

BUG=
==========

to

==========
Plumb received go away from server due to connection migration.

BUG=
==========

[Zhongyi Shi, 2016-03-09 17:35:25]

zhongyi@chromium.org changed reviewers:
+ juliatuttle@chromium.org, rch@chromium.org, ttuttle@chromium.org

[Zhongyi Shi, 2016-03-09 17:35:26]

This patch tried to identify a GoAway frame is received from server for
connection migration and plumb the flag all the way to up layers to help us
identify potential omg/1066 blackholing if the GoAwayFrame is not handled
correctly. PTAL. Thanks!

[Ryan Hamilton, 2016-03-09 19:53:03]

https://codereview.chromium.org/1775283002/diff/1/net/quic/quic_chromium_clie...
File net/quic/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/1775283002/diff/1/net/quic/quic_chromium_clie...
net/quic/quic_chromium_client_session.cc:1041:
stream_factory_->OnSessionGoingAwayForConnectionMigration();
Instead of doing this, can we have the session store some piece of state
(did_server_send_goaway_because_of_migration_) that the factory can query in the
OnSessionGoingAway() method?

https://codereview.chromium.org/1775283002/diff/1/net/quic/quic_stream_factor...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1775283002/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:1702: goaway_received_for_connection_mirgation_;
I don't understand how this works. The HttpNetworkTransaction is associated with
a single request. It then queries the HttpNetworkSession, which queries the
QuicStreamFactory. But the Factory is not associated with a request. It looks
like if there are 2 requests they will both end up getting the same value for
this field, even if one of them does not use QUIC. I think you'll need to move
this logic to something that is per-request (or per stream).

[Zhongyi Shi, 2016-03-10 21:56:05]

Description was changed from

==========
Plumb received go away from server due to connection migration.

BUG=
==========

to

==========
Plumb received go away from server due to connection migration.

BUG=593163
==========

[Zhongyi Shi, 2016-03-10 22:26:08]

Change the plumbing path to bind the flag per request. PTAL.

[Ryan Hamilton, 2016-03-10 22:41:34]

rch@chromium.org changed reviewers:
- juliatuttle@chromium.org

[Ryan Hamilton, 2016-03-10 22:46:44]

Looks much better!

https://codereview.chromium.org/1775283002/diff/40001/net/base/net_error_deta...
File net/base/net_error_details.h (right):

https://codereview.chromium.org/1775283002/diff/40001/net/base/net_error_deta...
net/base/net_error_details.h:38: bool received_goaway_because_of_migrating_port;
nit: since this is quic specific, it should probably start with quic_, so how
about quic_port_migration_failed?

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.h (right):

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:339: bool
received_goaway_because_of_migrating_port_;
nit: port_migration_failed?

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_http_stre...
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_http_stre...
net/quic/quic_http_stream.cc:440:
stream_request_.PopulateNetErrorDetails(details);
Instead of asking the stream_request_ to ask the session, can we just ask the
session_ directly?

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory_test.cc:632:
session->PopulateNetErrorDetails(&details);
Can you also call stream->PopulateNetErrorDetails()?

[Zhongyi Shi, 2016-03-11 03:53:54]

Patchset #4 (id:60001) has been deleted

[Zhongyi Shi, 2016-03-11 03:56:32]

Thanks Ryan! 

Spent sometime to figure out the difference between receiving a goaway and
connectionclose. Now unit tests are also online :D PTAL..

https://codereview.chromium.org/1775283002/diff/40001/net/base/net_error_deta...
File net/base/net_error_details.h (right):

https://codereview.chromium.org/1775283002/diff/40001/net/base/net_error_deta...
net/base/net_error_details.h:38: bool received_goaway_because_of_migrating_port;
On 2016/03/10 22:46:44, Ryan Hamilton wrote:
> nit: since this is quic specific, it should probably start with quic_, so how
> about quic_port_migration_failed?

maybe quic_port_migration_detected? I hoped to keep the same var name in
quic_chromium_client_session, having it called failed sounds confusing there.

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.h (right):

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:339: bool
received_goaway_because_of_migrating_port_;
On 2016/03/10 22:46:44, Ryan Hamilton wrote:
> nit: port_migration_failed?

Done.

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_http_stre...
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_http_stre...
net/quic/quic_http_stream.cc:440:
stream_request_.PopulateNetErrorDetails(details);
On 2016/03/10 22:46:44, Ryan Hamilton wrote:
> Instead of asking the stream_request_ to ask the session, can we just ask the
> session_ directly?

Yes, we can, I knew there's a stream_request_ in quic_http_stream and has ref to
session when we try to send the request, but missed the part that
quic_http_stream has direct ref to session. Super easy now!

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1775283002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory_test.cc:632:
session->PopulateNetErrorDetails(&details);
On 2016/03/10 22:46:44, Ryan Hamilton wrote:
> Can you also call stream->PopulateNetErrorDetails()? 

Done.

[Ryan Hamilton, 2016-03-11 22:48:58]

On suggestion. Looks good otherwise.

https://codereview.chromium.org/1775283002/diff/100001/net/quic/quic_http_str...
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/1775283002/diff/100001/net/quic/quic_http_str...
net/quic/quic_http_stream.cc:440: if (session_)
In the case that there is no session, I wonder if we're going to have missed
this information. In other words, when the session becomes disconnected from the
QuicHttpStream, can we grab the port_migration_detected value and save it in
case PopulateNetErrorDetails is called later.

[Zhongyi Shi, 2016-03-11 23:15:30]

Thanks Ryan for the suggestion! This patch now caches the flag in
quic_http_stream when the underlying quic_chromium_client_session is closed.
PTAL:D

https://codereview.chromium.org/1775283002/diff/100001/net/quic/quic_http_str...
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/1775283002/diff/100001/net/quic/quic_http_str...
net/quic/quic_http_stream.cc:440: if (session_)
On 2016/03/11 22:48:58, Ryan Hamilton wrote:
> In the case that there is no session, I wonder if we're going to have missed
> this information. In other words, when the session becomes disconnected from
the
> QuicHttpStream, can we grab the port_migration_detected value and save it in
> case PopulateNetErrorDetails is called later.

Good point! I changed the method of OnSessionClosed to notify stream to cache
the flag.

[Ryan Hamilton, 2016-03-11 23:46:16]

lgtm

Thanks!

https://codereview.chromium.org/1775283002/diff/120001/net/quic/quic_http_str...
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/1775283002/diff/120001/net/quic/quic_http_str...
net/quic/quic_http_stream.cc:527: port_migration_detected_ =
port_migration_detected;
nit: I think there's no need for the conditional since this method should only
be called once, and the member is initialized to false.

[Zhongyi Shi, 2016-03-11 23:57:23]

Thanks for reviewing :D

Landing this patch, once the other patch in domain_reliability is shipped
(hopefully next Mon, since CAM folks are off work now), patch #2 will be
finished.

https://codereview.chromium.org/1775283002/diff/120001/net/quic/quic_http_str...
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/1775283002/diff/120001/net/quic/quic_http_str...
net/quic/quic_http_stream.cc:527: port_migration_detected_ =
port_migration_detected;
On 2016/03/11 23:46:16, Ryan Hamilton wrote:
> nit: I think there's no need for the conditional since this method should only
> be called once, and the member is initialized to false.

Done.

[Zhongyi Shi, 2016-03-11 23:57:41]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2016-03-11 23:57:41]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1775283002/#ps140001
(title: "remove conditional assignment")

[commit-bot: I haz the power, 2016-03-11 23:58:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1775283002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1775283002/140001

[commit-bot: I haz the power, 2016-03-12 04:46:30]

Description was changed from

==========
Plumb received go away from server due to connection migration.

BUG=593163
==========

to

==========
Plumb received go away from server due to connection migration.

BUG=593163
==========

[commit-bot: I haz the power, 2016-03-12 04:46:31]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-03-12 04:48:00]

Description was changed from

==========
Plumb received go away from server due to connection migration.

BUG=593163
==========

to

==========
Plumb received go away from server due to connection migration.

BUG=593163

Committed: https://crrev.com/6b5a389af3231391673ca3654034571a17a53034
Cr-Commit-Position: refs/heads/master@{#380849}
==========

[commit-bot: I haz the power, 2016-03-12 04:48:01]

Patchset 7 (id:??) landed as
https://crrev.com/6b5a389af3231391673ca3654034571a17a53034
Cr-Commit-Position: refs/heads/master@{#380849}