Closed
Bug 1194259
Opened 8 years ago
Closed 8 years ago
Make ICE IP restriction to default routes work in E10S
Categories
(Core :: WebRTC: Networking, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla46
backlog | webrtc/webaudio+ |
People
(Reporter: ekr, Assigned: jesup)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
Bug 1189041 doesn't work on E10S. That needs to be fixed.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → ekr
![]() |
||
Updated•8 years ago
|
tracking-e10s:
--- → +
Reporter | ||
Comment 1•8 years ago
|
||
Bug 1194259 - Make ICE IP restriction to default routes work in E10S
Reporter | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Comment 3•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Attachment #8648865 -
Flags: review+
Assignee | ||
Comment 4•8 years ago
|
||
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
Assignee | ||
Comment 5•8 years ago
|
||
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+
Comment 6•8 years ago
|
||
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)
Reporter | ||
Comment 7•8 years ago
|
||
You should probably find someone else to finish it.
Flags: needinfo?(ekr)
Updated•8 years ago
|
Assignee: ekr → drno
Comment 8•8 years ago
|
||
Bug 1194259 - Make ICE IP restriction to default routes work in E10S
Comment 9•8 years ago
|
||
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/
Updated•8 years ago
|
Rank: 15 → 7
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
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.
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31573/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31573/
Attachment #8709834 -
Flags: review?(wmccloskey)
Attachment #8709834 -
Flags: review?(rjesup)
Attachment #8709834 -
Flags: review?(mcmanus)
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31575/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31575/
Attachment #8709835 -
Flags: review?(mcmanus)
Attachment #8709835 -
Flags: review?(drno)
Assignee | ||
Comment 14•8 years ago
|
||
Patrick - please feel free to forward these review requests as appropriate (dragana, jduell, jdm, ...)
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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 17•8 years ago
|
||
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+
Assignee | ||
Comment 18•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8709834 -
Flags: review?(wmccloskey) → review?(rjesup)
Comment 19•8 years ago
|
||
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.
Assignee | ||
Comment 20•8 years ago
|
||
I hate mozreview sometimes: Flags: review?(wmccloskey@mozilla.com) → review?(rjesup@jesup.org) Patrick: any thoughts about comment 19?
Flags: needinfo?(mcmanus)
Comment 21•8 years ago
|
||
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 23•8 years ago
|
||
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)
Comment 24•8 years ago
|
||
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)
Assignee | ||
Comment 25•8 years ago
|
||
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+
Assignee | ||
Comment 26•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Attachment #8648865 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8687512 -
Attachment is obsolete: true
Assignee | ||
Comment 27•8 years ago
|
||
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)
Comment 32•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5c372359d5ba https://hg.mozilla.org/mozilla-central/rev/0dafc2f29765 https://hg.mozilla.org/mozilla-central/rev/542c5ae29fdf
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•