Ensuring that UDP TURN servers are always used as STUN servers.

Ensuring that UDP TURN servers are always used as STUN servers.

This was already working in most cases, but not for some corner cases:
* If the PORTALLOCATOR_ENABLE_SHARED_SOCKET flag is not set
* If both a STUN server and TURN server are configured

I added unit tests for these cases, and centralized the code that gets
STUN server addresses in order to fix these and any related issues.

BUG=webrtc:4215
Committed: https://crrev.com/c5d0d95fd8957a7a6645b1196e5f1e9cee33525c
Cr-Commit-Position: refs/heads/master@{#9596}

[juberti1, 2015-06-26 18:50:33]

juberti@webrtc.org changed reviewers:
+ juberti@webrtc.org

[juberti1, 2015-06-26 18:50:34]

I think this doesn't solve the core problem, which is that the code at
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
tries to do the same logic, and gets it wrong.

[Taylor Brandstetter, 2015-06-26 19:37:35]

On 2015/06/26 18:50:34, juberti1 wrote:
> I think this doesn't solve the core problem, which is that the code at
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> tries to do the same logic, and gets it wrong.

You're right. I was planning on changing that code after this webrtc change is
rolled into chromium.

I just didn't want to change peer_connection_dependency_factory.cc without
making this change, because that would introduce a slight change in behavior: if
you configured both a STUN and TURN server, the TURN server would NOT be used as
a STUN server. I assume that's undesired behavior.

[Taylor Brandstetter, 2015-06-26 20:19:38]

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

[juberti1, 2015-06-27 04:39:53]

juberti@webrtc.org changed reviewers:
+ juberti@webrtc.org

[juberti1, 2015-06-27 04:39:54]

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
webrtc/p2p/client/portallocator_unittest.cc:953: // Don't bother waiting for
STUN timeout, since we already verified
I don't understand this. Don't we want to get the response from the STUN server,
to ensure it's not a different srflx candidate that we might want to use?

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
webrtc/p2p/client/portallocator_unittest.cc:959: // expect two ports, a UDPPort
and TurnPort.
You'll only have two ports anytime you use shared socket. 

In order to test this properly, I think you need to have a NAT, and verify that
you don't get a srflx candidate.

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
webrtc/p2p/client/portallocator_unittest.cc:960: TEST_F(PortAllocatorTest,
TestNonSharedSocketWithoutNatUsingTurnTcp) {
The socket is shared, so this name is confusing

[Taylor Brandstetter, 2015-06-27 17:05:50]

As per Justin's commit, I plan to try making a test with two layers of NAT when
I'm in the office on Monday.

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
webrtc/p2p/client/portallocator_unittest.cc:953: // Don't bother waiting for
STUN timeout, since we already verified
On 2015/06/27 04:39:54, juberti1 wrote:
> I don't understand this. Don't we want to get the response from the STUN
server,
> to ensure it's not a different srflx candidate that we might want to use?

In this case the main thing I wanted to test was that if a STUN server isn't
available (see stun_server_.reset()), we still get a srflx candidate from the
TURN server. This wasn't working before.

But you're right that testing getting two different srflx candidates would be
even more valuable. I'll see if I can figure out how to configure two layers of
NAT and put the STUN/TURN servers at different levels.

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
webrtc/p2p/client/portallocator_unittest.cc:959: // expect two ports, a UDPPort
and TurnPort.
On 2015/06/27 04:39:54, juberti1 wrote:
> You'll only have two ports anytime you use shared socket. 
> 
> In order to test this properly, I think you need to have a NAT, and verify
that
> you don't get a srflx candidate.

That's why I had intended to use non-shared socket mode, since it *would* result
in a separate UDPPort and StunPort. I verified that after doing that (Patch set
2), the test fails if I remove the TCP filter in basicportallocator.cc.

Also: I couldn't do this test with a NAT because the NATServer doesn't work with
TCP (TCP connects are dropped).

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
webrtc/p2p/client/portallocator_unittest.cc:960: TEST_F(PortAllocatorTest,
TestNonSharedSocketWithoutNatUsingTurnTcp) {
On 2015/06/27 04:39:54, juberti1 wrote:
> The socket is shared, so this name is confusing

As mentioned above, fixed in patch set 2.

[juberti1, 2015-06-29 18:40:00]

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
webrtc/p2p/client/portallocator_unittest.cc:953: // Don't bother waiting for
STUN timeout, since we already verified
On 2015/06/27 at 17:05:49, Taylor Brandstetter wrote:
> On 2015/06/27 04:39:54, juberti1 wrote:
> > I don't understand this. Don't we want to get the response from the STUN
server,
> > to ensure it's not a different srflx candidate that we might want to use?
> 
> In this case the main thing I wanted to test was that if a STUN server isn't
available (see stun_server_.reset()), we still get a srflx candidate from the
TURN server. This wasn't working before.
> 
> But you're right that testing getting two different srflx candidates would be
even more valuable. I'll see if I can figure out how to configure two layers of
NAT and put the STUN/TURN servers at different levels.

OK, I see what you are saying. I think the 2-level NAT might be overkill in that
case.

The main thing that I am troubled by is the 3537 comment and the need for a
specific step delay; that seems like it is testing implementation details.

[juberti1, 2015-06-29 18:54:37]

>
https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
> webrtc/p2p/client/portallocator_unittest.cc:959: // expect two ports, a
UDPPort and TurnPort.
> On 2015/06/27 04:39:54, juberti1 wrote:
> > You'll only have two ports anytime you use shared socket. 
> > 
> > In order to test this properly, I think you need to have a NAT, and verify
that
> > you don't get a srflx candidate.
> 
> That's why I had intended to use non-shared socket mode, since it *would*
result in a separate UDPPort and StunPort. I verified that after doing that
(Patch set 2), the test fails if I remove the TCP filter in
basicportallocator.cc.
> 
> Also: I couldn't do this test with a NAT because the NATServer doesn't work
with TCP (TCP connects are dropped).
> 

Support for non-shared socket is going away, and any important behavior should
be tested using shared socket (i.e. this behavior). 

I see the problem regarding NAT, but you'll need to find some clever way to work
around that - perhaps by implementing the TCP behavior for NATServer :)

[juberti1, 2015-06-29 18:54:49]

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:1167: for (ServerAddresses::iterator
turn_server = turn_servers.begin();
I think this could be done as 

for (auto turn_server : turn_servers)

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/portall...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:1017:
EXPECT_NE(candidates_[2].related_address(), candidates_[1].address());
But candidates_[2] is not server reflexive, so this comment is confusing.

I think this test should come right after the previous TurnAsStun tests, since
it is most similar to them.

[Taylor Brandstetter, 2015-06-29 20:18:35]

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
webrtc/p2p/client/portallocator_unittest.cc:953: // Don't bother waiting for
STUN timeout, since we already verified
On 2015/06/29 18:40:00, juberti1 wrote:
> On 2015/06/27 at 17:05:49, Taylor Brandstetter wrote:
> > On 2015/06/27 04:39:54, juberti1 wrote:
> > > I don't understand this. Don't we want to get the response from the STUN
> server,
> > > to ensure it's not a different srflx candidate that we might want to use?
> > 
> > In this case the main thing I wanted to test was that if a STUN server isn't
> available (see stun_server_.reset()), we still get a srflx candidate from the
> TURN server. This wasn't working before.
> > 
> > But you're right that testing getting two different srflx candidates would
be
> even more valuable. I'll see if I can figure out how to configure two layers
of
> NAT and put the STUN/TURN servers at different levels.
> 
> OK, I see what you are saying. I think the 2-level NAT might be overkill in
that
> case.
> 
> The main thing that I am troubled by is the 3537 comment and the need for a
> specific step delay; that seems like it is testing implementation details.

The 3537 comment and step delay aren't really needed for this test so I'll
remove them.

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:1167: for (ServerAddresses::iterator
turn_server = turn_servers.begin();
On 2015/06/29 18:54:48, juberti1 wrote:
> I think this could be done as 
> 
> for (auto turn_server : turn_servers)

Good point. In this case I think I'll use "for (rtc::SocketAddress turn_server :
turn_servers)", since "auto" doesn't really improve readability.

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/portall...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:1017:
EXPECT_NE(candidates_[2].related_address(), candidates_[1].address());
On 2015/06/29 18:54:49, juberti1 wrote:
> But candidates_[2] is not server reflexive, so this comment is confusing.
> 
> I think this test should come right after the previous TurnAsStun tests, since
> it is most similar to them.

candidates_[2] isn't server reflexive, but its related_address is; see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Also, I moved the test as you suggested.

[juberti1, 2015-06-29 20:25:02]

On 2015/06/29 at 20:18:35, deadbeef wrote:
>
https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
> File webrtc/p2p/client/portallocator_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1215713003/diff/1/webrtc/p2p/client/portallocat...
> webrtc/p2p/client/portallocator_unittest.cc:953: // Don't bother waiting for
STUN timeout, since we already verified
> On 2015/06/29 18:40:00, juberti1 wrote:
> > On 2015/06/27 at 17:05:49, Taylor Brandstetter wrote:
> > > On 2015/06/27 04:39:54, juberti1 wrote:
> > > > I don't understand this. Don't we want to get the response from the STUN
> > server,
> > > > to ensure it's not a different srflx candidate that we might want to
use?
> > > 
> > > In this case the main thing I wanted to test was that if a STUN server
isn't
> > available (see stun_server_.reset()), we still get a srflx candidate from
the
> > TURN server. This wasn't working before.
> > > 
> > > But you're right that testing getting two different srflx candidates would
be
> > even more valuable. I'll see if I can figure out how to configure two layers
of
> > NAT and put the STUN/TURN servers at different levels.
> > 
> > OK, I see what you are saying. I think the 2-level NAT might be overkill in
that
> > case.
> > 
> > The main thing that I am troubled by is the 3537 comment and the need for a
> > specific step delay; that seems like it is testing implementation details.
> 
> The 3537 comment and step delay aren't really needed for this test so I'll
remove them.
> 
>
https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
> File webrtc/p2p/client/basicportallocator.cc (right):
> 
>
https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
> webrtc/p2p/client/basicportallocator.cc:1167: for (ServerAddresses::iterator
turn_server = turn_servers.begin();
> On 2015/06/29 18:54:48, juberti1 wrote:
> > I think this could be done as 
> > 
> > for (auto turn_server : turn_servers)
> 
> Good point. In this case I think I'll use "for (rtc::SocketAddress turn_server
: turn_servers)", since "auto" doesn't really improve readability.

OK; suggest |rtc::SocketAddress& turn_server| to avoid unnecessary copies

>
https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/portall...
> File webrtc/p2p/client/portallocator_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/portall...
> webrtc/p2p/client/portallocator_unittest.cc:1017:
EXPECT_NE(candidates_[2].related_address(), candidates_[1].address());
> On 2015/06/29 18:54:49, juberti1 wrote:
> > But candidates_[2] is not server reflexive, so this comment is confusing.
> > 
> > I think this test should come right after the previous TurnAsStun tests,
since
> > it is most similar to them.
> 
> candidates_[2] isn't server reflexive, but its related_address is; see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Makes sense; I read too quickly and missed the related_ prefix.

[Taylor Brandstetter, 2015-06-30 18:56:51]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-06-30 18:56:51]

The CQ bit was unchecked by deadbeef@webrtc.org

[pthatcher1, 2015-07-06 22:28:32]

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:928: if (config_ &&
!config_->StunServers().empty()) {
Does it make sense to check for !empty() still?  Should we just always
set_server_addresses?

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:1166: PROTO_UDP);
Please run "git cl format" and use whatever style it chooses here.

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:1166: PROTO_UDP);
Do we still need the check for SupportsProtocol(RELAY_TURN, PROTO_UDP)?

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:1167: for (ServerAddresses::iterator
turn_server = turn_servers.begin();
On 2015/06/29 20:18:34, Taylor Brandstetter wrote:
> On 2015/06/29 18:54:48, juberti1 wrote:
> > I think this could be done as 
> > 
> > for (auto turn_server : turn_servers)
> 
> Good point. In this case I think I'll use "for (rtc::SocketAddress turn_server
:
> turn_servers)", since "auto" doesn't really improve readability.

I think 

for (const rtc::SocketAddress& turn_server : turn_servers) {
  ...
}

Would be even better.  It's how we normally have it in the code base.

https://codereview.webrtc.org/1215713003/diff/60001/webrtc/p2p/client/portall...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1215713003/diff/60001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:921: // 'relay' candidates.
Non-shared sockets will be deleted soon (I'm surprised they haven't been
already).  Can you just make all the test use shared-socket mode?

[Taylor Brandstetter, 2015-07-07 00:00:03]

deadbeef@webrtc.org changed reviewers:
- guoweis@webrtc.org, juberti@webrtc.org

[Taylor Brandstetter, 2015-07-07 00:00:04]

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:928: if (config_ &&
!config_->StunServers().empty()) {
On 2015/07/06 22:28:32, pthatcher1 wrote:
> Does it make sense to check for !empty() still?  Should we just always
> set_server_addresses?

It's a little redundant but at least it means you won't see a log message about
STUN candidates if there are none.

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:1166: PROTO_UDP);
On 2015/07/06 22:28:32, pthatcher1 wrote:
> Please run "git cl format" and use whatever style it chooses here.

Done

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:1166: PROTO_UDP);
On 2015/07/06 22:28:32, pthatcher1 wrote:
> Do we still need the check for SupportsProtocol(RELAY_TURN, PROTO_UDP)?  

GetRelayServerAddresses(RELAY_TURN, PROTO_UDP) will do that for us.

https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:1167: for (ServerAddresses::iterator
turn_server = turn_servers.begin();
On 2015/07/06 22:28:32, pthatcher1 wrote:
> On 2015/06/29 20:18:34, Taylor Brandstetter wrote:
> > On 2015/06/29 18:54:48, juberti1 wrote:
> > > I think this could be done as 
> > > 
> > > for (auto turn_server : turn_servers)
> > 
> > Good point. In this case I think I'll use "for (rtc::SocketAddress
turn_server
> :
> > turn_servers)", since "auto" doesn't really improve readability.
> 
> I think 
> 
> for (const rtc::SocketAddress& turn_server : turn_servers) {
>   ...
> }
> 
> Would be even better.  It's how we normally have it in the code base.

Already did that. :)

https://codereview.webrtc.org/1215713003/diff/60001/webrtc/p2p/client/portall...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1215713003/diff/60001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:921: // 'relay' candidates.
On 2015/07/06 22:28:32, pthatcher1 wrote:
> Non-shared sockets will be deleted soon (I'm surprised they haven't been
> already).  Can you just make all the test use shared-socket mode?

Well, when that happens we can easily just remove the
"TestNonSharedSocketWithNatUsingTurnAsStun" test since it will be obsolete.

But I don't know what to do about the
"TestNonSharedSocketWithoutNatUsingTurnTcp" test in that case, which aims to
make sure a TCP TURN server isn't used as a STUN server. If we used shared
socket mode, then we couldn't differentiate based on the number of ports
allocated. And since TCP doesn't work through the NAT infrastructure we have, I
can't use a NAT in this test. Which means I won't be able to tell if we
attempted to use the TURN server as a STUN server, since the STUN candidate
would have been discarded anyway.

So... Should we try to get TCP working through the NAT socket server? Or do you
have any other ideas?

[pthatcher1, 2015-07-07 06:06:09]

On 2015/07/07 00:00:04, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
> File webrtc/p2p/client/basicportallocator.cc (right):
> 
>
https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
> webrtc/p2p/client/basicportallocator.cc:928: if (config_ &&
> !config_->StunServers().empty()) {
> On 2015/07/06 22:28:32, pthatcher1 wrote:
> > Does it make sense to check for !empty() still?  Should we just always
> > set_server_addresses?
> 
> It's a little redundant but at least it means you won't see a log message
about
> STUN candidates if there are none.
> 
>
https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
> webrtc/p2p/client/basicportallocator.cc:1166: PROTO_UDP);
> On 2015/07/06 22:28:32, pthatcher1 wrote:
> > Please run "git cl format" and use whatever style it chooses here.
> 
> Done
> 
>
https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
> webrtc/p2p/client/basicportallocator.cc:1166: PROTO_UDP);
> On 2015/07/06 22:28:32, pthatcher1 wrote:
> > Do we still need the check for SupportsProtocol(RELAY_TURN, PROTO_UDP)?  
> 
> GetRelayServerAddresses(RELAY_TURN, PROTO_UDP) will do that for us.
> 
>
https://codereview.webrtc.org/1215713003/diff/20001/webrtc/p2p/client/basicpo...
> webrtc/p2p/client/basicportallocator.cc:1167: for (ServerAddresses::iterator
> turn_server = turn_servers.begin();
> On 2015/07/06 22:28:32, pthatcher1 wrote:
> > On 2015/06/29 20:18:34, Taylor Brandstetter wrote:
> > > On 2015/06/29 18:54:48, juberti1 wrote:
> > > > I think this could be done as 
> > > > 
> > > > for (auto turn_server : turn_servers)
> > > 
> > > Good point. In this case I think I'll use "for (rtc::SocketAddress
> turn_server
> > :
> > > turn_servers)", since "auto" doesn't really improve readability.
> > 
> > I think 
> > 
> > for (const rtc::SocketAddress& turn_server : turn_servers) {
> >   ...
> > }
> > 
> > Would be even better.  It's how we normally have it in the code base.
> 
> Already did that. :)
> 
>
https://codereview.webrtc.org/1215713003/diff/60001/webrtc/p2p/client/portall...
> File webrtc/p2p/client/portallocator_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1215713003/diff/60001/webrtc/p2p/client/portall...
> webrtc/p2p/client/portallocator_unittest.cc:921: // 'relay' candidates.
> On 2015/07/06 22:28:32, pthatcher1 wrote:
> > Non-shared sockets will be deleted soon (I'm surprised they haven't been
> > already).  Can you just make all the test use shared-socket mode?
> 
> Well, when that happens we can easily just remove the
> "TestNonSharedSocketWithNatUsingTurnAsStun" test since it will be obsolete.

Good point.  Since you already wrote it, I guess you could leave it.  In that
case, at least put a TODO like "TODO(deadbeef): Remove this test when support
for non-shared sockets is removed.", so that the person removing non-shared
sockets doesn't feel blocked by this test.
> 
> But I don't know what to do about the
> "TestNonSharedSocketWithoutNatUsingTurnTcp" test in that case, which aims to
> make sure a TCP TURN server isn't used as a STUN server. If we used shared
> socket mode, then we couldn't differentiate based on the number of ports
> allocated. And since TCP doesn't work through the NAT infrastructure we have,
I
> can't use a NAT in this test. Which means I won't be able to tell if we
> attempted to use the TURN server as a STUN server, since the STUN candidate
> would have been discarded anyway.
> 
> So... Should we try to get TCP working through the NAT socket server? Or do
you
> have any other ideas?

If you have a TCP socket and a NAT in the test, the TCP traffic won't get
through, but STUN UDP traffic would (if the code were wrong, that is).  So,
would't the lack of a UDP STUN candidate mean the code is correct?  In other
words, if we change the code to be broken and use the TCP TURN server as a UDP
STUN server, and we checked the candidates, wouldn't we see a UDP STUN
candidate?   


Another option, which would actually be more thorough in testing for privacy
issues,  would be to have record the last UDP packet sent, and make sure it
nothing is ever sent.  That could be done in the VirtualSocketServer, or the
NAT, or the TestTurnServer.  I think this might be a good spot:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Maybe you could subclass TestTurnServer, override AddInternalSocket, count how
many times (proto == cricket::PROTO_UDP) and then make sure it's zero.

[Taylor Brandstetter, 2015-07-13 23:23:41]

All comments should be addressed now.

I got TCP working through the NATServer, using the ProxyServer class. The work
was already about half done (on the NATSocketFactory side), I just had to finish
it on the NATServer side.

Anyway: this means that the "don't use TCP TURN server as UDP STUN server" test
is now more robust, since it goes through a NAT and doesn't rely on the
"non-shared socket mode".

[pthatcher1, 2015-07-15 18:22:40]

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.cc
File webrtc/base/natserver.cc (right):

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.cc...
webrtc/base/natserver.cc:83: if (*len < kNATEncodedIPv6AddressSize)
Please use {}s (consistent with lines 90-02)

Also on lines 97-98.

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.cc...
webrtc/base/natserver.cc:94: bool remainder = (*len > 0);
Can you make this code shared with the other place in the code base that does
the same thing (NATServer::OnInternalUDPPacket)?

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.h
File webrtc/base/natserver.h (right):

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.h#...
webrtc/base/natserver.h:58: const int NAT_SERVER_TCP_PORT = 4238;
Would it simply the code to just always have the UDP and TCP internal addresses
always be the same?  It might save on lots of arguments being passed around.

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natsocketfac...
File webrtc/base/natsocketfactory.cc (right):

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natsocketfac...
webrtc/base/natsocketfactory.cc:272: PackAddressForNAT(buf.get(),
kNATEncodedIPv6AddressSize, remote_addr_);
Looks like an rtc::Buffer might be better here.

[Taylor Brandstetter, 2015-07-15 19:44:23]

I addressed your comments, and I also fixed a bug with the TCP NAT interface,
which was being masked by another bug (which I also fixed).

If you're curious: the bug was that the NATSocket was always sending 20 bytes to
indicate the remote address, but if it was an IPv4 address only 12 bytes were
being read off, leaving 8 bytes of junk data. But that junk data was discarded,
due to a bug in BufferedReadAdapter.

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.cc
File webrtc/base/natserver.cc (right):

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.cc...
webrtc/base/natserver.cc:83: if (*len < kNATEncodedIPv6AddressSize)
On 2015/07/15 18:22:39, pthatcher1 wrote:
> Please use {}s (consistent with lines 90-02)
> 
> Also on lines 97-98.

Done.

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.cc...
webrtc/base/natserver.cc:94: bool remainder = (*len > 0);
On 2015/07/15 18:22:39, pthatcher1 wrote:
> Can you make this code shared with the other place in the code base that does
> the same thing (NATServer::OnInternalUDPPacket)? 

Not really, because it works differently for UDP and TCP. UDP, being
datagram-oriented, provides a remote address with every packet. But for TCP, the
remote address is provided as the first few bytes in the steam, which causes the
remote connection to be made. That's what this code is doing. After
BufferInput(false) is called, input will no longer come here and instead will
start being handled by the ProxyServer.

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.h
File webrtc/base/natserver.h (right):

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natserver.h#...
webrtc/base/natserver.h:58: const int NAT_SERVER_TCP_PORT = 4238;
On 2015/07/15 18:22:39, pthatcher1 wrote:
> Would it simply the code to just always have the UDP and TCP internal
addresses
> always be the same?  It might save on lots of arguments being passed around.

It WOULD simplify things a lot, but unfortunately the VirtualSocketServer class
doesn't allow a UDP socket and TCP socket to share the same port.

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natsocketfac...
File webrtc/base/natsocketfactory.cc (right):

https://codereview.webrtc.org/1215713003/diff/120001/webrtc/base/natsocketfac...
webrtc/base/natsocketfactory.cc:272: PackAddressForNAT(buf.get(),
kNATEncodedIPv6AddressSize, remote_addr_);
On 2015/07/15 18:22:39, pthatcher1 wrote:
> Looks like an rtc::Buffer might be better here.

Actually this originally had a char[] on the stack and I think that should be
sufficient. I forgot why I changed it; the reason probably became obsolete at
some point.

[pthatcher1, 2015-07-15 23:42:24]

lgtm

If you want to cleanup the last item, it's up to you.

https://codereview.webrtc.org/1215713003/diff/140001/webrtc/base/socketadapte...
File webrtc/base/socketadapters.cc (right):

https://codereview.webrtc.org/1215713003/diff/140001/webrtc/base/socketadapte...
webrtc/base/socketadapters.cc:88: }
I think this would be more clear as something like this:

if (res >= 0) {
  // Read from socket and maybe buffer.  Return combined length.
  return res + static_cast<int>(read);
}

if (read > 0) {
  // Read from buffer and failed to read from socket.
  // Return amount read from buffer.  
  return read;
}

// Didn't read from buffer and failed to read from socket.
// Return error from socket.
return res;

[Taylor Brandstetter, 2015-07-16 01:23:09]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-07-16 01:23:10]

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/1215713003/#ps160001 (title:
"Adding some comments to make logic in BufferedReadAdapter a bit more clear")

[commit-bot: I haz the power, 2015-07-16 01:23:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215713003/160001

[commit-bot: I haz the power, 2015-07-16 01:27:06]

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

[commit-bot: I haz the power, 2015-07-16 01:27:08]

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

[Taylor Brandstetter, 2015-07-16 16:21:42]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-07-16 16:21:42]

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/1215713003/#ps180001 (title:
"Fixing compiler warning from size_t->int cast")

[commit-bot: I haz the power, 2015-07-16 16:22:16]

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

[commit-bot: I haz the power, 2015-07-16 17:22:27]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-07-16 17:22:40]

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