Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid value for stunServers #117

Closed
jwoertink opened this issue Oct 27, 2014 · 8 comments
Closed

Invalid value for stunServers #117

jwoertink opened this issue Oct 27, 2014 · 8 comments
Assignees

Comments

@jwoertink
Copy link

I'm getting an Invalid value error when trying to add an array of stunServers to my config. Using version 0.6.3.

http://sipjs.com/api/0.6.0/ua_configuration_parameters/#stunservers

screen shot 2014-10-27 at 3 09 13 pm

@wakamoleguy
Copy link
Contributor

It looks like we are translating the array of stun servers improperly when creating the media handler. The error shows up only when you attempt to place a call. For the lazy, here are statements that can reproduce the error:

a = new SIP.UA({stunServers: ['stun:stun.l.google.com','stun:stun2.l.google.com']})
a.invite('foo@example.com');

@josephfrazier josephfrazier self-assigned this Oct 29, 2014
@josephfrazier
Copy link
Contributor

@jwoertink, I wasn't able to reproduce your error in JSFiddle. Could you provide a code snippet that will (in either a browser or Node)?

However, that fiddle did reproduce @wakamoleguy's error in Firefox. It looks like Firefox only allows String RTCIceServer urls, so I'll push a fix for that.

josephfrazier pushed a commit that referenced this issue Oct 29, 2014
This avoids the following sort of situation, which Firefox (as of 33.0.2) has
trouble with:

  var RTCPeerConnection =
    window.webkitRTCPeerConnection || window.mozRTCPeerConnection;

  new RTCPeerConnection({
      'iceServers': [{
          "url": ["stun:stun.l.google.com", "stun:stun2.l.google.com"]
      }]
  })

addresses #117
@jwoertink
Copy link
Author

hm... Yeah, I made a separate app to test it, and I can't reproduce it either. It must be something else in my app that conflicts and causing the issue. At least we got one issue solved! I'll close this out for now.

josephfrazier pushed a commit to egreenmachine/SIP.js that referenced this issue Nov 24, 2014
This avoids the following sort of situation, which Firefox (as of 33.0.2) has
trouble with:

  var RTCPeerConnection =
    window.webkitRTCPeerConnection || window.mozRTCPeerConnection;

  new RTCPeerConnection({
      'iceServers': [{
          "url": ["stun:stun.l.google.com", "stun:stun2.l.google.com"]
      }]
  })

addresses onsip#117

(cherry picked from commit 28523b6)
@jwoertink
Copy link
Author

I'm getting this issue again, but a little easier to debug.

Using stun:global.stun.twilio.com:3478?transport=udp for my stunServers setting throws this error. If I remove the query param, then it will work.

@jwoertink jwoertink reopened this Nov 25, 2014
@josephfrazier
Copy link
Contributor

Hmm, I would have expected that to work, too. Since it didn't, I dug into the code and the relevant specs to find out why. Here's what I found:

When you create the UA, it validates the stunServers against a formal grammar derived from draft-nandakumar-rtcweb-stun-uri (now RFC 7064), which does not allow for parameters in the URI. The Design Notes Appendix of the aforementioned draft links to a blog post that goes into detail about why the "transport" parameter is not allowed in a STUN URI, if you're really curious.

@jwoertink
Copy link
Author

oh wow. Yeah, that's the URI given by twilio for their new STUN/TURN service. Both the stun and trun uri's have a transport query param 😒 I'll see what they say about the specs to see if they are planning on changing that.

@josephfrazier
Copy link
Contributor

Are you able to make calls using the STUN URI with the parameter removed? If so, I'd just go with that.

EDIT: Note that TURN URIs can have the "transport" parameter and should work with the turnServers UA option

@wakamoleguy
Copy link
Contributor

I do not believe we currently plan on supporting invalid STUN URLS. I'll close the issue, and it can be reopened if it looks like there is a compelling reason to tackle this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants