Closed Bug 1189041 Opened 9 years ago Closed 9 years ago

Add about:config pref to limit ICE discovery to default route

Categories

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

Product:
Core
Component:
WebRTC: Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox40 --- wontfix
firefox41 --- wontfix
firefox42 --- fixed
firefox43 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

WIP patch
23.93 KB, patch
Details | Diff | Splinter Review
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.
40 bytes, text/x-review-board-request
bwc
: review+
Sylvestre
: approval-mozilla-aurora+
Details
WIP patch for alternative suppression of non-default-routes
19.29 KB, patch
Details | Diff | Splinter Review
update to ekr's patch to resolve issue with ptr to stack array
2.71 KB, patch
Details | Diff | Splinter Review 
To improve privacy in the "split-VPN" case, where the real external address is still accessible (bypassing the VPN), we should provide a config pref to limit ICE checks to the default route/routes (IPV6?).  Note that there may be issues determining the default route..

This behavior may be the default in private browsing mode; if so we should consider if it should be possible to override this limit in that mode.
backlog: --- → webRTC+
Rank: 5
Priority: -- → P1
Blocks: 1189167
Assignee: nobody → rjesup
Attached patch WIP patch — — Splinter Review 
Untested WIP patch for nICEr half of this.
Comment on attachment 8641710 [details] [diff] [review]
WIP patch

Review of attachment 8641710 [details] [diff] [review]:
-----------------------------------------------------------------

I am on a plane with apparently limited UDP so I am having trouble testing this. Will do it when I get on the ground.
Byron,

Would like to get your opinion about approaches. This patch filters the candidate
list against the "default" address, determined by trying to connect to something
we know is public. The alternative approach in Jesup's patch is to bind to
INADDR_ANY. I had considered and rejected that approach because I know there
are places where we compare stuff to the address we bound to, as in:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c?from=nr_ice_candidate_pair_stun_cb&case=true#262

So I was worried about side effects. What do you think?
Flags: needinfo?(docfaraday)
One more note: I think it's pretty likely that if you have two nodes behind the same NAT and only one has this policy that the INADDR_ANY version ends up with prflx candidates because the "mapped" address != INADDR_ANY
(In reply to Eric Rescorla (:ekr) from comment #3)
> Byron,
> 
> Would like to get your opinion about approaches. This patch filters the
> candidate
> list against the "default" address, determined by trying to connect to
> something
> we know is public. The alternative approach in Jesup's patch is to bind to
> INADDR_ANY. I had considered and rejected that approach because I know there
> are places where we compare stuff to the address we bound to, as in:
> 
> https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/
> nICEr/src/ice/ice_candidate_pair.
> c?from=nr_ice_candidate_pair_stun_cb&case=true#262
> 
> So I was worried about side effects. What do you think?

   Binding to INADDR_ANY is definitely going to violate a bunch of assumptions. It is also unclear to me what host candidates we trickle in this case (none? only the host candidates that end up routing to a remote candidate?). If we just pick one as you're proposing, that reduces the risk that the other end could bamboozle us into revealing additional addresses by sending carefully chosen candidates (I don't know how risky this is for those using VPN to anonymize). On the flipside, it would likely yield sub-optimal behavior in cases where someone is using a VPN to connect to a work network (ICE with an agent at work would not use the VPN).
Flags: needinfo?(docfaraday)
Well, I think that any default-route-only approach needs to block HOST entries, right?  (And it would be bad to expose any with a real IP, and INADDR_ANY ones would be 0.0.0.0/::0 in any case and so not useful to the far end.)

If the other side provides HOST entries, we can try to access them (from INADDR_ANY at our end) which will use default routing.  

Right now my patch implements roughly something similar to Chrome - instead of getifaddrs(), I instead return an INADDR_ANY IP4 (and maybe an equivalent IPV6) address, and then later I filter out any candidates with an ANY address (to avoid sending HOST 0.0.0.0/etc candidates).  The SRFLX will have an raddr of 0.0.0.0 right now (and IIRC I see the same in Chrome).
raddr isn't used for anything, but I opted to use the same address for raddr and srflx
One minor concern I had with ekr's patch was that 8.8.8.8 *might* (ok, slight stretch) be static-routed to the real external interface and bypass the VPN if they're using it for DNSSEC (and thus think it's safe). I think at minimum we should select something that would never be static-routed for any reason to find the default interface.

My patch is likely smaller/simpler, but if it's violating assumptions it may not be better.  Worth looking a bit more at though; make sure we find the best solution (and keep maintenance in mind too).

Almost makes me with we could do some magic with TTL 1 or 2 packets to probe for a local connection without exposing to the outside world.... Sigh.  And probably not possible on Windows IIRC.
Comment on attachment 8641710 [details] [diff] [review]
WIP patch

Review of attachment 8641710 [details] [diff] [review]:
-----------------------------------------------------------------

I am on a plane with apparently limited UDP so I am having trouble testing this. Will do it when I get on the ground.

Note to self: need to also add filtering logic for host in the call to trickle_cb
Comment on attachment 8641878 [details] [diff] [review]
WIP patch for alternative suppression of non-default-routes

Review of attachment 8641878 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
@@ +498,5 @@
>      if (cand->state == NR_ICE_CAND_STATE_INITIALIZED) {
>        int was_pruned = 0;
> +#define SUPPRESS_WILDCARD_HOSTS
> +#if defined(SUPPRESS_WILDCARD_HOSTS)
> +// XXX NOTE: this also *appears* to suppress SRFLX, hmmm

You can't remove host candidates from the internal list of candidates because
you don't do checks on local srflx and prflx candidates. See:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c?from=nr_ice_component_pair_candidate&case=true#975

I believe that the right implementation strategy here is to filter the candidates
that you pass to the outside, not to remove them internally.
Bug 1189041 - Add option to only gather addresses for default route.
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

This is just a WIP and probably is bug-infested, but shows the direction I'm going. Byron PTAL and LMK if you think this is the right idea.
Attachment #8642204 - Flags: feedback?(docfaraday)
https://reviewboard.mozilla.org/r/14711/#review13277

I think this is the way to go here. One refinement I can think of is to be willing to gather/trickle additional srflx based on the default route to stuff that is trickled at us, but I suspect this would open up the possibility of someone tricking fake candidates at us in order to learn about other routes (maybe a long shot, but possible). There's a similar refinement where we'd get the route to things like the STUN/TURN server and use that, but again this could be abused I think.
Attachment #8642204 - Flags: feedback?(docfaraday) → feedback+
https://reviewboard.mozilla.org/r/14709/#review13311

feedback+

::: media/mtransport/test/ice_unittest.cpp:259
(Diff revision 1)
> -                                enable_tcp)),
> +                                enable_tcp, allow_link_local, hide_non_default)),

I take it there was another issue with allow_link_local not getting passed down?

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:944
(Diff revision 1)
> +      &cand->addr : &cand->base;

So question: do "unusual" values here provide a hint to the JS code that the user has set a special privacy mode?  Likely so, however, the JS could infer that from no HOST candidates being generated.  

This ability to notice use of default-host routing makes it hard to use this feature without at least raising a red flag to the JS that the user is trying to hide.  While the JS may not be able to break it, it can flag all the fingerprint info for the user for heightened de-anonymization.

Possible mitigations (likely imperfect):
a) provide a fake HOST entry on a LAN address NOT in the list of routable addresses.  Choose an unused 192.168 subnet for example.
We'll likely have to remember this address and may need to *appear* to be trying to ICE open the address, depedning on what info is exposed about candidates to the JS.  Use this address for raddr as well.
Note that with some smart work, this could be sniffed out, but it would not be obvious or trivial.
b) ?

All that said: this is better than no ability to retrict routing, definitely, but maybe we can improve the mimicry of "normal" mode.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:574
(Diff revision 1)
> +        if ((r=nr_str_port_to_transport_addr("8.8.8.8", 53, IPPROTO_UDP, &remote_addr)))

Still concerned that 8.8.8.8 *might* be static-routed (pretty unlikely, but maybe possible, or people might be social-engineeered into doing so).  Perhaps there's a better  never-would-be-static-routed address.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:580
(Diff revision 1)
> +        if ((r=nr_str_port_to_transport_addr("2001:4860:4860::8888", 53, IPPROTO_UDP, &remote_addr)))

Similar concern

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:563
(Diff revision 1)
> +static int nr_ice_get_default_address(nr_ice_ctx *ctx, int ip_version, nr_transport_addr* addrp)

The other option here is to do like Chrome and use INADDR_ANY/in6addr_any to find the default interfaces - which per comments from ekr might require some tweaks elsewhere, though it works fine for me in practice.
Attachment #8642204 - Flags: feedback+
Updated to use some bits from ekr's patch to hide HOSTs; works better now (SRFLX is offered with external IP of default route).  May be moot, but might as well keep the option up-to-date
Attachment #8641878 - Attachment is obsolete: true
FYI, you may need to tweak gonk_addrs.c as well; that could be a followup bug, if needed
https://reviewboard.mozilla.org/r/14711/#review13521

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:654
(Diff revision 1)
> +        addrs = default_addrs;

Bug - default_addrs will be out-of-scope a couple of lines below.... so addrs will be pointing at stack which may be overwritten at any point.   If a separate array is to be used, it must be global to the function, or copied back over the original (using nr_local_addr_copy, since there are internal pointers)
Note: two full-sized arrays is overkill for this patch, but for my patch for whitelists at least in theory it could have any number (though in practice likely just two).  two full-sized seems safer, and it's just stack space
Attachment #8643486 - Flags: review?(ekr)
(In reply to Randell Jesup [:jesup] from comment #18)
> https://reviewboard.mozilla.org/r/14711/#review13521
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:654
> (Diff revision 1)
> > +        addrs = default_addrs;
> 
> Bug - default_addrs will be out-of-scope a couple of lines below.... so
> addrs will be pointing at stack which may be overwritten at any point.   If
> a separate array is to be used, it must be global to the function, or copied
> back over the original (using nr_local_addr_copy, since there are internal
> pointers)

Yes, that's a good point. I'd just make it function-global.
Yeah, the patch I attached with r? to you does so.  I have a rebase of the force-interface patch on top of that that uses the function-global arrays; we'll want to land in that order (this first, then the force-interface patch).
Attachment #8642204 - Flags: feedback+
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

Bug 1189041 - Add option to only gather addresses for default route.
https://reviewboard.mozilla.org/r/14709/#review13809

::: media/mtransport/nr_socket_prsock.cpp:578
(Diff revision 2)
>      }
>  
> -    if((r=nr_praddr_to_transport_addr(&naddr,&my_addr_,addr->protocol,1)))
> +    if((r=nr_praddr_to_transport_addr(&naddr,&my_addr_,my_addr_.protocol,1)))
>        ABORT(r);

This is unnecessary.

::: media/mtransport/test/ice_unittest.cpp:2053
(Diff revision 2)
> +TEST_F(IceConnectTest, DISABLED_TestConnectDefaultRouteOnly) {
> +  Init(false, false, false, true);
> +  AddStream("first", 1);
> +  ASSERT_TRUE(Gather());
> +  SetExpectedTypes(NrIceCandidate::Type::ICE_SERVER_REFLEXIVE,
> +    NrIceCandidate::Type::ICE_SERVER_REFLEXIVE, kNrIceTransportTcp);
> +  Connect();
> +}
> +

Comment that this doesn't always work b/c of hairpinning.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:556
(Diff revision 2)
>  
> -int nr_ice_gather(nr_ice_ctx *ctx, NR_async_cb done_cb, void *cb_arg)
> +   IPv4: 8.8.8.8
> +   IPv6: 2001:4860:4860::8888
> +
> +   Then we can do getsockname to get the address. No packets get sent

TODO: find a better external address.
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

Bug 1189041 - Add option to only gather addresses for default route.
Attached file MozReview Request: Minor tweaks (obsolete) — 
Minor tweaks
This is out to drno for testing.

try push at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=825be9b0dd2b
Flags: needinfo?(drno)
Jesup, you had asked for another external IP. Perhaps one of ours?
Flags: needinfo?(rjesup)
Seems to work as expected, from what I can tell.

With default_address_only set to false my dual homes machine connects to the remote machine over its secondary interface, which is on the same LAN as the remote machine and has not the default route set. And I see the IPs of both interfaces show up in about:webrtc for ICE candidates.
If I flip default_address_only to true and make a new call, the traffic gets routed over the primary interface with the default route and I only see ICE candidates with the IP address of the primary, default route interface in about:webrtc.

Note: I experienced some crashes with e10s on, but did not check if they are related to this new code, or something else.
Flags: needinfo?(drno)
Looks like the crashes I saw are in fact related to this code here:

#02: nr_socket_local_connect(void*, nr_transport_addr_*)[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x13b2057]
#03: nr_socket_connect[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47d7018]
#04: nr_ice_get_default_address[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47ce894]
#05: nr_ice_get_default_local_address[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47ce2b3]
#06: nr_ice_get_local_addresses[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47cd785]
#07: nr_ice_gather[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47cd4bd]
#08: mozilla::NrIceCtx::StartGathering()[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x13c07e0]
#09: mozilla::PeerConnectionMedia::EnsureIceGathering_s()[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x135b511]
Previous stack trace was missing crucial information:

Assertion failure: false, at /Users/nohlmeier/src/mozilla-central/media/mtransport/nr_socket_prsock.cpp:1301
[Child 69446] WARNING: Transparent content with displayports can be expensive.: file /Users/nohlmeier/src/mozilla-central/layout/base/nsDisplayList.cpp, line 1642
[Child 69446] WARNING: Transparent content with displayports can be expensive.: file /Users/nohlmeier/src/mozilla-central/layout/base/nsDisplayList.cpp, line 1642
#01: mozilla::NrSocketIpc::connect(nr_transport_addr_*)[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x13b1a68]
#02: nr_socket_local_connect(void*, nr_transport_addr_*)[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x13b2057]
#03: nr_socket_connect[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47d7018]
#04: nr_ice_get_default_address[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47ce894]
#05: nr_ice_get_default_local_address[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47ce2b3]
#06: nr_ice_get_local_addresses[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47cd785]
#07: nr_ice_gather[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x47cd4bd]
#08: mozilla::NrIceCtx::StartGathering()[/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin14.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x13c07e0]
https://reviewboard.mozilla.org/r/14709/#review13837

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:590
(Diff revision 3)
> +    if ((r=nr_socket_connect(sock, &remote_addr)))

connect() is not implemented for NrSocketIpc and therefor results in trouble with e10s.
(In reply to Eric Rescorla (:ekr) from comment #27)
> Jesup, you had asked for another external IP. Perhaps one of ours?

That could work - though I'd prefer something no one would ever think of static-routing.  It's possible there is no 'better' address than 8.8.8.8.  (4.2.2.2 is another classic 'DNS' address; not from google)
Flags: needinfo?(rjesup)
(In reply to Nils Ohlmeier [:drno] from comment #31)
> https://reviewboard.mozilla.org/r/14709/#review13837
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:590
> (Diff revision 3)
> > +    if ((r=nr_socket_connect(sock, &remote_addr)))
> 
> connect() is not implemented for NrSocketIpc and therefor results in trouble
> with e10s.

OK, I'll take a look tomorrow and see if I can figure out what to do about that.
Awesome. nsIUDPSocket doesn't implement connect.
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

Byron,

I still need to do the E10S side, but since that's a separable piece, I suggest we start the review process on this first.
Attachment #8642204 - Flags: review?(docfaraday)
Attachment #8642204 - Flags: review?(docfaraday)
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

https://reviewboard.mozilla.org/r/14711/#review13907

The most serious thing that I see is that some tests in ice_unittests seem to be expecting local host candidates to be exposed by the same code that feeds the stats API. Also, I don't see us doing anything to prevent local host candidates from being exposed by the default candidate stuff (nr_ice_media_stream_get_default_candidate).

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoDefaultRoute.html:10
(Diff revision 3)
> +    bug: "796890",
> +    title: "Basic audio/video (separate) peer connection"

Update this.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoDefaultRoute.html:36
(Diff revision 3)
> +       );

Fixup ws.

::: media/mtransport/nr_socket_prsock.cpp:737
(Diff revision 3)
> -  int32_t status;
> +  int32_t status, status2;

If we're going to have more than one of these, let's name them something like connect_status and getsockname_status.

::: media/mtransport/nricectx.h:212
(Diff revision 3)
>                                   bool offerer,
>                                   bool set_interface_priorities = true,
>                                   bool allow_loopback = false,
>                                   bool tcp_enabled = true,
> -                                 bool allow_link_local = false);
> +                                 bool allow_link_local = false,
> +                                 bool hide_non_default = false);

This arg list of bools is getting pretty long. We should probably put a TODO here for switching this over to using an init struct or flags or something.

::: media/mtransport/test/ice_unittest.cpp:255
(Diff revision 3)
> -              bool allow_loopback = false, bool enable_tcp = true) :
> +              bool allow_loopback = false, bool enable_tcp = true,
> +              bool allow_link_local = false, bool hide_non_default = false) :

Same comment regarding length of init list.

::: media/mtransport/test/ice_unittest.cpp:1249
(Diff revision 3)
> +  void DumpCandidates(unsigned int stream) {

I've filed bug 1192867 since you can't make this const correct without modifying a bunch of other stuff.

::: media/mtransport/test/ice_unittest.cpp:1333
(Diff revision 3)
>      } else {
> +      if (setupStunServers) {

Let's just merge this into an else if.

::: media/mtransport/test/ice_unittest.cpp:1970
(Diff revision 3)
> +// apparently NATted. Result should be a srflx candidate

This is simulating non-NATed, right?

::: media/mtransport/test/ice_unittest.cpp:1965
(Diff revision 3)
> +  ASSERT_FALSE(StreamHasMatchingCandidate(0, "host"));
> +  ASSERT_TRUE(StreamHasMatchingCandidate(0, "srflx"));

This does not square with the comment up top.

::: media/mtransport/test/ice_unittest.cpp:2170
(Diff revision 3)
> +  SetExpectedTypes(NrIceCandidate::Type::ICE_HOST,

I would not expect local host candidates to percolate up, since that's the same code-path that the stats API uses.

::: media/mtransport/test/ice_unittest.cpp:1956
(Diff revision 3)
> +// apparently non-NATted. Result should be a host candidate

We're trying to simulate NATed here, right? Also, shouldn't local host candidates not percolate up?

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:583
(Diff revision 3)
> +    default:
> +      assert(0);
> +      ABORT(R_INTERNAL);

Indent.
https://reviewboard.mozilla.org/r/14711/#review13931

::: media/mtransport/test/ice_unittest.cpp:1965
(Diff revision 3)
> +  ASSERT_FALSE(StreamHasMatchingCandidate(0, "host"));
> +  ASSERT_TRUE(StreamHasMatchingCandidate(0, "srflx"));

Yep. Comment is wrong.

::: media/mtransport/test/ice_unittest.cpp:1956
(Diff revision 3)
> +// apparently non-NATted. Result should be a host candidate

Oh, yeah. Will swap the comments.

::: media/mtransport/test/ice_unittest.cpp:2170
(Diff revision 3)
> +  SetExpectedTypes(NrIceCandidate::Type::ICE_HOST,

The situation here is that always do checks from a HOST candidate, and then you infer from the response whether the HOST or SRFLX succeeded. So, if you aren't NATted, you get success on the HOST.
https://reviewboard.mozilla.org/r/14711/#review13907

Oh, yeah, good catch about default candidate. I can fix that.

Can you explain the first point a bit more. I don't quite follow.

> This arg list of bools is getting pretty long. We should probably put a TODO here for switching this over to using an init struct or flags or something.

Yeah, I had the same thought as I was working on this.
https://reviewboard.mozilla.org/r/14711/#review13907

Oh, GetActivePair is used only by ice_unittest, so I guess this doesn't have implications for the stats API. Although this implies that the stats API will likely hand back a list of candidate pairs, none of which are selected, even though ICE succeeded. That might pose problems for the mochitest if/when it is ever enabled. Not sure what is to be done about that.
Yeah, we may need to clean up the stats API for this....
[Tracking Requested - why for this release]:
Plan is to uplift this into 41 to support an extension that can mitigate these issues ASAP (see mail to drivers)
status-firefox40: --- → wontfix
status-firefox41: --- → affected
tracking-firefox41: --- → ?
Gah, I can't seem to get this patch published.
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

Bug 1189041 - Add option to only gather addresses for default route.

Minor tweaks

Checkpoint: Byron's minor comments

Suppress host default candidate
Attachment #8645182 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/14711/#review14083

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoDefaultRoute.html:29
(Diff revisions 2 - 4)
> -  }, function() {
> +,function() {
> -    var test = new PeerConnectionTest(options);
> +   var test = new PeerConnectionTest(options);
> -    test.setMediaConstraints([{audio: true}, {video: true}],
> +   test.setMediaConstraints([{audio: true}, {video: true}],
> -      [{audio: true}, {video: true}]);
> +     [{audio: true}, {video: true}]);
> -    test.chain.insertAfter("PC_LOCAL_SETUP_ICE_HANDLER", PC_LOCAL_NO_HOST_CANDIDATES);
> +   test.chain.insertAfter("PC_LOCAL_SETUP_ICE_HANDLER", PC_LOCAL_NO_HOST_CANDIDATES);
> -    test.run();
> +   test.run();
> -   }
> -       );
> -});
> +  });
> +});

This doesn't look right to me. It needs to be something like:

pushPrefEnv({'set':[["foo", true]]}, function() { ... });
This test is commented out. let's remove it.
Ugh. Try push failed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e253f729813d contains alleged fix. Will update patch if it works.
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

Bug 1189041 - Add option to only gather addresses for default route.

Minor tweaks

Checkpoint: Byron's minor comments

Suppress host default candidate

Fix thread issue

Remove non-used mochitest
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

Bug 1189041 - Add option to only gather addresses for default route.

Minor tweaks

Checkpoint: Byron's minor comments

Suppress host default candidate

Fix thread issue

Remove non-used mochitest
Attachment #8643486 - Flags: review?(ekr)
Attachment #8642204 - Flags: review?(docfaraday)
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

https://reviewboard.mozilla.org/r/14711/#review14171
Attachment #8642204 - Flags: review?(docfaraday) → review+
Whiteboard: checkin-needed
Clearing checkin-needed while I discuss E10S with :drno.

:drno, can you advise on how non-debug builds behave on E10S. I would expect that
all calls fail because there are no candidates.
Flags: needinfo?(drno)
Whiteboard: checkin-needed
Confirmed. It works without e10s. With e10s it hits the assertion in connect() on a debug build and on a non-debug build it keeps running but never produces local ICE candidates.
Flags: needinfo?(drno)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f0f45233e6b
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Tracked as this work is planned to ship in FF41.
tracking-firefox41: ? → +
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Inability to hide LAN addresses and non-default external addresses (i.e. to limit traffic to a VPN).  Important for privacy/user-security for some users, and to limit some forms of tracking.

[Describe test coverage new/current, TreeHerder]: C++ unit tests.  Note: followon patch will enable this in e10s, but that will ride the train in 43.

[Risks and why]: lower risk and only should affect operation if the pref is set (risk would be if the pref is set and there's some problem with interface discovery); but only asking for Aurora/42 uplift.

[String/UUID change made/needed]: none
Attachment #8642204 - Flags: approval-mozilla-aurora?
Comment on attachment 8642204 [details]
MozReview Request: Bug 1189041 - Add option to only gather addresses for default route.

Approving now to increase the testing coverage. Thanks
Attachment #8642204 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Per previous discussions, we decided not to try to push this into 41, and hadn't set the tracking flags to match
status-firefox41: affected → wontfix
tracking-firefox41: + → ---
Depends on: 1304600
No longer depends on: 1304600
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: