Start ICE connectivity checks as soon as the first pair is pingable.

Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.

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

[Taylor Brandstetter, 2016-06-24 23:52:19]

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

[pthatcher1, 2016-06-25 00:02:36]

lgtm

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

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:507: //
Who is going to fix this, and how soon?

[honghaiz3, 2016-06-27 16:24:20]

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:159: thread()->Post(RTC_FROM_HERE, this,
MSG_CHECK_AND_PING);
Should we worry about that the first connection may not have ice_pwd?
It may be created from a stun ping from unknown address. 
If that happened and the signaling arrived shortly after, it may effectively
delay the first stun ping request by up to 50ms.

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:160: started_pinging_ = true;
The two variables had_connection_ and started_pinging_  have identical values. 
Should we just keep one of them?

[pthatcher1, 2016-06-27 18:32:20]

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:159: thread()->Post(RTC_FROM_HERE, this,
MSG_CHECK_AND_PING);
On 2016/06/27 16:24:20, honghaiz3 wrote:
> Should we worry about that the first connection may not have ice_pwd?
> It may be created from a stun ping from unknown address. 
> If that happened and the signaling arrived shortly after, it may effectively
> delay the first stun ping request by up to 50ms. 

It's true that we need to make sure we have the remote pwd.   Perhaps a simple
(if (!connection->remote_candidate().pwd().empty())) check would do. 

But then we'll need to maybe start pinging inside of SetRemoteIceCredentials if
connections_ is not empty.  

By the way, we should probably add a test for this case: that we ping when we
get the remote pwd and a connection, but not until both.

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:160: started_pinging_ = true;
On 2016/06/27 16:24:20, honghaiz3 wrote:
> The two variables had_connection_ and started_pinging_  have identical values.

> Should we just keep one of them?

I won't any more if we check that we have the remote pwd.

[Taylor Brandstetter, 2016-06-27 22:19:34]

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:159: thread()->Post(RTC_FROM_HERE, this,
MSG_CHECK_AND_PING);
On 2016/06/27 18:32:20, pthatcher1 wrote:
> On 2016/06/27 16:24:20, honghaiz3 wrote:
> > Should we worry about that the first connection may not have ice_pwd?
> > It may be created from a stun ping from unknown address. 
> > If that happened and the signaling arrived shortly after, it may effectively
> > delay the first stun ping request by up to 50ms. 
> 
> It's true that we need to make sure we have the remote pwd.   Perhaps a simple
> (if (!connection->remote_candidate().pwd().empty())) check would do. 
> 
> But then we'll need to maybe start pinging inside of SetRemoteIceCredentials
if
> connections_ is not empty.  
> 
> By the way, we should probably add a test for this case: that we ping when we
> get the remote pwd and a connection, but not until both.

Great observation, Honghai. I was only thinking about the callee pinging as soon
as possible (which generally has ICE credentials before it has candidates) and
not the caller.

I fixed this and added a test. With the fake clock, a test for this is pretty
simple to write.

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:160: started_pinging_ = true;
On 2016/06/27 18:32:20, pthatcher1 wrote:
> On 2016/06/27 16:24:20, honghaiz3 wrote:
> > The two variables had_connection_ and started_pinging_  have identical
values.
> 
> > Should we just keep one of them?
> 
> I won't any more if we check that we have the remote pwd.

Peter is correct.

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

https://codereview.webrtc.org/2099563004/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:507: //
On 2016/06/25 00:02:36, pthatcher1 wrote:
> Who is going to fix this, and how soon?

Me, and this week if possible. I also plan to re-enable the 48 tests for NAT
combinations that are completely disabled.

[pthatcher1, 2016-06-28 01:16:33]

lgtm 

Nice unit test and fix.

https://codereview.webrtc.org/2099563004/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2099563004/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:957: if (IsPingable(c, now)) {
Could be slightly more readable with:

if (std::none_of(connections_....IsPingable...)) {
  return;
}

...
started_pinging_ = true;

[honghaiz3, 2016-06-28 01:58:41]

lgtm

[Taylor Brandstetter, 2016-06-28 02:01:42]

Honghai, you may want to review again. I'm now calling MaybeStartPinging from
SortConnections, which I renamed to SortConnectionsAndUpdateState to make it
more clear that updating the state is part of its responsibilities.

https://codereview.webrtc.org/2099563004/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2099563004/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:957: if (IsPingable(c, now)) {
On 2016/06/28 01:16:32, pthatcher1 wrote:
> Could be slightly more readable with:
> 
> if (std::none_of(connections_....IsPingable...)) {
>   return;
> }
> 
> ...
> started_pinging_ = true;

Done. Though I think:

if (any_of) {
  do_thing();
}

is more readable than:

if (none_of) {
  return;
}
do_thing();"

[Taylor Brandstetter, 2016-06-29 17:16:03]

Friendly reminder that I'm waiting for another review, since I did some
refactoring.

[pthatcher1, 2016-06-29 17:45:44]

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

https://codereview.webrtc.org/2099563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1213: // * A TCP connection became
connected.
Oh, wow, good catch about TCP.  And much cleaner.

https://codereview.webrtc.org/2099563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1621: }
Do we need to update the state even if the destroyed connection isn't the
selected connection?  Looking at ComputeState, it appears that we would miss
going to the failed/completed state when the last non-selected connection is
destroyed.

Perhaps just add "else { UpdateState(); }" ?

[honghaiz3, 2016-06-29 17:47:44]

lgtm

[Taylor Brandstetter, 2016-06-29 18:04:56]

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

https://codereview.webrtc.org/2099563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1213: // * A TCP connection became
connected.
On 2016/06/29 17:45:43, pthatcher1 wrote:
> Oh, wow, good catch about TCP.  And much cleaner.

It actually wasn't my catch; it was the unit tests doing their job (yay, unit
tests!)

https://codereview.webrtc.org/2099563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1621: }
On 2016/06/29 17:45:43, pthatcher1 wrote:
> Do we need to update the state even if the destroyed connection isn't the
> selected connection?  Looking at ComputeState, it appears that we would miss
> going to the failed/completed state when the last non-selected connection is
> destroyed.
> 
> Perhaps just add "else { UpdateState(); }" ?

You're right; I didn't mean to delete that. But it's probably a good idea to put
it in an "else", because that means if the selected connection is destroyed but
we have a backup, we'll seamlessly switch to the backup instead of emitting a
"disconnected" briefly before the connections are sorted.

[pthatcher1, 2016-06-29 18:14:19]

lgtm

(Too bad there wasn't a unit test for this failed/connected state thing, though)

[honghaiz3, 2016-06-29 18:19:32]

On 2016/06/29 18:04:56, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2099563004/diff/60001/webrtc/p2p/base/p2ptransp...
> File webrtc/p2p/base/p2ptransportchannel.cc (right):
> 
>
https://codereview.webrtc.org/2099563004/diff/60001/webrtc/p2p/base/p2ptransp...
> webrtc/p2p/base/p2ptransportchannel.cc:1213: // * A TCP connection became
> connected.
> On 2016/06/29 17:45:43, pthatcher1 wrote:
> > Oh, wow, good catch about TCP.  And much cleaner.
> 
> It actually wasn't my catch; it was the unit tests doing their job (yay, unit
> tests!)
> 
>
https://codereview.webrtc.org/2099563004/diff/60001/webrtc/p2p/base/p2ptransp...
> webrtc/p2p/base/p2ptransportchannel.cc:1621: }
> On 2016/06/29 17:45:43, pthatcher1 wrote:
> > Do we need to update the state even if the destroyed connection isn't the
> > selected connection?  Looking at ComputeState, it appears that we would miss
> > going to the failed/completed state when the last non-selected connection is
> > destroyed.
> > 
> > Perhaps just add "else { UpdateState(); }" ?
> 
> You're right; I didn't mean to delete that. But it's probably a good idea to
put
> it in an "else", because that means if the selected connection is destroyed
but
> we have a backup, we'll seamlessly switch to the backup instead of emitting a
> "disconnected" briefly before the connections are sorted.

By the way, please update the CL description.

[Taylor Brandstetter, 2016-06-29 18:20:39]

Description was changed from

==========
Start ICE connectivity checks as soon as the first pair is made.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection is made.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.
==========

to

==========
Start ICE connectivity checks as soon as the first pair is made.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.
==========

[Taylor Brandstetter, 2016-06-29 18:28:13]

Description was changed from

==========
Start ICE connectivity checks as soon as the first pair is made.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.
==========

to

==========
Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.
==========

[commit-bot: I haz the power, 2016-06-29 20:07:32]

Description was changed from

==========
Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.
==========

to

==========
Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.

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

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

[Taylor Brandstetter, 2016-06-29 20:07:32]

Description was changed from

==========
Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.
==========

to

==========
Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.

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

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

[commit-bot: I haz the power, 2016-06-29 20:07:33]

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

[Taylor Brandstetter, 2016-06-29 20:07:34]

Committed patchset #5 (id:80001) manually as
b825aee04a4a0a0ff302139185122f1490dcd542 (presubmit successful).

[Taylor Brandstetter, 2016-06-29 21:13:03]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/2105413002/ by deadbeef@webrtc.org.

The reason for reverting is: Reverting because Chromoting depended on the
Connect method. Will reland with the Connect method as a no-op and a TODO to
remove..

[Taylor Brandstetter, 2016-06-29 21:14:09]

Description was changed from

==========
Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.

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

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

to

==========
Relanding: Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.

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

[Taylor Brandstetter, 2016-06-29 22:16:12]

Description was changed from

==========
Relanding: Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.

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

to

==========
Start ICE connectivity checks as soon as the first pair is pingable.

Previously, we were starting a periodic timer when the local
description was set. The first connection may be created at any
time after this happens, so after creating the first connection, we
need to wait until that timer next fires before sending a ping.

Now we just start that timer (and send the first ping) immediately
after the first connection becomes pingable.

This CL also removes the "Connect" method. The only vestigal
effect of this method was to start the periodic timer, which is
now not needed since it happens automatically.

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