Create a new connection if a candidate reuses an address

Create a new connection if a candidate reuses an address

If the remote side sends a candidate with the same address and port with an existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from it
and destroy the old connection with the same remote address.

BUG=webrtc:5915
Committed: https://crrev.com/36f50e8e4e3e947ad8db1179e358b11431a8f079
Cr-Commit-Position: refs/heads/master@{#13000}

[honghaiz3, 2016-05-26 22:53:17]

Description was changed from

==========
Create a new connection if a candidate has a dup address

If the remote side sends a candidate with the same address and port
but a new ufrag and pwd, we will create a new connection from it
and destroy the old connection with the same remote address.

BUG=
==========

to

==========
Create a new connection if a candidate reuses an address

If the remote side sends a candidate with the same address and port with an
existing candidate,
but with a new ufrag and pwd, we will create a new connection from it
and destroy the old connection with the same remote address.

BUG=webrtc:5915
==========

[honghaiz3, 2016-05-26 22:54:34]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-05-26 22:55:17]

Description was changed from

==========
Create a new connection if a candidate reuses an address

If the remote side sends a candidate with the same address and port with an
existing candidate,
but with a new ufrag and pwd, we will create a new connection from it
and destroy the old connection with the same remote address.

BUG=webrtc:5915
==========

to

==========
Create a new connection if a candidate reuses an address

If the remote side sends an candidate with the same address and port with an
existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from
it
and destroy the old connection with the same remote address.

BUG=webrtc:5915
==========

[honghaiz3, 2016-05-27 17:17:41]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2016-05-27 17:17:49]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2016-05-27 17:18:15]

Description was changed from

==========
Create a new connection if a candidate reuses an address

If the remote side sends an candidate with the same address and port with an
existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from
it
and destroy the old connection with the same remote address.

BUG=webrtc:5915
==========

to

==========
Create a new connection if a candidate reuses an address

If the remote side sends a candidate with the same address and port with an
existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from
it
and destroy the old connection with the same remote address.

BUG=webrtc:5915
==========

[honghaiz3, 2016-05-27 17:23:26]

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

[honghaiz3, 2016-05-27 17:23:27]

PTAL.

[pthatcher1, 2016-05-27 18:42:54]

Thanks for making a change so quickly.

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

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:885: PortInterface* origin_port) {
I don't quite understand why you split these methods into two, since both of
them don't create a connection under certain circumstances.  So how do you
decide to put the logic in one vs. the other?

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:888: // Don't create connection if this
is a candidate we received in a
create connection => create a caonnection

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:890: if (origin ==
PortInterface::ORIGIN_MESSAGE && incoming_only_)
{}s please

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:225: PortInterface* origin_port);
Can you leave a comment explaining when we create one and when we don't (the
"maybe" part)?

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

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2487: }
I think we also need a test that if we receive a STUN ping with the new
ufrag/pwd, we will process it correctly.

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:273: void Port::AddConnection(Connection* conn) {
If we're going to destroy existing connections when we add ones, we should
rename this AddOrReplaceConnection, and comment that the decision of whether to
add or replace is based on remote address.

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:283: ret.first->second = conn;
Instead of putting logic in OnConnectionDestroyed to check if it's the
connection we're looking for, why not just

ret.first->SignalDestroyed.disconnect(this); 

Before destroying it?

[honghaiz3, 2016-05-31 18:42:49]

Patchset #3 (id:100001) has been deleted

[honghaiz3, 2016-05-31 18:57:32]

PTAL.

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

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:885: PortInterface* origin_port) {
On 2016/05/27 18:42:54, pthatcher1 wrote:
> I don't quite understand why you split these methods into two, since both of
> them don't create a connection under certain circumstances.  So how do you
> decide to put the logic in one vs. the other?

I thought it may improve readability.
Since it caused confusion, I just put them together in one method.

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:888: // Don't create connection if this
is a candidate we received in a
On 2016/05/27 18:42:54, pthatcher1 wrote:
> create connection => create a caonnection

Done.

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:890: if (origin ==
PortInterface::ORIGIN_MESSAGE && incoming_only_)
On 2016/05/27 18:42:54, pthatcher1 wrote:
> {}s please

Done.

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:225: PortInterface* origin_port);
On 2016/05/27 18:42:54, pthatcher1 wrote:
> Can you leave a comment explaining when we create one and when we don't (the
> "maybe" part)?

Removed the maybe method.

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

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2487: }
On 2016/05/27 18:42:54, pthatcher1 wrote:
> I think we also need a test that if we receive a STUN ping with the new
> ufrag/pwd, we will process it correctly.
I tried to do it but found that it is kind of unnecessary and it would be
complicated to do so. 
The ping test essentially checks 
1) Whether GetConnectionTo() finds the right connection, 
2) Whether the ufrag in the remote candidate matches the one in the stun ping
request. 
Both of them have already been tested here. 
The operation of combining these two are actually in each subclass of Port. 
Trying to test the combined operation needs to expose methods in the subclasses
and some extra hackings, which may not be really worth it. WDYT?

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:273: void Port::AddConnection(Connection* conn) {
On 2016/05/27 18:42:54, pthatcher1 wrote:
> If we're going to destroy existing connections when we add ones, we should
> rename this AddOrReplaceConnection, and comment that the decision of whether
to
> add or replace is based on remote address.

Done.

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:283: ret.first->second = conn;
On 2016/05/27 18:42:54, pthatcher1 wrote:
> Instead of putting logic in OnConnectionDestroyed to check if it's the
> connection we're looking for, why not just
> 
> ret.first->SignalDestroyed.disconnect(this); 
> 
> Before destroying it?
That would also do. 
But in the OnConnectionDestroyed method, deleting a connection without checking
whether the connection in the map is actually the one destroyed has a potential
risk. It is perhaps slightly safer to check it there.
I could also disconnect here and also do a check there although one of them is
really not necessary. 
WDYT?

[pthatcher1, 2016-05-31 20:09:30]

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:283: ret.first->second = conn;
On 2016/05/31 18:57:31, honghaiz3 wrote:
> On 2016/05/27 18:42:54, pthatcher1 wrote:
> > Instead of putting logic in OnConnectionDestroyed to check if it's the
> > connection we're looking for, why not just
> > 
> > ret.first->SignalDestroyed.disconnect(this); 
> > 
> > Before destroying it?
> That would also do. 
> But in the OnConnectionDestroyed method, deleting a connection without
checking
> whether the connection in the map is actually the one destroyed has a
potential
> risk. It is perhaps slightly safer to check it there.
> I could also disconnect here and also do a check there although one of them is
> really not necessary. 
> WDYT?

I think the check is overkill.  We don't have the check now, and it's been fine.
 I think the disconnect from the signal is all we need.

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:874: AddConnection(connection);
I think this should also be AddOrReplaceConnectionk, since that's what it does.

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2486: EXPECT_EQ(1u,
conn2->remote_candidate().generation());
Can't we right here inject a STUN ping into the connection and verify that the
last ping received was that one?

[honghaiz3, 2016-05-31 23:09:25]

Patchset #4 (id:140001) has been deleted

[honghaiz3, 2016-05-31 23:25:23]

PTAL.

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:874: AddConnection(connection);
On 2016/05/31 20:09:29, pthatcher1 wrote:
> I think this should also be AddOrReplaceConnectionk, since that's what it
does.
It might look strange to use AddOrRepalceConnection here if you look at the
implementation of the method. We are not replacing anything directly in the
method. It will get replaced simply because the old connection (on the same
remote address) was destroyed and removed from the list. 
WDYT?

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2486: EXPECT_EQ(1u,
conn2->remote_candidate().generation());
On 2016/05/31 20:09:29, pthatcher1 wrote:
> Can't we right here inject a STUN ping into the connection and verify that the
> last ping received was that one?

Added per your suggestion, although I still feel it is a little strange to
include that here. 
All it (the faked ping message) does is to construct a Ping message using the
info from the connection except for the remote ufrag. But since we have already
checked that the remote ufrag is the same as the one in the connection remote
candidate, it is actually using the info from the connection completely to check
whether the connection can handle a ping request.
Perhaps, it can be tested independently that a connection can handle a ping
request properly. 
But that kind of tests are already there in port_unittest file.

[pthatcher1, 2016-06-01 01:57:04]

lgtm

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:874: AddConnection(connection);
On 2016/05/31 23:25:23, honghaiz3 wrote:
> On 2016/05/31 20:09:29, pthatcher1 wrote:
> > I think this should also be AddOrReplaceConnectionk, since that's what it
> does.
> It might look strange to use AddOrRepalceConnection here if you look at the
> implementation of the method. We are not replacing anything directly in the
> method. It will get replaced simply because the old connection (on the same
> remote address) was destroyed and removed from the list. 
> WDYT?

Ah, you're completely right.  Please keep it AddConnection.

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2486: EXPECT_EQ(1u,
conn2->remote_candidate().generation());
On 2016/05/31 23:25:23, honghaiz3 wrote:
> On 2016/05/31 20:09:29, pthatcher1 wrote:
> > Can't we right here inject a STUN ping into the connection and verify that
the
> > last ping received was that one?
> 
> Added per your suggestion, although I still feel it is a little strange to
> include that here. 
> All it (the faked ping message) does is to construct a Ping message using the
> info from the connection except for the remote ufrag. But since we have
already
> checked that the remote ufrag is the same as the one in the connection remote
> candidate, it is actually using the info from the connection completely to
check
> whether the connection can handle a ping request.
> Perhaps, it can be tested independently that a connection can handle a ping
> request properly. 
> But that kind of tests are already there in port_unittest file. 



I agree it's not optimal, but I still think it's better than nothing.

https://codereview.webrtc.org/2018693002/diff/160001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2018693002/diff/160001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1987: void
ReceivedPingOnConnection(cricket::Connection* conn,
Should call this ReceivePingOnConnection or maybe just ReceivePing

[honghaiz3, 2016-06-01 16:37:59]

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:283: ret.first->second = conn;
On 2016/05/31 20:09:29, pthatcher1 wrote:
> On 2016/05/31 18:57:31, honghaiz3 wrote:
> > On 2016/05/27 18:42:54, pthatcher1 wrote:
> > > Instead of putting logic in OnConnectionDestroyed to check if it's the
> > > connection we're looking for, why not just
> > > 
> > > ret.first->SignalDestroyed.disconnect(this); 
> > > 
> > > Before destroying it?
> > That would also do. 
> > But in the OnConnectionDestroyed method, deleting a connection without
> checking
> > whether the connection in the map is actually the one destroyed has a
> potential
> > risk. It is perhaps slightly safer to check it there.
> > I could also disconnect here and also do a check there although one of them
is
> > really not necessary. 
> > WDYT?
> 
> I think the check is overkill.  We don't have the check now, and it's been
fine.
>  I think the disconnect from the signal is all we need.

Done. You are right. We only hit the replace-connection very rarely.

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:874: AddConnection(connection);
On 2016/06/01 01:57:04, pthatcher1 wrote:
> On 2016/05/31 23:25:23, honghaiz3 wrote:
> > On 2016/05/31 20:09:29, pthatcher1 wrote:
> > > I think this should also be AddOrReplaceConnectionk, since that's what it
> > does.
> > It might look strange to use AddOrRepalceConnection here if you look at the
> > implementation of the method. We are not replacing anything directly in the
> > method. It will get replaced simply because the old connection (on the same
> > remote address) was destroyed and removed from the list. 
> > WDYT?
> 
> Ah, you're completely right.  Please keep it AddConnection.

Acknowledged.

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:2486: EXPECT_EQ(1u,
conn2->remote_candidate().generation());
On 2016/06/01 01:57:04, pthatcher1 wrote:
> On 2016/05/31 23:25:23, honghaiz3 wrote:
> > On 2016/05/31 20:09:29, pthatcher1 wrote:
> > > Can't we right here inject a STUN ping into the connection and verify that
> the
> > > last ping received was that one?
> > 
> > Added per your suggestion, although I still feel it is a little strange to
> > include that here. 
> > All it (the faked ping message) does is to construct a Ping message using
the
> > info from the connection except for the remote ufrag. But since we have
> already
> > checked that the remote ufrag is the same as the one in the connection
remote
> > candidate, it is actually using the info from the connection completely to
> check
> > whether the connection can handle a ping request.
> > Perhaps, it can be tested independently that a connection can handle a ping
> > request properly. 
> > But that kind of tests are already there in port_unittest file. 
> 
> 
> 
> I agree it's not optimal, but I still think it's better than nothing.  

Acknowledged.

https://codereview.webrtc.org/2018693002/diff/160001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2018693002/diff/160001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1987: void
ReceivedPingOnConnection(cricket::Connection* conn,
On 2016/06/01 01:57:04, pthatcher1 wrote:
> Should call this ReceivePingOnConnection or maybe just ReceivePing

Done.

[honghaiz3, 2016-06-01 16:38:40]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-06-01 16:38:40]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2018693002/#ps180001 (title:
"address comments")

[commit-bot: I haz the power, 2016-06-01 16:38:51]

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

[commit-bot: I haz the power, 2016-06-01 18:39:36]

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

[commit-bot: I haz the power, 2016-06-01 18:39:37]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[honghaiz3, 2016-06-01 22:55:19]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2016-06-01 22:55:27]

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

[commit-bot: I haz the power, 2016-06-01 22:57:06]

Description was changed from

==========
Create a new connection if a candidate reuses an address

If the remote side sends a candidate with the same address and port with an
existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from
it
and destroy the old connection with the same remote address.

BUG=webrtc:5915
==========

to

==========
Create a new connection if a candidate reuses an address

If the remote side sends a candidate with the same address and port with an
existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from
it
and destroy the old connection with the same remote address.

BUG=webrtc:5915
==========

[commit-bot: I haz the power, 2016-06-01 22:57:07]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2016-06-01 22:57:22]

Description was changed from

==========
Create a new connection if a candidate reuses an address

If the remote side sends a candidate with the same address and port with an
existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from
it
and destroy the old connection with the same remote address.

BUG=webrtc:5915
==========

to

==========
Create a new connection if a candidate reuses an address

If the remote side sends a candidate with the same address and port with an
existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from
it
and destroy the old connection with the same remote address.

BUG=webrtc:5915

Committed: https://crrev.com/36f50e8e4e3e947ad8db1179e358b11431a8f079
Cr-Commit-Position: refs/heads/master@{#13000}
==========

[commit-bot: I haz the power, 2016-06-01 22:57:24]

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