When QUIC is disabled because of timeouts with open streams, re-enable

When QUIC is disabled because of timeouts with open streams, re-enable
QUIC if TCP jobs are also failing, since this likely indicates a
loss of network, not a problem with QUIC.

BUG=571098
Committed: https://crrev.com/cee7bebda0eda4ce9316e72459e33995b4bd4f12
Cr-Commit-Position: refs/heads/master@{#380553}

[Ryan Hamilton, 2016-03-05 00:34:27]

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

[Ryan Hamilton, 2016-03-05 00:34:28]

rtenneti: PTAL
ianswett: FYI

[ramant (doing other things), 2016-03-05 00:55:18]

lgtm

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

https://codereview.chromium.org/1768593003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:1156: if (disabled_reason ==
QuicChromiumClientSession::QUIC_DISABLED_NOT)
nit: a small question when "disabled_reason ==
QuicChromiumClientSession::QUIC_DISABLED_NOT", status_ should be OPEN and 
IsQuicDisabled(port) should be false. Wondering if status_ being a  CLOSED is a
valid status?. I am guessing that status_ is CLOSED should never happen. thanks.

IMO, in the worst case, if this were ever to happen, 
Net.QuicStreamFactory.DisabledReasons histogram will have count for 
QUIC_DISABLED_NOT.

https://codereview.chromium.org/1768593003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:1163: // to either fully disabled or back to
open
overly nit. "." at the end of line#1163.

[Ryan Hamilton, 2016-03-05 00:59:50]

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

https://codereview.chromium.org/1768593003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:1156: if (disabled_reason ==
QuicChromiumClientSession::QUIC_DISABLED_NOT)
On 2016/03/05 00:55:18, ramant wrote:
> nit: a small question when "disabled_reason ==
> QuicChromiumClientSession::QUIC_DISABLED_NOT", status_ should be OPEN and 
> IsQuicDisabled(port) should be false. Wondering if status_ being a  CLOSED is
a
> valid status?. I am guessing that status_ is CLOSED should never happen.
thanks.
> 
> IMO, in the worst case, if this were ever to happen, 
> Net.QuicStreamFactory.DisabledReasons histogram will have count for 
> QUIC_DISABLED_NOT.

Yes, I believe status_ must be OPEN there. I've added a DCHECK. Does that sound
workable?

https://codereview.chromium.org/1768593003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:1163: // to either fully disabled or back to
open
On 2016/03/05 00:55:18, ramant wrote:
> overly nit. "." at the end of line#1163.

Done.

[ramant (doing other things), 2016-03-05 01:02:19]

On 2016/03/05 00:59:50, Ryan Hamilton wrote:
>
https://codereview.chromium.org/1768593003/diff/1/net/quic/quic_stream_factor...
> File net/quic/quic_stream_factory.cc (right):
> 
>
https://codereview.chromium.org/1768593003/diff/1/net/quic/quic_stream_factor...
> net/quic/quic_stream_factory.cc:1156: if (disabled_reason ==
> QuicChromiumClientSession::QUIC_DISABLED_NOT)
> On 2016/03/05 00:55:18, ramant wrote:
> > nit: a small question when "disabled_reason ==
> > QuicChromiumClientSession::QUIC_DISABLED_NOT", status_ should be OPEN and 
> > IsQuicDisabled(port) should be false. Wondering if status_ being a  CLOSED
is
> a
> > valid status?. I am guessing that status_ is CLOSED should never happen.
> thanks.
> > 
> > IMO, in the worst case, if this were ever to happen, 
> > Net.QuicStreamFactory.DisabledReasons histogram will have count for 
> > QUIC_DISABLED_NOT.
> 
> Yes, I believe status_ must be OPEN there. I've added a DCHECK. Does that
sound
> workable?

:-). Perfect. Thanks much Ryan.

> 
>
https://codereview.chromium.org/1768593003/diff/1/net/quic/quic_stream_factor...
> net/quic/quic_stream_factory.cc:1163: // to either fully disabled or back to
> open
> On 2016/03/05 00:55:18, ramant wrote:
> > overly nit. "." at the end of line#1163.
> 
> Done.

[Ryan Hamilton, 2016-03-07 22:59:59]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-03-07 22:59:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org
Link to the patchset: https://codereview.chromium.org/1768593003/#ps20001
(title: "Address comments")

[commit-bot: I haz the power, 2016-03-07 23:00:31]

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

[commit-bot: I haz the power, 2016-03-08 00:37:36]

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

[commit-bot: I haz the power, 2016-03-08 00:37:37]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Hamilton, 2016-03-10 18:08:02]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2016-03-10 18:08:24]

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

[commit-bot: I haz the power, 2016-03-10 19:48:02]

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

[commit-bot: I haz the power, 2016-03-10 19:48:03]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ryan Hamilton, 2016-03-10 22:37:23]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2016-03-10 22:38:01]

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

[commit-bot: I haz the power, 2016-03-11 00:22:22]

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

[commit-bot: I haz the power, 2016-03-11 00:22:23]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Hamilton, 2016-03-11 05:24:02]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2016-03-11 05:25:03]

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

[commit-bot: I haz the power, 2016-03-11 06:16:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-11 06:18:08]

Description was changed from

==========
When QUIC is disabled because of timeouts with open streams, re-enable
QUIC if TCP jobs are also failing, since this likely indicates a
loss of network, not a problem with QUIC.
==========

to

==========
When QUIC is disabled because of timeouts with open streams, re-enable
QUIC if TCP jobs are also failing, since this likely indicates a
loss of network, not a problem with QUIC.

Committed: https://crrev.com/cee7bebda0eda4ce9316e72459e33995b4bd4f12
Cr-Commit-Position: refs/heads/master@{#380553}
==========

[commit-bot: I haz the power, 2016-03-11 06:18:09]

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

[Ryan Hamilton, 2016-03-16 18:25:17]

Description was changed from

==========
When QUIC is disabled because of timeouts with open streams, re-enable
QUIC if TCP jobs are also failing, since this likely indicates a
loss of network, not a problem with QUIC.

Committed: https://crrev.com/cee7bebda0eda4ce9316e72459e33995b4bd4f12
Cr-Commit-Position: refs/heads/master@{#380553}
==========

to

==========
When QUIC is disabled because of timeouts with open streams, re-enable
QUIC if TCP jobs are also failing, since this likely indicates a
loss of network, not a problem with QUIC.

BUG=571098

Committed: https://crrev.com/cee7bebda0eda4ce9316e72459e33995b4bd4f12
Cr-Commit-Position: refs/heads/master@{#380553}
==========