Connection resurrected with incorrect candidate type.

Connection resurrected with incorrect candidate type.

Connection can be resurrected with current code when there is no any existing connection for the same address. However, it's always resurrected with prflx candidate priority hence the new connection could bump down other better connection.

Migrated from https://webrtc-codereview.appspot.com/51959004/

This is based on test cases added for triggered checks.

BUG=webrtc:4724
R=pthatcher@webrtc.org

Committed: https://crrev.com/372f2fcc59829f9eaa2b10519bebd0f71e915f93
Cr-Commit-Position: refs/heads/master@{#9429}

[guoweis_webrtc, 2015-06-08 23:52:46]

Patchset #2 (id:20001) has been deleted

[guoweis_webrtc, 2015-06-08 23:52:51]

Patchset #1 (id:1) has been deleted

[guoweis_webrtc, 2015-06-08 23:56:29]

guoweis@webrtc.org changed reviewers:
+ juberti@webrtc.org, pthatcher@webrtc.org

[guoweis_webrtc, 2015-06-08 23:56:30]

PTAL.

[pthatcher1, 2015-06-09 23:58:03]

Just style/nits.

By the way, are you able to run and test this manually somehow?  I can't think
of a good way without making a custom ICE client that interacts with Chrome. 
Can you?

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

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:482: bool remote_candidate_is_new =
false;
Perhaps more clear as:

bool remote_candidate_is_new = (candidate == nullptr);
if (candidate_is_new) {
  ...
} else {
  ..
}

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:541: }
Can you move this up into the rest of the "remote_candidate_is_new" code?  That
way we'll have one set_priority instead of two:

if (port->IceProtocol() == ICEPROTO_RFC5245) {
  ...
  remote_candidate.set_priority(priority_attr->value())
} else {
  remote_candidate.set_priority(remote_candidate.GetPriority(
    ICE_TYPE_PREFERENCE_PRFLX, port->Network()->preference(), 0));
}

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

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1724: class
P2PTransportChannelSingleEndpointTest : public testing::Test,
Let's just call it P2PTransportChannelTest

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1888: cricket::Port* port =
static_cast<cricket::Port*>(ch.ports()[0]);
Can you make this its own function?

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1896:
(conn1->remote_candidate().priority() & 0x00FFFFFF);
Can you make this its own method?

[guoweis_webrtc, 2015-06-10 18:06:15]

Patchset #2 (id:60001) has been deleted

[guoweis_webrtc, 2015-06-10 18:10:18]

Patchset #2 (id:80001) has been deleted

[guoweis_webrtc, 2015-06-10 18:12:41]

PTAL.

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

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:482: bool remote_candidate_is_new =
false;
On 2015/06/09 23:58:03, pthatcher1 wrote:
> Perhaps more clear as:
> 
> bool remote_candidate_is_new = (candidate == nullptr);
> if (candidate_is_new) {
>   ...
> } else {
>   ..
> }

Done.

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:541: }
On 2015/06/09 23:58:03, pthatcher1 wrote:
> Can you move this up into the rest of the "remote_candidate_is_new" code? 
That
> way we'll have one set_priority instead of two:
> 
> if (port->IceProtocol() == ICEPROTO_RFC5245) {
>   ...
>   remote_candidate.set_priority(priority_attr->value())
> } else {
>   remote_candidate.set_priority(remote_candidate.GetPriority(
>     ICE_TYPE_PREFERENCE_PRFLX, port->Network()->preference(), 0));
> }
> 

Done.

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

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1724: class
P2PTransportChannelSingleEndpointTest : public testing::Test,
On 2015/06/09 23:58:03, pthatcher1 wrote:
> Let's just call it P2PTransportChannelTest

P2PTransportChannelTest exists already.

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1888: cricket::Port* port =
static_cast<cricket::Port*>(ch.ports()[0]);
On 2015/06/09 23:58:03, pthatcher1 wrote:
> Can you make this its own function?

Done.

https://codereview.webrtc.org/1172483002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1896:
(conn1->remote_candidate().priority() & 0x00FFFFFF);
On 2015/06/09 23:58:03, pthatcher1 wrote:
> Can you make this its own method?

Done.

[pthatcher1, 2015-06-10 19:43:04]

lgtm

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

https://codereview.webrtc.org/1172483002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:483: if (candidate != NULL) {
Could you change this to "if(!remote_candidate_is_new)" ?

[guoweis_webrtc, 2015-06-10 19:53:21]

Patchset #4 (id:140001) has been deleted

[guoweis_webrtc, 2015-06-12 06:05:19]

Patchset #5 (id:180001) has been deleted

[guoweis_webrtc, 2015-06-12 06:05:36]

Patchset #5 (id:200001) has been deleted

[guoweis_webrtc, 2015-06-12 17:12:59]

Committed patchset #6 (id:240001) manually as
372f2fcc59829f9eaa2b10519bebd0f71e915f93 (presubmit successful).

[commit-bot: I haz the power, 2015-06-12 17:13:04]

Patchset 6 (id:??) landed as
https://crrev.com/372f2fcc59829f9eaa2b10519bebd0f71e915f93
Cr-Commit-Position: refs/heads/master@{#9429}