1194259 - Make ICE IP restriction to default routes work in E10S

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Eric Rescorla (:ekr)]

Bug 1189041 doesn't work on E10S. That needs to be fixed.

Comment 1

[Eric Rescorla (:ekr)]

Attachment: MozReview Request: Bug 1194259 - Make ICE IP restriction to default routes work in E10S

Bug 1194259 - Make ICE IP restriction to default routes work in E10S

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 8648865 [details]
MozReview Request: Bug 1194259 - Make ICE IP restriction to default routes work in E10S

Here is my first cut at remoting nsIUDPSocket::Connect().

Here are things I know are wrong:
1. I didn't update the UUID
2. I am doing the connect on the incoming thread.
3. There is a scary MOZ_CRASH() that is protecting an umimplemented ad hopefully unneeded callback in UDPSocket.cpp

Does this look approximately right. I would also appreciate guidance on the ownership model for #2, since if I make nsUDPSocket.cpp::Connect() async, then what keeps the parent alive.

Comment 3

[Jason Duell]

Comment on attachment 8648865 [details]
MozReview Request: Bug 1194259 - Make ICE IP restriction to default routes work in E10S

Ownership model for AsyncOpen is typically that we hold a ref to the listener (which should be the ParentChannel in e10s?) in between AsyncOpen and whenever the socket gets closed. 

Make sure that when you change the code to do the connect on the socket transport thread (it's non-blocking, right?), you still pass back any errors in connecting to the child, i.e. they need to wind up calling FireInternalError (on the main thread, I assume, since this is IPDL, unless IPDL for UDP sockets is running on a non-main thread).

>  // callback while socket is connected. localPort and localAddress is ready until this time.
>  void callListenerConnected();

is *not* ready until this time?

I don't know the ICE code well enough to comment on it, but the rest of the code looks like the approach is right.

I don't see you doing any IP restriction to default routes, but I'm assuming you'll add that later once you get the basics working here.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8648865 [details]
MozReview Request: Bug 1194259 - Make ICE IP restriction to default routes work in E10S

https://reviewboard.mozilla.org/r/16311/#review14851

f+ - hope mozReview sets the right flag...

::: media/mtransport/nr_socket_prsock.cpp:1138
(Diff revision 1)
> +    return rv;

Style nits (a few places): use braces on if() unless there's a hard "don't do that" for mtransport

::: media/mtransport/nr_socket_prsock.cpp:1155
(Diff revision 1)
> +    return rv;

Do we need to Notify on the monitor on failure?

::: netwerk/base/nsUDPSocket.cpp:870
(Diff revision 1)
> +    UDPSOCKET_LOG(("NOT ON STS THREAD"));

Likely planned once you fix up the thread issue, but should assert on the thread instead of log-and-continue

::: netwerk/base/nsUDPSocket.cpp:889
(Diff revision 1)
> +  return NS_OK;

extra blank lines

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8648865 [details]
MozReview Request: Bug 1194259 - Make ICE IP restriction to default routes work in E10S

It didn't of course

Comment 6

[Maire Reavy [:mreavy]]

Ekr - Given the e10s release plans, we should land this in 45 (which uplifts in mid-December).  Will you have time to finish this off for Fx45 or should I find someone else to take over?  Thanks.

Comment 7

[Eric Rescorla (:ekr)]

You should probably find someone else to finish it.

Comment 8

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm

Bug 1194259 - Make ICE IP restriction to default routes work in E10S

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 8687512 [details]
MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25219/diff/1-2/

Comment 10

[Maire Reavy [:mreavy]]

Nils is going on PTO after tomorrow so Randell is going to take this across the finish line.

Nils -- Can you upload your latest patch set to this bug?

Comment 11

[Nils Ohlmeier [:drno]]

The latest patch set is the one in mozreview which I uploaded in comment #9.
That patch set should "only" the one remaining issue of having to dispatch the connect() call to STS and how to store a reference for that call and the result of that call.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm

Review commit: https://reviewboard.mozilla.org/r/31573/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31573/

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1194259: update ICE IP restriction code to not call PR_Connect() on the PBackground thread r?drno,mcmanus

Review commit: https://reviewboard.mozilla.org/r/31575/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31575/

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Patrick - please feel free to forward these review requests as appropriate (dragana, jduell, jdm, ...)

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 8709835 [details]
MozReview Request: Bug 1194259: update ICE IP restriction code to not call PR_Connect() on the PBackground thread r?drno,mcmanus

https://reviewboard.mozilla.org/r/31575/#review28353

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 8709834 [details]
MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm

https://reviewboard.mozilla.org/r/31573/#review28355

::: netwerk/base/nsUDPSocket.cpp:684
(Diff revision 1)
> +    // TODO turn this into ASSERT

is there a reason this isn't done? I know that connect on udp won't actually do any io, but we're less sure of the threadsafety of things that might hook the io methods..

Comment 17

[Nils Ohlmeier [:drno]]

Comment on attachment 8709835 [details]
MozReview Request: Bug 1194259: update ICE IP restriction code to not call PR_Connect() on the PBackground thread r?drno,mcmanus

https://reviewboard.mozilla.org/r/31575/#review28411

LGTM (up to you if you want to change the log levels)

::: media/mtransport/nr_socket_prsock.cpp:1190
(Diff revision 1)
> +  r_log(LOG_GENERIC, LOG_INFO, "UDP socket opened this=%p", (void*) this);

Nit: this looks more like LOG_DEBUG to me.

::: media/mtransport/nr_socket_prsock.cpp:1207
(Diff revision 1)
> +  r_log(LOG_GENERIC, LOG_INFO, "UDP socket connected this=%p", (void*) this);

Nit: for consistency LOG_DEBUG

::: media/mtransport/nr_socket_prsock.cpp:1228
(Diff revision 1)
> +  r_log(LOG_GENERIC, LOG_INFO, "UDP socket closed this=%p", (void*) this);

Nit: and another LOG_DEBUG

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8709834 [details]
MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm

https://reviewboard.mozilla.org/r/31573/#review28525

::: dom/network/UDPSocketParent.cpp:231
(Diff revision 1)
>    rv = sock->SetMulticastLoopback(aLoopback);

Nils - we need to resolve this. What's the deal here?  Can we uncomment it and check for IPV6?  Do we need to file a bug for IPV6 support in SetMulticastLoopback?

::: media/mtransport/nr_socket_prsock.cpp:1318
(Diff revision 1)
> +  r_log(LOG_GENERIC,LOG_DEBUG,"NrUdpSocketIpc::close()");

General comment, spaces after commas

::: media/mtransport/nr_socket_prsock.cpp:1419
(Diff revision 1)
> +  _status=0;

spaces around =

::: netwerk/base/nsUDPSocket.cpp:685
(Diff revision 1)
> +    UDPSOCKET_LOG(("NOT ON STS THREAD"));

I'll make this an assert

Comment 19

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/31573/#review28525

> Nils - we need to resolve this. What's the deal here?  Can we uncomment it and check for IPV6?  Do we need to file a bug for IPV6 support in SetMulticastLoopback?

On my machine the SetMulticastLoopback() call always failed for IPv6 sockets. I failed to find out what that call is actually suppose to do because Necko uses non standard socket flags (I think this just tries to set a socket flag).
As I don't understand what this call is suppose the options for my point of view are:
A) Remove that call altogether
B) Only call this for IPv4 sockets
C) File a bug with Necko for implementing SetMulticastLoopback for IPv6
Or a combination of B and C.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

I hate mozreview sometimes:
 Flags: review?([e-mail]) → review?([e-mail])

Patrick: any thoughts about comment 19?

Comment 21

[Patrick McManus [:mcmanus]]

I'm not really familiar with it.. but maybe jryans

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nils Ohlmeier [:drno] from comment #19)
> https://reviewboard.mozilla.org/r/31573/#review28525
> 
> > Nils - we need to resolve this. What's the deal here?  Can we uncomment it and check for IPV6?  Do we need to file a bug for IPV6 support in SetMulticastLoopback?
> 
> On my machine the SetMulticastLoopback() call always failed for IPv6
> sockets. I failed to find out what that call is actually suppose to do
> because Necko uses non standard socket flags (I think this just tries to set
> a socket flag).
> As I don't understand what this call is suppose the options for my point of
> view are:
> A) Remove that call altogether
> B) Only call this for IPv4 sockets
> C) File a bug with Necko for implementing SetMulticastLoopback for IPv6
> Or a combination of B and C.

It's meant to allow disabling multicast loopback if desired, like the following test:

https://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_udp_multicast.js#84

AFAIK, the existing in-product usages of multicast are all IPv4 not IPv6.  So, we should at least do B to preserve IPv4 behavior.  Doing C as well just to track the work seems good, but it does not seem like the functionality is needed immediately (based on existing usages at least).

Comment 23

[Nils Ohlmeier [:drno]]

Comment on attachment 8687512 [details]
MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25219/diff/2-3/

Comment 24

[Nils Ohlmeier [:drno]]

Ryan, could you have a look at the interdiff: https://reviewboard.mozilla.org/r/25219/diff/2-3/
and let me know if that looks like a sane way of doing the SetMulticastLoopback() call only for IPv4 sockets?

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8687512 [details]
MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm

https://reviewboard.mozilla.org/r/25219/#review28565

r+ - not sure what the "whitespace-only" changes are, but that's fine.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8687512 [details]
MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm

Sigh, sometimes I hate mozreview

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8709834 [details]
MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm

This is a mess....

Comment 28

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nils Ohlmeier [:drno] from comment #24)
> Ryan, could you have a look at the interdiff:
> https://reviewboard.mozilla.org/r/25219/diff/2-3/
> and let me know if that looks like a sane way of doing the
> SetMulticastLoopback() call only for IPv4 sockets?

It seems reasonable to me.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c372359d5ba

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dafc2f29765

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/542c5ae29fdf

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5c372359d5ba
https://hg.mozilla.org/mozilla-central/rev/0dafc2f29765
https://hg.mozilla.org/mozilla-central/rev/542c5ae29fdf