When HttpNetworkTransaction encounters QUIC errors, retry the request

When HttpNetworkTransaction encounters QUIC errors, retry the request
with alt-svc disabled, and if that succeeds then mark QUIC as broken.

Protected by the retry_without_alt_svc_on_quic_errors finch param.

BUG=705033
Review-Url: https://codereview.chromium.org/2818623002
Cr-Original-Commit-Position: refs/heads/master@{#464391}
Committed: https://chromium.googlesource.com/chromium/src/+/d0dbccf1f8d23dfa32defdd02c0a3df990729358
Review-Url: https://codereview.chromium.org/2818623002
Cr-Commit-Position: refs/heads/master@{#464502}
Committed: https://chromium.googlesource.com/chromium/src/+/2f2991ce896d93583dc3dab2ba6466fcae3d23b3

[Ryan Hamilton, 2017-04-12 23:32:44]

Description was changed from

==========
When HttpNetworkTransaction encounters QUIC errors, retry the request
with alt-svc disabled, and if that succeeds then mark QUIC as broken.

Protected by the retry_without_alt_svc_on_quic_errors finch param.
==========

to

==========
When HttpNetworkTransaction encounters QUIC errors, retry the request
with alt-svc disabled, and if that succeeds then mark QUIC as broken.

Protected by the retry_without_alt_svc_on_quic_errors finch param.

BUG=705033
==========

[Ryan Hamilton, 2017-04-12 23:32:56]

rch@chromium.org changed reviewers:
+ ianswett@chromium.org, jri@chromium.org

[Ryan Hamilton, 2017-04-13 02:41:29]

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

[commit-bot: I haz the power, 2017-04-13 02:42:18]

Dry run: 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-04-13 04:17:56]

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

[commit-bot: I haz the power, 2017-04-13 04:17:57]

Dry run: This issue passed the CQ dry run.

[Jana, 2017-04-13 06:21:58]

lgtm

Awesome - thanks much! I can't think of a way to do this, but I wonder if we
could do an "end-to-end" test that actually causes these failure modes to
confirm that Chrome works as expected.

[Ryan Hamilton, 2017-04-13 13:36:34]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2017-04-13 13:36:46]

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-04-13 13:40:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492090594641420,
"parent_rev": "ebbc5aae76b78ca9d118091a711588fd9fa06584", "commit_rev":
"d0dbccf1f8d23dfa32defdd02c0a3df990729358"}

[commit-bot: I haz the power, 2017-04-13 13:45:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492090594641420,
"parent_rev": "ebbc5aae76b78ca9d118091a711588fd9fa06584", "commit_rev":
"d0dbccf1f8d23dfa32defdd02c0a3df990729358"}

[commit-bot: I haz the power, 2017-04-13 13:46:16]

Description was changed from

==========
When HttpNetworkTransaction encounters QUIC errors, retry the request
with alt-svc disabled, and if that succeeds then mark QUIC as broken.

Protected by the retry_without_alt_svc_on_quic_errors finch param.

BUG=705033
==========

to

==========
When HttpNetworkTransaction encounters QUIC errors, retry the request
with alt-svc disabled, and if that succeeds then mark QUIC as broken.

Protected by the retry_without_alt_svc_on_quic_errors finch param.

BUG=705033

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

[commit-bot: I haz the power, 2017-04-13 13:46:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d0dbccf1f8d23dfa32defdd02c0a...

[ianswett, 2017-04-13 14:02:46]

Thanks Ryan, looks good, though a few extra comments would be nice.

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1365: retried_alternative_service_.protocol
!= kProtoUnknown) {
To clarify, does this break any alt-svc, not just QUIC?

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1570: if (GetResponseHeaders() != nullptr
||
Can you add some comments here?  I believe I understand this, but I'm not as
familiar with this code as I'd like.

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.h:385: AlternativeService
retried_alternative_service_;
This sort of mechanism could be useful for TFO and TLS 1.3(if there are
deployment issues with that), as well.

[jdoerrie, 2017-04-13 15:05:55]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2811993005/ by jdoerrie@chromium.org.

The reason for reverting is: Likely cause of compilation error in Win x64.

BUG=711316.

[Ryan Hamilton, 2017-04-13 17:18:26]

Description was changed from

==========
When HttpNetworkTransaction encounters QUIC errors, retry the request
with alt-svc disabled, and if that succeeds then mark QUIC as broken.

Protected by the retry_without_alt_svc_on_quic_errors finch param.

BUG=705033

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

to

==========
When HttpNetworkTransaction encounters QUIC errors, retry the request
with alt-svc disabled, and if that succeeds then mark QUIC as broken.

Protected by the retry_without_alt_svc_on_quic_errors finch param.

BUG=705033

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

[Ryan Hamilton, 2017-04-13 17:26:56]

Thanks!

Re-landing since I'm quite confident this CL was not the cause of the build
breakage.

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1365: retried_alternative_service_.protocol
!= kProtoUnknown) {
On 2017/04/13 14:02:45, ianswett wrote:
> To clarify, does this break any alt-svc, not just QUIC?

Yes, this code here is not QUIC specific so it would mark any alt-svc as broken.

That said, the only code that sets retried_alternative_service_ does so in the
QUIC case. So for the time being, this is "QUIC specific".

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1570: if (GetResponseHeaders() != nullptr
||
On 2017/04/13 14:02:45, ianswett wrote:
> Can you add some comments here?  I believe I understand this, but I'm not as
> familiar with this code as I'd like.

Done.

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/2818623002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.h:385: AlternativeService
retried_alternative_service_;
On 2017/04/13 14:02:46, ianswett wrote:
> This sort of mechanism could be useful for TFO and TLS 1.3(if there are
> deployment issues with that), as well.

*nod* I don't think those systems use alternative services, but a retry-on-error
+ disable on success could definitely work!

[Ryan Hamilton, 2017-04-13 17:27:26]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2017-04-13 17:27:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jri@chromium.org
Link to the patchset: https://codereview.chromium.org/2818623002/#ps60001
(title: "More comments")

[commit-bot: I haz the power, 2017-04-13 17:28:02]

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-04-13 19:28:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492104446045910,
"parent_rev": "3ca2419cbb0dc16043535a1ec1e04b4977e725c1", "commit_rev":
"2f2991ce896d93583dc3dab2ba6466fcae3d23b3"}

[commit-bot: I haz the power, 2017-04-13 19:29:15]

Description was changed from

==========
When HttpNetworkTransaction encounters QUIC errors, retry the request
with alt-svc disabled, and if that succeeds then mark QUIC as broken.

Protected by the retry_without_alt_svc_on_quic_errors finch param.

BUG=705033

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

to

==========
When HttpNetworkTransaction encounters QUIC errors, retry the request
with alt-svc disabled, and if that succeeds then mark QUIC as broken.

Protected by the retry_without_alt_svc_on_quic_errors finch param.

BUG=705033

Review-Url: https://codereview.chromium.org/2818623002
Cr-Original-Commit-Position: refs/heads/master@{#464391}
Committed:
https://chromium.googlesource.com/chromium/src/+/d0dbccf1f8d23dfa32defdd02c0a...
Review-Url: https://codereview.chromium.org/2818623002
Cr-Commit-Position: refs/heads/master@{#464502}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f2991ce896d93583dc3dab2ba64...
==========

[commit-bot: I haz the power, 2017-04-13 19:29:16]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2f2991ce896d93583dc3dab2ba64...