Make ICE IP restriction to default routes work in E10S

RESOLVED FIXED in Firefox 46

Status

()

defect
P1
normal
Rank:
7
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ekr, Assigned: jesup)

Tracking

(Blocks 1 bug)

38 Branch
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Bug 1189041 doesn't work on E10S. That needs to be fixed.
Assignee: nobody → ekr
Blocks: 1189167
tracking-e10s: --- → +
Bug 1194259 - Make ICE IP restriction to default routes work in E10S
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.
Attachment #8648865 - Flags: feedback?(rjesup)
Attachment #8648865 - Flags: feedback?(jduell.mcbugs)
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
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.
Attachment #8648865 - Flags: feedback?(jduell.mcbugs) → feedback+
Attachment #8648865 - Flags: review+
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 on attachment 8648865 [details]
MozReview Request: Bug 1194259 - Make ICE IP restriction to default routes work in E10S

It didn't of course
Attachment #8648865 - Flags: review+
Attachment #8648865 - Flags: feedback?(rjesup)
Attachment #8648865 - Flags: feedback+
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.
Flags: needinfo?(ekr)
You should probably find someone else to finish it.
Flags: needinfo?(ekr)
Assignee: ekr → 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/
Rank: 15 → 7
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?
Assignee: drno → rjesup
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.
Patrick - please feel free to forward these review requests as appropriate (dragana, jduell, jdm, ...)
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
Attachment #8709835 - Flags: review?(mcmanus) → review+
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..
Attachment #8709834 - Flags: review?(mcmanus) → review+
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
Attachment #8709835 - Flags: review?(drno) → review+
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
Attachment #8709834 - Flags: review?(rjesup)
Attachment #8709834 - Flags: review?(wmccloskey) → review?(rjesup)
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.
I hate mozreview sometimes:
 Flags: review?(wmccloskey@mozilla.com) → review?(rjesup@jesup.org)

Patrick: any thoughts about comment 19?
Flags: needinfo?(mcmanus)
I'm not really familiar with it.. but maybe jryans
Flags: needinfo?(mcmanus) → needinfo?(jryans)
(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).
Flags: needinfo?(jryans)
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/
Attachment #8687512 - Attachment description: MozReview Request: Bug 1194259 - Make ICE IP restriction to default routes work in E10S → MozReview Request: Bug 1194259: Make ICE IP restriction to default routes work in E10S r?jesup,mcmanus,billm
Attachment #8687512 - Flags: review?(wmccloskey)
Attachment #8687512 - Flags: review?(rjesup)
Attachment #8687512 - Flags: review?(mcmanus)
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?
Flags: needinfo?(jryans)
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.
Attachment #8687512 - Flags: review?(rjesup) → review+
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
Attachment #8687512 - Flags: review?(wmccloskey)
Attachment #8687512 - Flags: review?(mcmanus)
Attachment #8687512 - Flags: review+
Attachment #8648865 - Attachment is obsolete: true
Attachment #8687512 - Attachment is obsolete: true
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....
Attachment #8709834 - Flags: review?(rjesup) → review+
(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.
Flags: needinfo?(jryans)
Depends on: 1242719
See Also: → 1315248
You need to log in before you can comment on or make changes to this bug.