Confirm QUIC alternative service upon success.

Confirm QUIC alternative service upon success.

A non-broken but recently broken QUIC alternative service does not race for 0RTT
(because it is recently broken) but is still used (because it is not broken).
Upon successful connection, the service should be confirmed (marked as not
recently broken so that it can be used next time for 0RTT).

This CL adds this functionality and a test for it.  I locally verified that the
test fails without the rest of the change.

BUG=392575
Committed: https://crrev.com/ae8db840dac8d0c453355d3d922c91adfb61df8f
Cr-Commit-Position: refs/heads/master@{#322455}

[Bence, 2015-03-25 17:48:30]

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

[Bence, 2015-03-25 17:48:30]

Ryan: PTAL.  Do we need anything similar for the HTTP/2 case?  I don't think
recently brokenness matters for HTTP/2, so maybe we don't need to bother
confirming HTTP/2 alternative services.

[Ryan Hamilton, 2015-03-25 18:27:47]

Looks great!

https://codereview.chromium.org/1035713003/diff/1/net/quic/quic_client_sessio...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1035713003/diff/1/net/quic/quic_client_sessio...
net/quic/quic_client_session.cc:607: stream_factory_->ConfirmQuic(server_id_);
This is a minor nit, but can you do this in
QuicStreamFactory::ProcessGoingAwaySession instead? That way it keeps all the
Alt-Svc logic in that class.

[Bence, 2015-03-25 18:48:32]

Ryan: how 'bout this?

https://codereview.chromium.org/1035713003/diff/1/net/quic/quic_client_sessio...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1035713003/diff/1/net/quic/quic_client_sessio...
net/quic/quic_client_session.cc:607: stream_factory_->ConfirmQuic(server_id_);
On 2015/03/25 18:27:46, Ryan Hamilton wrote:
> This is a minor nit, but can you do this in
> QuicStreamFactory::ProcessGoingAwaySession instead? That way it keeps all the
> Alt-Svc logic in that class.

Done.

[Bence, 2015-03-25 20:11:48]

On 2015/03/25 18:48:32, Bence wrote:
> Ryan: how 'bout this?
> 
>
https://codereview.chromium.org/1035713003/diff/1/net/quic/quic_client_sessio...
> File net/quic/quic_client_session.cc (right):
> 
>
https://codereview.chromium.org/1035713003/diff/1/net/quic/quic_client_sessio...
> net/quic/quic_client_session.cc:607: stream_factory_->ConfirmQuic(server_id_);
> On 2015/03/25 18:27:46, Ryan Hamilton wrote:
> > This is a minor nit, but can you do this in
> > QuicStreamFactory::ProcessGoingAwaySession instead? That way it keeps all
the
> > Alt-Svc logic in that class.
> 
> Done.

Oops, Version/QuicNetworkTransactionTest.FailedZeroRttBrokenAlternateProtocol/*
fails, let me fix that first.

[Bence, 2015-03-25 21:50:16]

Hi Ryan, I'm just trying out things here.  Patch Set 3 seems to fix the issue,
at least when I run tests locally.  To call ConfirmAlternativeService in
ProcessGoingAwaySession, I would need to determine whether the connection was
successful.  How would I do that?  Also, what happens if the connection is
successful but then cancelled (by the user)?  We want to make sure to handle
that case as well.  Or can we just call it from OnResolution, like I do in Patch
Set 3, which seems to work fine?

[Ryan Hamilton, 2015-03-26 00:23:46]

On 2015/03/25 21:50:16, Bence wrote:
> Hi Ryan, I'm just trying out things here.  Patch Set 3 seems to fix the issue,
> at least when I run tests locally.  To call ConfirmAlternativeService in
> ProcessGoingAwaySession, I would need to determine whether the connection was
> successful.  How would I do that?  Also, what happens if the connection is
> successful but then cancelled (by the user)?  We want to make sure to handle
> that case as well.  Or can we just call it from OnResolution, like I do in
Patch
> Set 3, which seems to work fine?

I think you want to put it in ProcessGoingAwaySession, inside of:

 1144   if (session->IsCryptoHandshakeConfirmed()) {

Cool? BTW, users can cancel streams, but not sessions.

[Bence, 2015-03-26 13:36:36]

Cool.  PTAL.  Thanks for your help.

[Ryan Hamilton, 2015-03-26 19:12:03]

lgtm

[Bence, 2015-03-26 20:10:12]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-03-26 20:10:31]

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

[commit-bot: I haz the power, 2015-03-26 20:13:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-26 20:14:43]

Patchset 5 (id:??) landed as
https://crrev.com/ae8db840dac8d0c453355d3d922c91adfb61df8f
Cr-Commit-Position: refs/heads/master@{#322455}