Closed Bug 1266667 Opened 8 years ago Closed 7 years ago

[e10s] active ICE TCP fails because multiple connections with identical TCP SRC port fail

Categories

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

Product:
Core
Component:
WebRTC: Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
e10s - ---
firefox48 --- affected
firefox53 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: drno, Assigned: drno)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

mochitest that filters out UDP ICE candidates
2.45 KB, patch
Details | Diff | Splinter Review
Bug 1266667: socket reuse option for TCPSocket.
59 bytes, text/x-review-board-request
mcmanus
: review+
Details
Bug 1266667: added user-pref to force ICE TCP.
59 bytes, text/x-review-board-request
bwc
: review+
Details 
ICE TCP mandates active (=outgoing) candidates, which are the most useful for browsers behind NATs and firewalls.
Our current ICE TCP implementation creates one TCP socket and bind that to one ephemeral TCP port. Then this one port is used to connect to several IP/port combinations on the far end. For non e10s this works just fine.

But with e10s I observed the following:
- The first connection request via SendWindowlessOpenBind() goes from a local 10/8 address and port to a remote 10/8 address and port. This results in Firefox sending TCP SYN's to the remote 10/8 into the local network, which are obviously never gets answered.
- A second connection is requested again via SendWindowlessOpenBind() from the same local 10/8 address and port combination to a publicly routable IP address and port number. This how ever never results in
  - a single SYN packet getting send
  - or an error being delivered to the TCPSocketChild calling SendWindowlessOpenBind()

I verified that re-using the same IP and port number is causing the problem with TCPSocketChild, by changing the local port we feed into the SendWindowlessOpenBind() call into a random port number. This resulted in successful TCP connections getting established.
Without digging into the details here to me this smells like a bug in the e10s TCPSocket or TCPSocketChild implementation.

jdm, mcmanus: what are your thoughts on this?
backlog: --- → webrtc/webaudio+
Rank: 19
Flags: needinfo?(mcmanus)
Flags: needinfo?(josh)
See Also: → 1264351
I don't really have any thoughts on this without an STR to dig into.. but jason has better e10s fu than me..
Flags: needinfo?(mcmanus) → needinfo?(jduell.mcbugs)
The problem description is in a domain with which I am unfamiliar; the best I can do is point you at TCPSocketParent::OpenBind and maybe compare that to what nsServerSocket.cpp does.
Flags: needinfo?(josh)
tracking-e10s: --- → ?
I have an idea what is going on here: we are setting a bunch of socket options in case of non-e10s https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp?from=nr_socket_prsock.cpp#634
I probably need to add a way for the TCPSocketChild to request the same options to be set by the TCPSocketParent.
So after looking at the TCPSocket code for a little bit it looks like I will need to add more options to the |socketOptions| directory in the TCPSocket webidl here: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/TCPSocket.webidl#18
And then pass a new bool like |aUseRealTimeOptions| from the TCPSocketChild to the TCPSocketParent. Then have TCPSocketParent use the Constructor() and pass in |SocketOptions| created/set by TCPSocketParent according to the new flag passed by TCPSocketChild.

Does that sounds like a viable plan, or would you choose a different path?
Flags: needinfo?(josh)
I don't see a need to modify the WebIDL. These seem like changes that could be contained within the IPDL and passed directly to the C++ constructor from TCPSocketParent, rather than via the WebIDL-based Constructor method.
Flags: needinfo?(josh)
Is this a serious enough issue such that it should block e10s rollout?
Flags: needinfo?(jduell.mcbugs) → needinfo?(drno)
The ICE TCP feature is still preffed off via a user preference. So I don't think we need to block on this.
Flags: needinfo?(drno)
This is where the address information for the binding on the local sockets gets passed on: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/network/TCPSocketParent.cpp#190

Which gets us here: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/netwerk/base/nsSocketTransport2.cpp#2456

Which gets use here: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/netwerk/base/nsSocketTransport2.cpp#1425

So my current guess is that we should try to extend the Send|RecvOpenBind function with a new option, which then sets some special options in the nsSocketTransport to set PR_SockOpt_Reuseaddr and PR_SockOpt_Reuseport (PR_SockOpt_NoDelay is apparently already been taken care of).
Comment on attachment 8810909 [details]
Bug 1266667 - ice tcp plus pref for testing that blocks udp candidates

@mcmanus: does the part touching nsSocketTransport look sane to you?

@jdm: does the part touching the TCPSocket parts look sane to you?
Attachment #8810909 - Flags: feedback?(mcmanus)
Attachment #8810909 - Flags: feedback?(josh)
seems fine.. not sure you have to call it realtimeoptions instead of what it really is - reuseaddr options.. but its not a big deal to me
Attachment #8810909 - Flags: feedback?(mcmanus) → feedback+
Comment on attachment 8810909 [details]
Bug 1266667 - ice tcp plus pref for testing that blocks udp candidates

Yep, this looks like the right way to go about it.
Attachment #8810909 - Flags: feedback?(josh) → feedback+
Testing this from the WebRTC tests turns out to be very hard, as we don't have server/listening support yet.

:jdm would this http://searchfox.org/mozilla-central/source/dom/network/tests/test_tcpsocket_client_and_server_basics.js be an appropriate place to test this (or what would you suggest instead in case it's not)?

The basic test would be to open two server sockets and one client socket with the new option. And then try to make connections to both server sockets from one client socket.
Flags: needinfo?(josh)
That seems fine, although I guess that will require making the new option available through WebIDL.
Flags: needinfo?(josh)
Exposing these options through WebIDL sounds too dangerous just for the purposes of testing it.
Attachment #8810909 - Attachment is obsolete: true
(In reply to Nils Ohlmeier [:drno] from comment #18)
> Exposing these options through WebIDL sounds too dangerous just for the
> purposes of testing it.

+1
Comment on attachment 8821048 [details]
Bug 1266667: socket reuse option for TCPSocket.

https://reviewboard.mozilla.org/r/100426/#review101350

lgtm
Attachment #8821048 - Flags: review?(mcmanus) → review+
Comment on attachment 8821049 [details]
Bug 1266667: added user-pref to force ICE TCP.

https://reviewboard.mozilla.org/r/100428/#review101738

Add an entry in all.js, I think, unless you want it hidden
Attachment #8821049 - Flags: review+
Comment on attachment 8821049 [details]
Bug 1266667: added user-pref to force ICE TCP.

https://reviewboard.mozilla.org/r/100428/#review101738

Obviously, that's a drive-by review, but this is all pretty straightforward
Comment on attachment 8821049 [details]
Bug 1266667: added user-pref to force ICE TCP.

https://reviewboard.mozilla.org/r/100428/#review101914

It seems to me that simply filtering remote candidates for "UDP" would do the trick here, if the goal is to have a mochitest that disables UDP ICE without using the NAT simulator. Am I mistaken here?
Attachment #8821049 - Flags: review?(docfaraday) → review-
Comment on attachment 8821049 [details]
Bug 1266667: added user-pref to force ICE TCP.

https://reviewboard.mozilla.org/r/100428/#review101914

The problem here is that we can't use mochitest to test this as we don't have TCP listening capabilities with e10s. So we rely on using public services for testing. But with these we don't have an easy way to drop the UDP candidates. So that's why a user pref would be really helpful for dev and testing purposes.
Comment on attachment 8821049 [details]
Bug 1266667: added user-pref to force ICE TCP.

https://reviewboard.mozilla.org/r/100428/#review102256

The remote SDP needs to be filtered too. You could do some remote candidate filtering down in PeerConnectionMedia::ActivateOrRemoveTransports as well.
Attachment #8821049 - Flags: review?(docfaraday) → review-
Comment on attachment 8821049 [details]
Bug 1266667: added user-pref to force ICE TCP.

https://reviewboard.mozilla.org/r/100428/#review103918
Attachment #8821049 - Flags: review?(docfaraday) → review+
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/1e6818c1c8b7
socket reuse option for TCPSocket. r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/fea65d4dad89
added user-pref to force ICE TCP. r=bwc,jesup
https://hg.mozilla.org/mozilla-central/rev/1e6818c1c8b7
https://hg.mozilla.org/mozilla-central/rev/fea65d4dad89
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: