Do not switch to a high-cost connection that is not receiving

Do not switch to a high-cost connection that is not receiving.

This prevents connection switching due to remote-side network down.

R=deadbeef@webrtc.org, pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/fd16da290ce90cb0810890a8b7ffcee98cdb5c3c

[honghaiz3, 2016-08-10 05:13:40]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-08-10 05:13:48]

Patchset #2 (id:40001) has been deleted

[honghaiz3, 2016-08-10 05:15:00]

Description was changed from

==========
Do not switch to a high-cost but not receiving connection.
==========

to

==========
Do not switch to a high-cost connection that is not receiving.

This prevents connection switching due to remote-side network down.
==========

[honghaiz3, 2016-08-10 05:15:26]

honghaiz@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, pthatcher@webrtc.org

[pthatcher1, 2016-08-10 17:25:50]

lgtm

[Taylor Brandstetter, 2016-08-11 22:54:40]

lgtm, with a minor question

https://codereview.webrtc.org/2232563002/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2232563002/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2193:
clock.AdvanceTime(rtc::TimeDelta::FromSeconds(100u));
Can you explain why this is necessary? Can IsPingable be fixed?

[honghaiz3, 2016-08-12 17:04:26]

https://codereview.webrtc.org/2232563002/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2232563002/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2193:
clock.AdvanceTime(rtc::TimeDelta::FromSeconds(100u));
On 2016/08/11 22:54:40, Taylor Brandstetter wrote:
> Can you explain why this is necessary? Can IsPingable be fixed?
A backup connection is pinged at a slower rate (which is configurable). This is
controlled by checking the difference between now and the last time ping
response is received. When no ping response is received, the latter is 0. If the
clock time starts from 0, it may take a long time for the backup connection to
get the first chance to be pinged. In this test, we need to make sure the backup
connection become writable first.

It never has this issue with real clock. 

I think it may be good to initialize the FakeClock with the current reading of
the wall clock when it is created. That would have avoided several test issues
we have incurred and that will simulate more practical settings.

[honghaiz3, 2016-08-12 19:29:14]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2016-08-12 19:29:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-12 19:36:44]

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

[commit-bot: I haz the power, 2016-08-12 19:36:46]

Try jobs failed on following builders:
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/9513)

[honghaiz3, 2016-08-12 23:41:35]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-08-12 23:41:35]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2232563002/#ps80001 (title:
"Merge with head")

[commit-bot: I haz the power, 2016-08-12 23:41:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-12 23:45:30]

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

[commit-bot: I haz the power, 2016-08-12 23:45:31]

Try jobs failed on following builders:
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/9515)

[Taylor Brandstetter, 2016-08-15 17:37:32]

https://codereview.webrtc.org/2232563002/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2232563002/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2193:
clock.AdvanceTime(rtc::TimeDelta::FromSeconds(100u));
On 2016/08/12 17:04:26, honghaiz3 wrote:
> On 2016/08/11 22:54:40, Taylor Brandstetter wrote:
> > Can you explain why this is necessary? Can IsPingable be fixed?
> A backup connection is pinged at a slower rate (which is configurable). This
is
> controlled by checking the difference between now and the last time ping
> response is received. When no ping response is received, the latter is 0. If
the
> clock time starts from 0, it may take a long time for the backup connection to
> get the first chance to be pinged. In this test, we need to make sure the
backup
> connection become writable first.
> 
> It never has this issue with real clock. 
> 
> I think it may be good to initialize the FakeClock with the current reading of
> the wall clock when it is created. That would have avoided several test issues
> we have incurred and that will simulate more practical settings.  

So then the issue is that 0 is treated as a special value meaning "no ping
responses received"? I know this is a corner case issue, but can the logic be
changed so that "no ping responses received" is detected in some other way?

Also, I disagree about initializing the fake clock with a wall clock time. The
fake clock should allow writing fully repeatable tests, so it should use a
repeatable initial time. And if we did want to do time "fuzzing" for a specific
test, it would be better to use fully random values rather than the wall clock
time. The wall-clock time on the VMs we run the tests on could be biased. I
believe this has caused at least one bug to go under the radar in the past; a
64-bit timestamp was being cast to a 32-bit timestamp but the bot didn't catch
this because the timestamp always ended up in a 32-bit value range.

[honghaiz3, 2016-08-16 00:42:41]

Patchset #4 (id:100001) has been deleted

[honghaiz3, 2016-08-16 00:43:07]

https://codereview.webrtc.org/2232563002/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2232563002/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2193:
clock.AdvanceTime(rtc::TimeDelta::FromSeconds(100u));
On 2016/08/15 17:37:32, Taylor Brandstetter wrote:
> On 2016/08/12 17:04:26, honghaiz3 wrote:
> > On 2016/08/11 22:54:40, Taylor Brandstetter wrote:
> > > Can you explain why this is necessary? Can IsPingable be fixed?
> > A backup connection is pinged at a slower rate (which is configurable). This
> is
> > controlled by checking the difference between now and the last time ping
> > response is received. When no ping response is received, the latter is 0. If
> the
> > clock time starts from 0, it may take a long time for the backup connection
to
> > get the first chance to be pinged. In this test, we need to make sure the
> backup
> > connection become writable first.
> > 
> > It never has this issue with real clock. 
> > 
> > I think it may be good to initialize the FakeClock with the current reading
of
> > the wall clock when it is created. That would have avoided several test
issues
> > we have incurred and that will simulate more practical settings.  
> 
> So then the issue is that 0 is treated as a special value meaning "no ping
> responses received"? I know this is a corner case issue, but can the logic be
> changed so that "no ping responses received" is detected in some other way?
Added checking on the "no ping response is received using the rtt_samples(). 

> 
> Also, I disagree about initializing the fake clock with a wall clock time. The
> fake clock should allow writing fully repeatable tests, so it should use a
> repeatable initial time. And if we did want to do time "fuzzing" for a
specific
> test, it would be better to use fully random values rather than the wall clock
> time. The wall-clock time on the VMs we run the tests on could be biased. I
> believe this has caused at least one bug to go under the radar in the past; a
> 64-bit timestamp was being cast to a 32-bit timestamp but the bot didn't catch
> this because the timestamp always ended up in a 32-bit value range.

If a 32-bit initial timestamp may be an issue, using 0 as the default initial
timestamp may be a bigger issue. Using the random value might be better than the
wall clock time in the case you described, but we need to choose the range and
distribution reasonably. I am not sure if it is worth the complexity. If using
the call-clock is an issue, it will be an issue with a non-fake clock anyway
because the latter is what we use for all non-fake clocks. So it will be as good
as the real clock.  

Probably we want to allow the test code to initialize the fake clock with a
chosen value.

[honghaiz3, 2016-08-17 20:36:10]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-08-17 20:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2232563002/#ps120001 (title:
".")

[commit-bot: I haz the power, 2016-08-17 20:36:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-17 22:49:36]

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

[commit-bot: I haz the power, 2016-08-17 22:49:37]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[honghaiz3, 2016-08-17 23:13:02]

Description was changed from

==========
Do not switch to a high-cost connection that is not receiving.

This prevents connection switching due to remote-side network down.
==========

to

==========
Do not switch to a high-cost connection that is not receiving.

This prevents connection switching due to remote-side network down.

R=deadbeef@webrtc.org, pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/fd16da290ce90cb0810890a8b...
==========

[commit-bot: I haz the power, 2016-08-17 23:13:02]

Description was changed from

==========
Do not switch to a high-cost connection that is not receiving.

This prevents connection switching due to remote-side network down.
==========

to

==========
Do not switch to a high-cost connection that is not receiving.

This prevents connection switching due to remote-side network down.

R=deadbeef@webrtc.org, pthatcher@webrtc.org

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

[honghaiz3, 2016-08-17 23:13:03]

Committed patchset #4 (id:120001) manually as
fd16da290ce90cb0810890a8b7ffcee98cdb5c3c (presubmit successful).

[commit-bot: I haz the power, 2016-08-17 23:13:03]

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