Closed Bug 1143827 Opened 5 years ago Closed 4 years ago

Remove Mozilla's default STUN server

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed
Blocking Flags:

People

(Reporter: drno, Assigned: drno)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

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.
See Also: → 891551
Rank: 25
Flags: firefox-backlog+
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 :-)
Flags: needinfo?(adam)
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.
Flags: needinfo?(adam)
I overlooked that feature when I got into deleting code mode :-)

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26594afc0b8a
Attachment #8590007 - Attachment is obsolete: true
Attachment #8590409 - Flags: review?(adam)
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+.
Attachment #8590409 - Flags: review?(benjamin)
Attachment #8590409 - Flags: review?(adam)
Attachment #8590409 - Flags: review+
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+
backlog: --- → webRTC+
Flags: firefox-backlog+
Keywords: checkin-needed
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!
Flags: needinfo?(drno)
Keywords: checkin-needed
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
Attachment #8590409 - Attachment is obsolete: true
Flags: needinfo?(drno)
Attachment #8607820 - Flags: review+
I don't see any problems in our tests.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed5bbd4ce3cf
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.
Flags: needinfo?(mreavy)
(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.
Flags: needinfo?(mreavy)
(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.
See Also: → 1167922
You need to log in before you can comment on or make changes to this bug.