Support port range for IPC P2P UDP sockets.

Support port range for IPC P2P UDP sockets.

BUG=342476
Committed: https://crrev.com/b1c12ea6403aa8ec65ebaf62273d50871198d1e0
Cr-Commit-Position: refs/heads/master@{#405742}

[Guido Urdaneta, 2016-07-11 17:14:54]

guidou@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Guido Urdaneta, 2016-07-11 17:14:55]

Hi, PTAL

https://codereview.chromium.org/2140693002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/p2p/socket_host_tcp_server.cc (right):

https://codereview.chromium.org/2140693002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_host_tcp_server.cc:42: // TODO(guidou):
Add support for port range.
I can add this support in a follow-up CL or in another iteration of this CL.

[tommi (sloooow) - chröme, 2016-07-11 17:54:58]

tommi@chromium.org changed reviewers:
+ tommi@chromium.org

[tommi (sloooow) - chröme, 2016-07-11 17:54:59]

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host_tcp_server.cc (right):

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_tcp_server.cc:42: // TODO(guidou):
Add support for port range.
I guess this means a separate policy then?

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_udp.cc:145:
DCHECK(local_address.port() == 0 || (min_port == 0 && max_port == 0));
what if e.g. local_address.port() != 0 but the policy is also specified?

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_udp.cc:148: if (min_port == 0) {
maybe this should be:

if (min_port == 0 || local_address.port() != 0) {

But even so, we probably need an extra, nested, check:

  if (local_address.port() < min_port || local_address.port() > max_port) {
    result = -1;  // or some appropriate error value.
  } else {
    result = socket_.Listen(...);
  }

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_udp.cc:152: result =
socket_->Listen(net::IPEndPoint(local_address.address(), port));
otherwise, here we ignore local_address.port() if it's not 0 and could
potentially bind it to the wrong port.

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host_udp_unittest.cc (right):

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_udp_unittest.cc:179: new
std::vector<uint16_t>;
would be good to avoid global vars like this considering parallel or repeated
runs.
Can we instead, have a factory object that owns the vector and instances get a
pointer back to the vector that's owned by the factory? (and the factory goes
out of scope when the test ends)

[Guido Urdaneta, 2016-07-12 10:15:00]

Hi, PTAL.
The main issue was that I was not treating the port range as a policy to be
enforced, but as an alternative mechanism to specify the port.
This PS addresses this and updates the documentation to make it clear how it is
supposed to work.

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host_tcp_server.cc (right):

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_tcp_server.cc:42: // TODO(guidou):
Add support for port range.
On 2016/07/11 17:54:59, tommi-chrömium wrote:
> I guess this means a separate policy then?

In the meantime the only call site is
https://cs.chromium.org/chromium/src/content/renderer/p2p/ipc_socket_factory....
which ignores the range.

In this PS we pass the range to the browser, but ignore it there.
What do you think we should do in this CL?

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_udp.cc:145:
DCHECK(local_address.port() == 0 || (min_port == 0 && max_port == 0));
On 2016/07/11 17:54:59, tommi-chrömium wrote:
> what if e.g. local_address.port() != 0 but the policy is also specified?

The original PS was not treating the range as policy, but as an alternative way
to specify the port.
The new PS treats the range as policy.

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_udp.cc:148: if (min_port == 0) {
On 2016/07/11 17:54:59, tommi-chrömium wrote:
> maybe this should be:
> 
> if (min_port == 0 || local_address.port() != 0) {
> 
> But even so, we probably need an extra, nested, check:
> 
>   if (local_address.port() < min_port || local_address.port() > max_port) {
>     result = -1;  // or some appropriate error value.
>   } else {
>     result = socket_.Listen(...);
>   }

Done.

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_udp.cc:152: result =
socket_->Listen(net::IPEndPoint(local_address.address(), port));
On 2016/07/11 17:54:59, tommi-chrömium wrote:
> otherwise, here we ignore local_address.port() if it's not 0 and could
> potentially bind it to the wrong port.

Done.

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host_udp_unittest.cc (right):

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_udp_unittest.cc:179: new
std::vector<uint16_t>;
On 2016/07/11 17:54:59, tommi-chrömium wrote:
> would be good to avoid global vars like this considering parallel or repeated
> runs.
> Can we instead, have a factory object that owns the vector and instances get a
> pointer back to the vector that's owned by the factory? (and the factory goes
> out of scope when the test ends)

Done.

[tommi (sloooow) - chröme, 2016-07-12 10:21:47]

lgtm

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host_tcp_server.cc (right):

https://codereview.chromium.org/2140693002/diff/20001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host_tcp_server.cc:42: // TODO(guidou):
Add support for port range.
On 2016/07/12 10:15:00, Guido Urdaneta wrote:
> On 2016/07/11 17:54:59, tommi-chrömium wrote:
> > I guess this means a separate policy then?
> 
> In the meantime the only call site is
>
https://cs.chromium.org/chromium/src/content/renderer/p2p/ipc_socket_factory....
> which ignores the range.
> 
> In this PS we pass the range to the browser, but ignore it there.
> What do you think we should do in this CL?

I think it's OK to not address it in this CL but a followup CL would be good.

[Guido Urdaneta, 2016-07-12 12:06:27]

guidou@chromium.org changed reviewers:
+ jochen@chromium.org

[Guido Urdaneta, 2016-07-12 12:06:27]

jochen@chromium.org: Please review changes in content/common

[Guido Urdaneta, 2016-07-12 15:53:06]

guidou@chromium.org changed reviewers:
+ palmer@chromium.org

[Guido Urdaneta, 2016-07-12 15:53:07]

palmer@chromium.org: Please review changes in content/common/p2p_messages.h

[jochen (gone - plz use gerrit), 2016-07-12 16:01:41]

lgtm

[Guido Urdaneta, 2016-07-14 08:58:18]

Description was changed from

==========
Support port range for IPC P2P UDP sockets.

BUG=342476
==========

to

==========
Support port range for IPC P2P UDP sockets.

BUG=342476
==========

[Guido Urdaneta, 2016-07-14 08:58:18]

guidou@chromium.org changed reviewers:
+ mkwst@chromium.org
- palmer@chromium.org

[Guido Urdaneta, 2016-07-14 08:59:08]

mkwst@: can you take a look at p2p_messages.h?

[Mike West, 2016-07-14 09:21:06]

https://codereview.chromium.org/2140693002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host.h (right):

https://codereview.chromium.org/2140693002/diff/40001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host.h:51: // are valid.
Can these restrictions be enforced via `bad_message::ReceivedBadMessage(...)` in
`P2PSocketDispatcherHost::OnCreateSocket`?

[Guido Urdaneta, 2016-07-14 10:59:09]

https://codereview.chromium.org/2140693002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host.h (right):

https://codereview.chromium.org/2140693002/diff/40001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host.h:51: // are valid.
On 2016/07/14 09:21:06, Mike West wrote:
> Can these restrictions be enforced via `bad_message::ReceivedBadMessage(...)`
in
> `P2PSocketDispatcherHost::OnCreateSocket`?

Done.

[Mike West, 2016-07-14 11:02:39]

IPC LGTM, thanks.

[Guido Urdaneta, 2016-07-14 11:24:21]

guidou@chromium.org changed reviewers:
+ isherman@chromium.org

[Guido Urdaneta, 2016-07-14 11:24:22]

isherman: please review histograms.xml

[Guido Urdaneta, 2016-07-14 12:39:42]

On 2016/07/14 11:24:22, Guido Urdaneta wrote:
> isherman: please review histograms.xml

histograms.xml will probably have to be rebased on top of
https://codereview.chromium.org/2149943002/

[Ilya Sherman, 2016-07-14 18:54:37]

histograms.xml gltm

[Ilya Sherman, 2016-07-14 18:54:45]

On 2016/07/14 18:54:37, Ilya Sherman wrote:
> histograms.xml gltm

lgtm, even

[Guido Urdaneta, 2016-07-15 10:23:19]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-07-15 10:23:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
tommi@chromium.org, isherman@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2140693002/#ps80001
(title: "Rebase and remove unused constructor from test")

[commit-bot: I haz the power, 2016-07-15 10:23:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Guido Urdaneta, 2016-07-15 10:39:55]

sergeyu@chromium.org: I'm landing this assuming you cannot review it. Please
comment later on if you have any observations.

[commit-bot: I haz the power, 2016-07-15 11:19:00]

Description was changed from

==========
Support port range for IPC P2P UDP sockets.

BUG=342476
==========

to

==========
Support port range for IPC P2P UDP sockets.

BUG=342476
==========

[commit-bot: I haz the power, 2016-07-15 11:19:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-15 11:20:19]

Description was changed from

==========
Support port range for IPC P2P UDP sockets.

BUG=342476
==========

to

==========
Support port range for IPC P2P UDP sockets.

BUG=342476

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

[commit-bot: I haz the power, 2016-07-15 11:20:21]

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