Closed Bug 1143827 Opened 5 years ago Closed 4 years ago
Remove Mozilla's default STUN server
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.
Priority: -- → P2
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.
That seems surprising, since those demos will already not work with Chrome.
Good point, it would only affect demos specifically written for Firefox. Probably not that many of those.
Adam you mentioned traffic stats from our stun server the other day, so I thought you might have input on this topic :-)
It helps to have a working local build before pushing things to try... Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e7e8ea6bcfb
Attachment #8589922 - Attachment is obsolete: true
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.
I overlooked that feature when I got into deleting code mode :-) Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26594afc0b8a
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 on attachment 8590409 [details] [diff] [review] bug_1143827_remove_default_stun_server.patch Yeah I own the code, not the pref values.
Attachment #8590409 - Flags: review?(benjamin) → review+
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!
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
I don't see any problems in our tests.
Assignee: nobody → drno
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.
(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
I was thinking more like Hacks, though it may not warrant a post just on its own.
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.
(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.
Updated https://developer.mozilla.org/en-US/Firefox/Releases/41#WebRTC to note this change.
You need to log in before you can comment on or make changes to this bug.