1143827 - Remove Mozilla's default STUN server

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

As pointed out here https://bugzilla.mozilla.org/show_bug.cgi?id=891551#c181 Mozilla's STUN server does not support TCP and therefore won't allow Firefox to gather TCP server reflexive candidates once bug 891551 lands.

Furthermore it appears that stun.services.mozilla.com is not working 100% reliable and we tend to no trust it (any more).

And finally: do Firefox users gain anything from having a default STUN server, but no TURN relay?

I think we should remove this line https://dxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#331 and let WebRTC web services handle STUN and TURN as needed.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

If we're going to remove it then we should probably warn users a couple of months out, or we might break a few WebRTC demos.

Comment 2

[Eric Rescorla (:ekr)]

That seems surprising, since those demos will already not work with Chrome.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Good point, it would only affect demos specifically written for Firefox. Probably not that many of those.

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: bug_1143827_remove_mozilla_stun.patch

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=105c0333dce6

Comment 5

[Nils Ohlmeier [:drno]]

Adam you mentioned traffic stats from our stun server the other day, so I thought you might have input on this topic :-)

Comment 6

[Nils Ohlmeier [:drno]]

Attachment: bug_1143827_remove_mozilla_stun.patch

It helps to have a working local build before pushing things to try...

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e7e8ea6bcfb

Comment 7

[Adam Roach [:abr]]

Based on the assumption that Chrome does not have a default server, I think we should be okay removing the default server.

That said, this patch does significantly more than that: the current code allows pref settings to override the servers specified by the webapp, and this patch takes that functionality away. I've used this in testing certain problems in the past, and don't see a reason to remove it. Would there be any objection to scaling back the change here so that all it does is reduce the set of ice servers in all.js to "[]"?

There's a different conversation to be had around the disposition of the stun.services.mozilla.com server, but that's not the subject of this bug.

Comment 8

[Nils Ohlmeier [:drno]]

Attachment: bug_1143827_remove_default_stun_server.patch

I overlooked that feature when I got into deleting code mode :-)

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26594afc0b8a

Comment 9

[Adam Roach [:abr]]

Comment on attachment 8590409 [details] [diff] [review]
bug_1143827_remove_default_stun_server.patch

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

Looks good to me. I think a strict reading of module ownership rules would have bsmedberg (as the sole peer on https://wiki.mozilla.org/Modules/All#Preferences) signing off on changes under modules/libpref. Tagging him for proper i-dotting and t-crossing, although I expect him to rubberstamp my r+.

Comment 10

[Benjamin Smedberg]

Comment on attachment 8590409 [details] [diff] [review]
bug_1143827_remove_default_stun_server.patch

Yeah I own the code, not the pref values.

Comment 11

[Carsten Book [:Tomcat]]

sorry seems the patch does not apply:

Fetching... done
Parsing... done
adding 1143827 to series file
renamed 1143827 -> bug_1143827_remove_default_stun_server.patch
applying bug_1143827_remove_default_stun_server.patch
patching file browser/components/loop/test/functional/config.py
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/components/loop/test/functional/config.py.rej
patching file dom/media/tests/mochitest/head.js
Hunk #1 FAILED at 129
1 out of 1 hunks FAILED -- saving rejects to file dom/media/tests/mochitest/head.js.rej
patching file modules/libpref/init/all.js
Hunk #1 FAILED at 369
1 out of 1 hunks FAILED -- saving rejects to file modules/libpref/init/all.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh bug_1143827_remove_default_stun_server.patch

could you take a look, thanks!

Comment 12

[Nils Ohlmeier [:drno]]

Attachment: bug_1143827_remove_default_stun_server.patch

Sorry about that, I should have considered bit-rotting here...

Refreshed the patch. Carrying forward r+=abr,bsmedberg

Try run to be safe: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37f87195cfda

Comment 13

[Nils Ohlmeier [:drno]]

I don't see any problems in our tests.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed5bbd4ce3cf

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed5bbd4ce3cf

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed5bbd4ce3cf

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This breaks http://mozilla.github.io/webrtc-landing/pc_test.html

It also breaks all my jsfiddles, like this one http://jsfiddle.net/aynr0k5q which I link to from answers on stackoverflow and other places.

JSFiddle lets me update them, so that's not a problem for me, but presumably this wont just affect me. Shouldn't we warn people that we've done this? It just looks like things stop working otherwise.

Comment 18

[Eric Rescorla (:ekr)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #17)
> This breaks http://mozilla.github.io/webrtc-landing/pc_test.html
> 
> It also breaks all my jsfiddles, like this one http://jsfiddle.net/aynr0k5q
> which I link to from answers on stackoverflow and other places.
> 
> JSFiddle lets me update them, so that's not a problem for me, but presumably
> this wont just affect me. Shouldn't we warn people that we've done this? It
> just looks like things stop working otherwise.

Something like this, perhaps?

https://groups.google.com/forum/#!topic/mozilla.dev.media/LxKWH8g0HXE

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I was thinking more like Hacks, though it may not warrant a post just on its own.

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry, pilot error on my part. Does not break webrtc-landing after all.

As a test I cleared media.peerconnection.default_iceservers rather than setting it to [] like the patch does. Clearing it causes mozRTCPeerConnection to throw NS_ERROR_UNEXPECTED, which probably shouldn't happen, but is not urgent.

Comment 21

[Nils Ohlmeier [:drno]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #20)
> Sorry, pilot error on my part. Does not break webrtc-landing after all.

I was already scratching my head how this possibly could break simple demos.

I added this bug to the 41 webrtc release notes as further heads up.
 
> As a test I cleared media.peerconnection.default_iceservers rather than
> setting it to [] like the patch does. Clearing it causes
> mozRTCPeerConnection to throw NS_ERROR_UNEXPECTED, which probably shouldn't
> happen, but is not urgent.

I filled bug 1167922 as a follow up.

Comment 22

[Eric Shepherd [:sheppy]]

Updated https://developer.mozilla.org/en-US/Firefox/Releases/41#WebRTC to note this change.