Mark QUIC broken when the network blackholes after the handshake

Mark QUIC broken when the network blackholes after the handshake,
and retry such requests in HttpNetworkTransaction.

Only when enable via the mark_quic_broken_when_network_blackholed finch
param is enabled.

BUG=705031
Review-Url: https://codereview.chromium.org/2789093003
Cr-Commit-Position: refs/heads/master@{#463067}
Committed: https://chromium.googlesource.com/chromium/src/+/9ecde09b169332d9ebd04dba35fb4bd41b129840

[Ryan Hamilton, 2017-04-03 18:43:13]

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

[Ryan Hamilton, 2017-04-03 18:43:14]

jri: Please review. Feel free to ping me in person with questions.
isherman: histograms.xml OWNERS, please.
ianswett: FYI, but please take a look.

[mmenke, 2017-04-03 18:47:46]

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

[mmenke, 2017-04-03 18:47:47]

Driveby comment

https://codereview.chromium.org/2789093003/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2789093003/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:1544: error = ERR_QUIC_PROTOCOL_ERROR;
I don't think this is meant to fall through.  Should have a test for this.

[mmenke, 2017-04-03 18:48:34]

https://codereview.chromium.org/2789093003/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2789093003/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:1544: error = ERR_QUIC_PROTOCOL_ERROR;
Also, a comment on what one error is being replaced by another might be
useful...And the reason we care about having headers, for that matter.

[Bence, 2017-04-03 19:02:58]

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

[Bence, 2017-04-03 19:02:59]

Driveby comments.

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream.h (right):

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.h:154: QuicServerId server_id_;  // The ID of
the QUIC server for this stream.
This member should be const.

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream_test.cc (right):

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream_test.cc:596: bool
mark_quic_broken_when_network_suspected_;
This member is always false, so consider a file-scope const bool
kMarkQuicBrokenWhenNetworkSuspected to highlight this fact.

[Ilya Sherman, 2017-04-03 21:05:13]

histograms.xml lgtm

[Ryan Hamilton, 2017-04-05 19:26:20]

Thanks for the drive-by!

https://codereview.chromium.org/2789093003/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2789093003/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:1544: error = ERR_QUIC_PROTOCOL_ERROR;
On 2017/04/03 18:47:47, mmenke wrote:
> I don't think this is meant to fall through. 

*facepalm* Yes indeed :( Thanks!

> Should have a test for this.

Definitely! Interestingly, after attempting to write a test for this, it turns
out that this code is unreachable because this method (HandleIOError) is not
called after headers have been received. This is a bit confusing to me because
ShouldResendRequest() also checks that headers have not been received, so it
seems a bit redundant. Curious.

Anyway, this led me to rework the logic.

https://codereview.chromium.org/2789093003/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:1544: error = ERR_QUIC_PROTOCOL_ERROR;
On 2017/04/03 18:48:34, mmenke wrote:
> Also, a comment on what one error is being replaced by another might be
> useful...And the reason we care about having headers, for that matter.

Agreed. Though this has been reworked and is now moot.

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream.h (right):

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.h:154: QuicServerId server_id_;  // The ID of
the QUIC server for this stream.
On 2017/04/03 19:02:59, Bence wrote:
> This member should be const.

Done.

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream_test.cc (right):

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream_test.cc:596: bool
mark_quic_broken_when_network_suspected_;
On 2017/04/03 19:02:59, Bence wrote:
> This member is always false, so consider a file-scope const bool
> kMarkQuicBrokenWhenNetworkSuspected to highlight this fact.

Done.

[Bence, 2017-04-05 23:49:46]

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream_test.cc (right):

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream_test.cc:596: bool
mark_quic_broken_when_network_suspected_;
On 2017/04/05 19:26:20, Ryan Hamilton wrote:
> On 2017/04/03 19:02:59, Bence wrote:
> > This member is always false, so consider a file-scope const bool
> > kMarkQuicBrokenWhenNetworkSuspected to highlight this fact.
> 
> Done.

Sorry, I was rather thinking of setting kMarkQuicBrokenWhenNetworkSuspected
false and using it instead of its inverse everywhere. 
kMarkQuicBrokenWhenNetworkSuspected would then mean "whether to mark QUIC
broken" as opposed to "the value that would need to be used if QUIC was to be
marked broken" (as if it was an enum entry).

[Jana, 2017-04-06 02:29:18]

Thanks much, Ryan! This looks great. Happy to chat in person tomorrow to go over
the logic you have in mind, but comments inline.

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

https://codereview.chromium.org/2789093003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:1537: DCHECK(GetResponseHeaders() ==
nullptr);
why is this a DCHECK and not if (ShouldResendRequest())?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:368: session_error_ = ERR_ABORTED;
Why this change?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:865: error == QUIC_TOO_MANY_RTOS;
I'm surprised to see NETWORK_IDLE_TIMEOUT  treated as a network error. I also
don't think TOO_MANY_RTOS should cause QUIC to be marked as broken... this could
simply be a disconnected network, right?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:886: // Explicit stream error are always
fatal.
nit: error -> errors
I also don't understand this comment. What are streams errors fatal to? This
returns ERR_QUIC_PROTOCOL_ERROR, that's fatal to the request, since the error
causes the request to fail, doesn't it?

Ah, ok. I understand now -- that these errors are *always* fatal. I think this
flow is a bit confusing (to me)... I think it might be more readable if
IsPossiblyNetworkError encapsulated the logic to determine if this is something
that should be treated as a network error, and have a single branch here as
follows:
if (mark_quic_broken_when_network_suspected_ && 
    !IsPossiblyNetworkError(quic_connection_error_, quic_stream_error_)) {
    return ERR_QUIC_PROTOCOL_ERROR;
}
// Mark QUIC as broken ...

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:892: DCHECK_NE(QUIC_HANDSHAKE_TIMEOUT,
quic_connection_error_);
What's the point of this DCHECK? The branch on line 873 should ensure this, no?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:897: // Error codes which could not have
been caused by the network are fatal.
the use of "fatal" is confusing... perhaps you mean to say that only network
errors should result in marking QUIC  as broken, so requests can be retried?
Also, combine this branch with the one above (line 894).

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:901: // For error codes which could have
been generated by "the network", mark
odd to say that error codes could have been generated by the network, since the
network doesn't generate these codes... how about "errors caused by network
brokenness/issues"?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:907: // If the headesr have not been
received, return ERR_QUIC_BROKEN_ERROR to
nit: "headers"

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_network_transaction_unittest.cc:1409:
quic_data.AddWrite(client_maker_.MakeDataPacket(9, kHeadersStreamId, true,
Is it easy to set the number of timeouts for connection timeout to be 1, so you
don't have to do these many RTOs? It's not worth it if it's a lot of work

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_network_transaction_unittest.cc:1422: // before
encrypting so the HTTP job will still start.
Nice -- thanks for the elaborate comments!

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_network_transaction_unittest.cc:1462:
TEST_P(QuicNetworkTransactionTest, TooManyRtosAfterHandshakeConfirmed) {
Isn't the test above sufficient to test this case? Per my read, both tests are
looking to see what happens after too many RTOs, this one uses the 5RTO code.
Maybe I'm missing something?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_network_transaction_unittest.cc:1608: // After that
fails, it will be resent via HTTP.
via TCP?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1204:
mark_quic_broken_when_network_suspected_);
Is there an easy way to test this piece? That the param is passed correctly from
construction of QuicStreamFactory to construction of QuicHttpStream?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1965: return;
Did we not need this earlier because we wouldn't enter going away state when
QUIC was marked as broken (since that would only happen when the handshake
failed)?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_test_packet_maker.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_test_packet_maker.cc:251: ack.ack_delay_time =
ack_delay_time;
why this change?

[Ryan Hamilton, 2017-04-06 03:42:10]

Patchset #3 (id:40001) has been deleted

[Ryan Hamilton, 2017-04-06 03:43:39]

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

https://codereview.chromium.org/2789093003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:1537: DCHECK(GetResponseHeaders() ==
nullptr);
On 2017/04/06 02:29:18, Jana wrote:
> why is this a DCHECK and not if (ShouldResendRequest())?

Because this error code is never returned when headers have been received. And
I'm not checking ShouldResendRequest (which an earlier patch set did) because it
turns out that HandleIOError is not called after headers have been received, so
it's rather redundant.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:865: error == QUIC_TOO_MANY_RTOS;
On 2017/04/06 02:29:18, Jana wrote:
> I'm surprised to see NETWORK_IDLE_TIMEOUT  treated as a network error. I also
> don't think TOO_MANY_RTOS should cause QUIC to be marked as broken... this
could
> simply be a disconnected network, right?

The objective is to mark QUIC as broken when the network turns into a blackhole
after the handshake. Depending on how QUIC is configured, this may result in
TOO_MANY_RTOS (if the 5RTO experiment is enabled) or IDLE_NETWORK_TIMEOUT (if
that experiment is not active). IDLE_NETWORK_TIMEOUT is of course the normal way
for a session to be closed, but since this logic is running in the stream, that
means we have a timeout with an open stream, and hence we want to mark QUIC as
broken and fallback. 

Does that help?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:886: // Explicit stream error are always
fatal.
On 2017/04/06 02:29:18, Jana wrote:
> nit: error -> errors
> I also don't understand this comment. What are streams errors fatal to? This
> returns ERR_QUIC_PROTOCOL_ERROR, that's fatal to the request, since the error
> causes the request to fail, doesn't it?
> 
> Ah, ok. I understand now -- that these errors are *always* fatal. I think this
> flow is a bit confusing (to me)... I think it might be more readable if
> IsPossiblyNetworkError encapsulated the logic to determine if this is
something
> that should be treated as a network error, and have a single branch here as
> follows:
> if (mark_quic_broken_when_network_suspected_ && 
>     !IsPossiblyNetworkError(quic_connection_error_, quic_stream_error_)) {
>     return ERR_QUIC_PROTOCOL_ERROR;
> }
> // Mark QUIC as broken ...

So it's a bit more complicated than that :/

There are 4 different ways that a stream gets closed:
1) The stream is explicitly closed by the caller
2) The session is closed by the QuicStreamFactory (say because of connection
migration or IP change)
3) The stream is reset (via RST_STREAM frame)
4) The connection is closed because of an error

The first two cases set the session_error_ member to the net error.
The third sets the quic_stream_error member to the QuicRstError code.
The fourth sets the quic_connection_error_ member to the QuicError code.

Only the fourth case results in broken, and then only when the
mark_quic_broken_when_network_suspected_ experiment is active. It's not clear to
me that moving the check of the stream error code up into IsPossiblyNetworkError
helps since those are never network caused, nor are the session_error_ which
would not be passed in. The only reason I created the IsPossiblyNetworkError()
method was to avoid enumerating them down at line 898 below, but perhaps it
would be cleaner to just remove the method and inline it there?

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:892: DCHECK_NE(QUIC_HANDSHAKE_TIMEOUT,
quic_connection_error_);
On 2017/04/06 02:29:18, Jana wrote:
> What's the point of this DCHECK? The branch on line 873 should ensure this,
no?

That's why it's a DCHECK :)

QUIC_HANDSHAKE_TIMEOUT seems like an error code that might be caused by the
network, and it IS a valid error code, but hopefully we never see it after the
handshake. Hence the DCHECK. (DCHECK are for verifying impossible conditions:

Use DCHECK() or NOTREACHED() as assertions, e.g. to document pre- and
post-conditions. A DCHECK() means “this condition must always be true”, not
“this condition is normally true, but perhaps not in exceptional cases.” Things
like disk corruption or strange network errors are examples of exceptional
circumstances that nevertheless should not result in DCHECK() failure.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:897: // Error codes which could not have
been caused by the network are fatal.
On 2017/04/06 02:29:18, Jana wrote:
> the use of "fatal" is confusing... perhaps you mean to say that only network
> errors should result in marking QUIC  as broken, so requests can be retried?
> Also, combine this branch with the one above (line 894).

By "fatal" I mean fatal to the request. Happy to hear rephrasing suggestions.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:901: // For error codes which could have
been generated by "the network", mark
On 2017/04/06 02:29:18, Jana wrote:
> odd to say that error codes could have been generated by the network, since
the
> network doesn't generate these codes... how about "errors caused by network
> brokenness/issues"?

Done.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:907: // If the headesr have not been
received, return ERR_QUIC_BROKEN_ERROR to
On 2017/04/06 02:29:18, Jana wrote:
> nit: "headers"

Done.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:907: // If the headesr have not been
received, return ERR_QUIC_BROKEN_ERROR to
On 2017/04/06 02:29:18, Jana wrote:
> nit: "headers"

Done.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_network_transaction_unittest.cc:1409:
quic_data.AddWrite(client_maker_.MakeDataPacket(9, kHeadersStreamId, true,
On 2017/04/06 02:29:18, Jana wrote:
> Is it easy to set the number of timeouts for connection timeout to be 1, so
you
> don't have to do these many RTOs? It's not worth it if it's a lot of work

I didn't see an obvious mechanism :/

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_network_transaction_unittest.cc:1422: // before
encrypting so the HTTP job will still start.
On 2017/04/06 02:29:18, Jana wrote:
> Nice -- thanks for the elaborate comments!

Yay! Thanks!

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_network_transaction_unittest.cc:1462:
TEST_P(QuicNetworkTransactionTest, TooManyRtosAfterHandshakeConfirmed) {
On 2017/04/06 02:29:18, Jana wrote:
> Isn't the test above sufficient to test this case? Per my read, both tests are
> looking to see what happens after too many RTOs, this one uses the 5RTO code.
> Maybe I'm missing something?

The previous test handles the IDLE_NETWORK_TIMEOUT case, not the too many RTOs
case.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_network_transaction_unittest.cc:1608: // After that
fails, it will be resent via HTTP.
On 2017/04/06 02:29:18, Jana wrote:
> via TCP?

Done.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1204:
mark_quic_broken_when_network_suspected_);
On 2017/04/06 02:29:18, Jana wrote:
> Is there an easy way to test this piece? That the param is passed correctly
from
> construction of QuicStreamFactory to construction of QuicHttpStream?

This happens in the QuicNetworkTransaction test where we have 2 different tests
in which we have post-handshake RTOs and post-handshake timeouts. In on version
we set mark_broken to true and in the other to false. We verify that the
HttpStream behaves as expected. So I think that does the needful. Otherwise we
could use peers to look at the privates of QuicHttpStream, but that doesn't seem
terribly awesome.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1965: return;
On 2017/04/06 02:29:18, Jana wrote:
> Did we not need this earlier because we wouldn't enter going away state when
> QUIC was marked as broken (since that would only happen when the handshake
> failed)?

It needs to happen because otherwise we'd un-mark QUIC as broken on line 1968.
:) The new QuicNetworkTransaction tests discovered this. Yay, tests.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_test_packet_maker.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_test_packet_maker.cc:251: ack.ack_delay_time =
ack_delay_time;
On 2017/04/06 02:29:18, Jana wrote:
> why this change?

This isn't a change in behavior so much as it is adding a new variant of this
method which allow the delay to be specified. The 5-arg version of this method
is unchanged and uses Zero. The new 6-arg version uses the specified delta. This
new 6-arg version is used in one of the new tests.

[Jana, 2017-04-06 23:55:24]

The CQ bit was checked by jri@chromium.org

[Jana, 2017-04-06 23:55:26]

lgtm

Thanks!

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:865: error == QUIC_TOO_MANY_RTOS;
On 2017/04/06 03:43:38, Ryan Hamilton wrote:
> On 2017/04/06 02:29:18, Jana wrote:
> > I'm surprised to see NETWORK_IDLE_TIMEOUT  treated as a network error. I
also
> > don't think TOO_MANY_RTOS should cause QUIC to be marked as broken... this
> could
> > simply be a disconnected network, right?
> 
> The objective is to mark QUIC as broken when the network turns into a
blackhole
> after the handshake. Depending on how QUIC is configured, this may result in
> TOO_MANY_RTOS (if the 5RTO experiment is enabled) or IDLE_NETWORK_TIMEOUT (if
> that experiment is not active). IDLE_NETWORK_TIMEOUT is of course the normal
way
> for a session to be closed, but since this logic is running in the stream,
that
> means we have a timeout with an open stream, and hence we want to mark QUIC as
> broken and fallback. 
> 
> Does that help?

Thanks for the explanation -- if you could add a comment to the effect that the
errors capture timeout with open streams, that would be useful. Something like
"SInce this stream is still open, a connection timeout is considered a network
error"

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:892: DCHECK_NE(QUIC_HANDSHAKE_TIMEOUT,
quic_connection_error_);
On 2017/04/06 03:43:38, Ryan Hamilton wrote:
> On 2017/04/06 02:29:18, Jana wrote:
> > What's the point of this DCHECK? The branch on line 873 should ensure this,
> no?
> 
> That's why it's a DCHECK :)
> 
> QUIC_HANDSHAKE_TIMEOUT seems like an error code that might be caused by the
> network, and it IS a valid error code, but hopefully we never see it after the
> handshake. Hence the DCHECK. (DCHECK are for verifying impossible conditions:
> 
> Use DCHECK() or NOTREACHED() as assertions, e.g. to document pre- and
> post-conditions. A DCHECK() means “this condition must always be true”, not
> “this condition is normally true, but perhaps not in exceptional cases.”
Things
> like disk corruption or strange network errors are examples of exceptional
> circumstances that nevertheless should not result in DCHECK() failure.
> 

Acknowledged.

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2789093003/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1965: return;
On 2017/04/06 03:43:39, Ryan Hamilton wrote:
> On 2017/04/06 02:29:18, Jana wrote:
> > Did we not need this earlier because we wouldn't enter going away state when
> > QUIC was marked as broken (since that would only happen when the handshake
> > failed)?
> 
> It needs to happen because otherwise we'd un-mark QUIC as broken on line 1968.
> :) The new QuicNetworkTransaction tests discovered this. Yay, tests.

Ah! Of course... Makes sense.

[Jana, 2017-04-06 23:55:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2789093003/#ps80001
(title: "better comment")

[commit-bot: I haz the power, 2017-04-06 23:56:38]

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-06 23:59:41]

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

[commit-bot: I haz the power, 2017-04-06 23:59:42]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Hamilton, 2017-04-07 18:34:01]

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

[commit-bot: I haz the power, 2017-04-07 18:34:44]

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-07 20:43:37]

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

[commit-bot: I haz the power, 2017-04-07 20:43:38]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2017-04-07 21:37:52]

Description was changed from

==========
Mark QUIC broken when there are QUIC problems which a suggest the network,
and retry such requests in HttpNetworkTransaction.
Only when enable via the mark_quic_broken_when_network_suspected finch
param is enable.

BUG=705031
==========

to

==========
Mark QUIC broken when the network blackholes after the handshake,
and retry such requests in HttpNetworkTransaction.

Only when enable via the mark_quic_broken_when_network_blackholed finch
param is enabled.

BUG=705031
==========

[Ryan Hamilton, 2017-04-07 21:39:13]

jri: As discussed, this is cleaned up and hopefully better. I've moved the logic
around, added the "broken when RTO even w/o streams" behavior and renamed the
variables/param.

[Jana, 2017-04-07 21:59:29]

Looks great and definitely simpler to reason about! Thanks. One real comment.

https://codereview.chromium.org/2789093003/diff/140001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2789093003/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:996: } else {
Nice. I like the way you refactored code to get this new function in.

https://codereview.chromium.org/2789093003/diff/140001/net/quic/chromium/quic...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2789093003/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_http_stream.cc:861: bool
IsPossiblyNetworkError(QuicErrorCode error) {
You don't need this function anymore, do you?

[Ryan Hamilton, 2017-04-07 22:08:58]

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

[commit-bot: I haz the power, 2017-04-07 22:09:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Hamilton, 2017-04-07 22:37:20]

https://codereview.chromium.org/2789093003/diff/140001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2789093003/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:996: } else {
On 2017/04/07 21:59:29, Jana wrote:
> Nice. I like the way you refactored code to get this new function in.

Yay! Thanks. It took *way* too long for me to figure this out :)

https://codereview.chromium.org/2789093003/diff/140001/net/quic/chromium/quic...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2789093003/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_http_stream.cc:861: bool
IsPossiblyNetworkError(QuicErrorCode error) {
On 2017/04/07 21:59:29, Jana wrote:
> You don't need this function anymore, do you?

Right! Good point. I should move this comment to the session though.

Thanks. DOne.

[Jana, 2017-04-07 22:49:20]

lgtm

Awesome -- thanks much!

[commit-bot: I haz the power, 2017-04-07 23:35:12]

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

[commit-bot: I haz the power, 2017-04-07 23:35:13]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2017-04-08 00:07:23]

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream_test.cc (right):

https://codereview.chromium.org/2789093003/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream_test.cc:596: bool
mark_quic_broken_when_network_suspected_;
On 2017/04/05 23:49:46, Bence wrote:
> On 2017/04/05 19:26:20, Ryan Hamilton wrote:
> > On 2017/04/03 19:02:59, Bence wrote:
> > > This member is always false, so consider a file-scope const bool
> > > kMarkQuicBrokenWhenNetworkSuspected to highlight this fact.
> > 
> > Done.
> 
> Sorry, I was rather thinking of setting kMarkQuicBrokenWhenNetworkSuspected
> false and using it instead of its inverse everywhere. 
> kMarkQuicBrokenWhenNetworkSuspected would then mean "whether to mark QUIC
> broken" as opposed to "the value that would need to be used if QUIC was to be
> marked broken" (as if it was an enum entry).

I see. As it happens, I ended up reverting the changes to this file.

[Ryan Hamilton, 2017-04-08 00:07:31]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2017-04-08 00:07:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2789093003/#ps160001
(title: "jana's comments")

[commit-bot: I haz the power, 2017-04-08 00:08:03]

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-08 00:18:28]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491610051829880,
"parent_rev": "cbe9e8bb2512a3195b85efb50d3d80d9cf6dfa03", "commit_rev":
"9ecde09b169332d9ebd04dba35fb4bd41b129840"}

[commit-bot: I haz the power, 2017-04-08 00:19:21]

Description was changed from

==========
Mark QUIC broken when the network blackholes after the handshake,
and retry such requests in HttpNetworkTransaction.

Only when enable via the mark_quic_broken_when_network_blackholed finch
param is enabled.

BUG=705031
==========

to

==========
Mark QUIC broken when the network blackholes after the handshake,
and retry such requests in HttpNetworkTransaction.

Only when enable via the mark_quic_broken_when_network_blackholed finch
param is enabled.

BUG=705031

Review-Url: https://codereview.chromium.org/2789093003
Cr-Commit-Position: refs/heads/master@{#463067}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ecde09b169332d9ebd04dba35fb...
==========

[commit-bot: I haz the power, 2017-04-08 00:19:22]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/9ecde09b169332d9ebd04dba35fb...