Adding PortAllocator option to support cases where sockets can't be bound.

Adding PortAllocator option to support cases where sockets can't be bound.

This CL adds the flag "PORTALLOCATOR_ENABLE_ANY_ADDRESS_PORTS", which will
force the creation of ports not bound to any specific network interface.
These are normally only used when network enumeration fails or is disabled,
but in some circumstances (such as the one the test case adds), they're the
only thing that works.

This will result in extra ports being gathered, which is why it's only enabled
behind a flag for now. In the future, we could probably introduce more
sophisticated "pruning" logic that would lessen the impact of the extra ports
when they're redundant, and make the flag the default.

Some other minor changes that were required to make this use case work:

* Allow a TCPPort to be used for outgoing connections even if it tries and
  fails to create a server socket.
* Allow Bind to fail if being called before Connect, and the IP is an "any"
  address (0.0.0.0 or ::), since this bind would have been mostly pointless
  anyway.
* Prevent P2PTransprotChannel from keeping a "backup" candidate pair using
  an "any address" network; we only want this for actual networks.

BUG=webrtc:7798
Review-Url: https://codereview.webrtc.org/2936553003
Cr-Commit-Position: refs/heads/master@{#18578}
Committed: https://chromium.googlesource.com/external/webrtc/+/1ee2125909e90058981f123a0e5a665125a92336

[Taylor Brandstetter, 2017-06-13 03:26:17]

Description was changed from

==========
Adding PortAllocator option to support cases where sockets can't be bound.

This CL adds the flag "PORTALLOCATOR_ENABLE_ANY_ADDRESS_PORTS", which will
force the creation of ports not bound to any specific network interface.
These are normally only used when network enumeration fails or is disabled,
but in some circumstances (such as the one the test case adds), they're the
only thing that works.

This will result in extra ports being gathered, which is why it's only enabled
behind a flag for now. In the future, we could probably introduce more
sophisticated "pruning" logic that would lessen the impact of the extra ports
when they're redundant, and make the flag the default.

Some other minor changes that were required to make this use case work:

* Allow a TCPPort to be used for incoming connections even if it tries and
  fails to create a server socket.
* Allow Bind to fail if being called before Connect, and the IP is an "any"
  address (0.0.0.0 or ::), since this bind would have been mostly pointless
  anyway.
* Prevent P2PTransprotChannel from keeping a "backup" candidate pair using
  an "any address" network; we only want this for actual networks.

BUG=webrtc:7798
==========

to

==========
Adding PortAllocator option to support cases where sockets can't be bound.

This CL adds the flag "PORTALLOCATOR_ENABLE_ANY_ADDRESS_PORTS", which will
force the creation of ports not bound to any specific network interface.
These are normally only used when network enumeration fails or is disabled,
but in some circumstances (such as the one the test case adds), they're the
only thing that works.

This will result in extra ports being gathered, which is why it's only enabled
behind a flag for now. In the future, we could probably introduce more
sophisticated "pruning" logic that would lessen the impact of the extra ports
when they're redundant, and make the flag the default.

Some other minor changes that were required to make this use case work:

* Allow a TCPPort to be used for outgoing connections even if it tries and
  fails to create a server socket.
* Allow Bind to fail if being called before Connect, and the IP is an "any"
  address (0.0.0.0 or ::), since this bind would have been mostly pointless
  anyway.
* Prevent P2PTransprotChannel from keeping a "backup" candidate pair using
  an "any address" network; we only want this for actual networks.

BUG=webrtc:7798
==========

[Taylor Brandstetter, 2017-06-13 03:41:29]

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

[Taylor Brandstetter, 2017-06-13 03:41:30]

PTAL Peter.

Zhi, thought you may want to take a look as well; this is what I mentioned over
lunch.

https://codereview.webrtc.org/2936553003/diff/1/webrtc/base/natsocketfactory.cc
File webrtc/base/natsocketfactory.cc (right):

https://codereview.webrtc.org/2936553003/diff/1/webrtc/base/natsocketfactory....
webrtc/base/natsocketfactory.cc:108: result =
BindInternal(SocketAddress(GetAnyIP(family_), 0));
This is done by other socket factory classes, it just wasn't necessary in this
one until now.

https://codereview.webrtc.org/2936553003/diff/1/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/2936553003/diff/1/webrtc/base/network.h#newcode123
webrtc/base/network.h:123: // can optionally be called after GetNetworks.
Wanted to add this comment because I'm now starting to depend on the fact that
"GetNetworks" doesn't clear the list; I'm not sure if it was intentional
originally.

https://codereview.webrtc.org/2936553003/diff/1/webrtc/p2p/base/tcpport.cc
File webrtc/p2p/base/tcpport.cc (left):

https://codereview.webrtc.org/2936553003/diff/1/webrtc/p2p/base/tcpport.cc#ol...
webrtc/p2p/base/tcpport.cc:104: // should never happen.
"this should never happen" - juberti, Oct 13, 2013

[pthatcher1, 2017-06-13 19:52:50]

lgtm

It's all just stylish stuff.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/firewallsocke...
File webrtc/base/firewallsocketserver.h (right):

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/firewallsocke...
webrtc/base/firewallsocketserver.h:61: // Set the IP addresses which can't be
bound to using Bind. By default this
"which can't be bound to using Bind" => "for which Bind will fail"?

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/firewallsocke...
webrtc/base/firewallsocketserver.h:69: bool CanBindToIp(const rtc::IPAddress&
ip);
Might make these symmetric: SetInvalidBindIps and IsInvalidBindIp.   

And I'd also suggest using the word "(un)bindable".  "SetUnbindableIps" and
"IsBindable" and unbindable_ips_.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/firewallsocke...
webrtc/base/firewallsocketserver.h:97: // list is empty, any address is
accepted.
You just need one comment to explain this, not two (here and in the Set method).
 One can reference the other.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/natsocketfact...
File webrtc/base/natsocketfactory.cc (right):

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/natsocketfact...
webrtc/base/natsocketfactory.cc:220: return socket_ ? socket_->GetError() :
error_;
Is setting and getting error_ sufficient?  Why do we also need socket_->GetError
and SetError?

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/basicpack...
File webrtc/p2p/base/basicpacketsocketfactory.cc (right):

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/basicpack...
webrtc/p2p/base/basicpacketsocketfactory.cc:117: // Allow BindSocket to fail if
we're binding to the ANY address anyway. The
Not sure what "anyway" adds to the end of the sentence.  It seems like you could
remove it.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/basicpack...
webrtc/p2p/base/basicpacketsocketfactory.cc:121: << "; ignoring since socket is
using 'any' address.";
Can you use an early return and then leave the rest of the code outside of the
else block?

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

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1372: }
Instead of calling it "to_compare", I'd call it "best_conn" and then do
something like this:

// Do not prune connections if the current best connection
// is weak.  Otherwise, it may delete connections prematurely.
Connection* best_conn = selected_connection_;
if (!rtc::IPIsAny(conn->port()->Network()->ip())) {
  // If the connection has a known network (not the "any address"),
  // Use the best connection for that network rather than
  // The best connection overall.  This allows us to have more than
  // one unpruned connection per known network.
  best_conn = best_connection_by_network[conn->port()->Network()];
}
// ...

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:81: // When this flag is set, ports will be
created not bound to any specific
"created not bound" => "created but not bound"

[Taylor Brandstetter, 2017-06-13 22:11:18]

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/firewallsocke...
File webrtc/base/firewallsocketserver.h (right):

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/firewallsocke...
webrtc/base/firewallsocketserver.h:61: // Set the IP addresses which can't be
bound to using Bind. By default this
On 2017/06/13 19:52:49, pthatcher1 wrote:
> "which can't be bound to using Bind" => "for which Bind will fail"?

Done.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/firewallsocke...
webrtc/base/firewallsocketserver.h:69: bool CanBindToIp(const rtc::IPAddress&
ip);
On 2017/06/13 19:52:49, pthatcher1 wrote:
> Might make these symmetric: SetInvalidBindIps and IsInvalidBindIp.   
> 
> And I'd also suggest using the word "(un)bindable".  "SetUnbindableIps" and
> "IsBindable" and unbindable_ips_.

Done.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/firewallsocke...
webrtc/base/firewallsocketserver.h:97: // list is empty, any address is
accepted.
On 2017/06/13 19:52:49, pthatcher1 wrote:
> You just need one comment to explain this, not two (here and in the Set
method).
>  One can reference the other.

Done.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/natsocketfact...
File webrtc/base/natsocketfactory.cc (right):

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/base/natsocketfact...
webrtc/base/natsocketfactory.cc:220: return socket_ ? socket_->GetError() :
error_;
On 2017/06/13 19:52:49, pthatcher1 wrote:
> Is setting and getting error_ sufficient?  Why do we also need
socket_->GetError
> and SetError?

GetError needs to return the inner socket's error if there is one. I'm not sure
why SetError is public but I didn't want to touch that logic in this CL.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/basicpack...
File webrtc/p2p/base/basicpacketsocketfactory.cc (right):

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/basicpack...
webrtc/p2p/base/basicpacketsocketfactory.cc:117: // Allow BindSocket to fail if
we're binding to the ANY address anyway. The
On 2017/06/13 19:52:50, pthatcher1 wrote:
> Not sure what "anyway" adds to the end of the sentence.  It seems like you
could
> remove it.

Rephrased comment.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/basicpack...
webrtc/p2p/base/basicpacketsocketfactory.cc:121: << "; ignoring since socket is
using 'any' address.";
On 2017/06/13 19:52:49, pthatcher1 wrote:
> Can you use an early return and then leave the rest of the code outside of the
> else block?

I knew you would ask for an early return here. :)

And that's how I wrote the code originally, but it's notably less readable that
way; not worth removing a level of indentation imo.

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

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1372: }
On 2017/06/13 19:52:50, pthatcher1 wrote:
> Instead of calling it "to_compare", I'd call it "best_conn" and then do
> something like this:
> 
> // Do not prune connections if the current best connection
> // is weak.  Otherwise, it may delete connections prematurely.
> Connection* best_conn = selected_connection_;
> if (!rtc::IPIsAny(conn->port()->Network()->ip())) {
>   // If the connection has a known network (not the "any address"),
>   // Use the best connection for that network rather than
>   // The best connection overall.  This allows us to have more than
>   // one unpruned connection per known network.
>   best_conn = best_connection_by_network[conn->port()->Network()];
> }
> // ...

Done.

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2936553003/diff/20001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:81: // When this flag is set, ports will be
created not bound to any specific
On 2017/06/13 19:52:50, pthatcher1 wrote:
> "created not bound" => "created but not bound"

Revised comment.

[Taylor Brandstetter, 2017-06-13 22:11:23]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-06-13 22:11:24]

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/2936553003/#ps40001 (title:
"Minor changes (comments, renaming, etc.)")

[commit-bot: I haz the power, 2017-06-13 22:11:30]

CQ is trying da patch.

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

[Taylor Brandstetter, 2017-06-13 22:13:29]

The CQ bit was unchecked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-06-13 22:20:55]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-06-13 22:21:00]

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, 2017-06-13 22:49:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497392455505920,
"parent_rev": "1d560e1b9ae9f3a1c61fe229b5b63bcf3aa971c3", "commit_rev":
"1ee2125909e90058981f123a0e5a665125a92336"}

[commit-bot: I haz the power, 2017-06-13 22:49:50]

Description was changed from

==========
Adding PortAllocator option to support cases where sockets can't be bound.

This CL adds the flag "PORTALLOCATOR_ENABLE_ANY_ADDRESS_PORTS", which will
force the creation of ports not bound to any specific network interface.
These are normally only used when network enumeration fails or is disabled,
but in some circumstances (such as the one the test case adds), they're the
only thing that works.

This will result in extra ports being gathered, which is why it's only enabled
behind a flag for now. In the future, we could probably introduce more
sophisticated "pruning" logic that would lessen the impact of the extra ports
when they're redundant, and make the flag the default.

Some other minor changes that were required to make this use case work:

* Allow a TCPPort to be used for outgoing connections even if it tries and
  fails to create a server socket.
* Allow Bind to fail if being called before Connect, and the IP is an "any"
  address (0.0.0.0 or ::), since this bind would have been mostly pointless
  anyway.
* Prevent P2PTransprotChannel from keeping a "backup" candidate pair using
  an "any address" network; we only want this for actual networks.

BUG=webrtc:7798
==========

to

==========
Adding PortAllocator option to support cases where sockets can't be bound.

This CL adds the flag "PORTALLOCATOR_ENABLE_ANY_ADDRESS_PORTS", which will
force the creation of ports not bound to any specific network interface.
These are normally only used when network enumeration fails or is disabled,
but in some circumstances (such as the one the test case adds), they're the
only thing that works.

This will result in extra ports being gathered, which is why it's only enabled
behind a flag for now. In the future, we could probably introduce more
sophisticated "pruning" logic that would lessen the impact of the extra ports
when they're redundant, and make the flag the default.

Some other minor changes that were required to make this use case work:

* Allow a TCPPort to be used for outgoing connections even if it tries and
  fails to create a server socket.
* Allow Bind to fail if being called before Connect, and the IP is an "any"
  address (0.0.0.0 or ::), since this bind would have been mostly pointless
  anyway.
* Prevent P2PTransprotChannel from keeping a "backup" candidate pair using
  an "any address" network; we only want this for actual networks.

BUG=webrtc:7798

Review-Url: https://codereview.webrtc.org/2936553003
Cr-Commit-Position: refs/heads/master@{#18578}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1ee2125909e90058981f123a0...
==========

[commit-bot: I haz the power, 2017-06-13 22:49:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/1ee2125909e90058981f123a0...