Closed
Bug 1143827
Opened 10 years ago
Closed 10 years ago
Remove Mozilla's default STUN server
Categories
(Core :: WebRTC: Networking, defect, P2)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: drno, Assigned: drno)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 3 obsolete files)
3.41 KB,
patch
|
drno
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Rank: 25
Flags: firefox-backlog+
Priority: -- → P2
Comment 1•10 years ago
|
||
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•10 years ago
|
||
That seems surprising, since those demos will already not work with Chrome.
Comment 3•10 years ago
|
||
Good point, it would only affect demos specifically written for Firefox. Probably not that many of those.
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Updated•10 years ago
|
backlog: --- → webRTC+
Flags: firefox-backlog+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 11•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
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+
Comment 14•10 years ago
|
||
Keywords: checkin-needed
Comment 15•10 years ago
|
||
Assignee: nobody → drno
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 17•10 years ago
|
||
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)
Comment 18•10 years ago
|
||
(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•10 years ago
|
||
I was thinking more like Hacks, though it may not warrant a post just on its own.
Comment 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
(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•9 years ago
|
Keywords: dev-doc-needed
Comment 22•9 years ago
|
||
Updated https://developer.mozilla.org/en-US/Firefox/Releases/41#WebRTC to note this change.
Updated•9 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•