User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.95 Safari/537.36 Steps to reproduce: Create a mozRTCPeerConnection with multiple TURN URLs in the iceServers field: var pc = new mozRTCPeerConnection({ iceServers: [{ url:['turn:1.2.3.4', 'turn:1.2.3.5'], username:'foo', credential:'1234' }] }); Actual results: [Exception... "Unexpected error" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1" data: no] (note that I run the above JS code in the browser console) Expected results: It should just work.

It also fails if two STUN URLs are set. It does not fail if multiple **single** URLs are set in separate entries of the .url Array.

It also fails if two STUN URLs are set. It does not fail if multiple **single** URLs are set in separate entries of the .url Array.

Yes, passing an array in the .url field is a late addition to the spec that we need to add support for.

Sorry, make that the .urls field (plural) for the array version (see spec URL in this bug). The syntax in comment 0 is invalid, and the error appropriate. I'm renaming the bug to mark that we should add support for the newer .urls field asap, but at least we're safely ignoring those and not fatally erroring out.

Also add a better error message since I'm sure the confusion will be common.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4) > but at least we're safely ignoring those and not fatally erroring out. Hmm, we still fatally error out with a "missing url" error with just a urls member in there, so the workaround would be: var pc = new mozRTCPeerConnection({ iceServers: [{ urls:['turn:1.2.3.4', 'turn:1.2.3.5'], url:'turn:1.2.3.4', // fallback for legacy browser username:'foo', credential:'1234' }] });

Thanks, but I do not understand. Since when does Firefox supports the .urls field (with an array of more than 1 URL)? It does not work in 37.0a1 (2014-12-29) (today's Nightly).

I didn't mean to suggest that Firefox supports .urls. On the contrary, the following would fatally error out (with "missing url"): var pc = new mozRTCPeerConnection({ iceServers: [{ urls:['turn:1.2.3.4', 'turn:1.2.3.5'], username:'foo', credential:'1234' }] }); So to avoid that, I suggested in comment 6 something that should work across browsers (Firefox will silently ignore .urls only if a .url is also found in there).

Also update tests to validate DOMExceptions (Bug 1111666) around iceServer validation (see URL).

Error names are here http://w3c.github.io/webrtc-pc/#dom-peerconnection-updateice Patch clobbers, so no longer depends on Bug 1103153.

Comment on attachment 8542651 [details] [diff] [review] support RTCIceServer.urls (plural) array form Review of attachment 8542651 [details] [diff] [review]: ----------------------------------------------------------------- Fix the missing .close() there. (I still think that this isn't the best choice of API). ::: dom/media/PeerConnection.js @@ +336,5 @@ > + if (typeof server.urls === "string") { > + server.urls = [server.urls]; > + } else if (!server.urls && server.url) { > + server.urls = [server.url]; > + // TODO: Remove support for legacy iceServer.url eventually Either a bug number or a date here probably ::: dom/media/tests/mochitest/test_peerConnection_bug825703.html @@ +16,5 @@ > > + makePC = (config, expected_error) => { > + var exception; > + try { > + new mozRTCPeerConnection(config); .close() ?

Incorporate feedback. Carrying forward r=mt.

Comment on attachment 8542982 [details] [diff] [review] support RTCIceServer.urls (plural) array form (2) r=mt >+PeerConnectionImpl::AddIceServer(const RTCIceServer &aServer, >+ IceConfiguration *aDst) nit, both & and * go with the type, not argument name. >+{ >+#ifdef MOZILLA_INTERNAL_API >+ NS_ENSURE_TRUE(aServer.mUrls.WasPassed(), NS_ERROR_UNEXPECTED); >+ NS_ENSURE_TRUE(aServer.mUrls.Value().IsStringSequence(), NS_ERROR_UNEXPECTED); I'd use NS_ENSURE_STATE here. http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h?rev=4b527c42326f&mark=369-370#369 >+ auto &urls = aServer.mUrls.Value().GetAsStringSequence(); Nit, & goes with the type, not variable name. (and personally I wouldn't use auto. Makes it less clear what kind of object 'urls' is)

(In reply to Olli Pettay [:smaug] from comment #13) > I'd use NS_ENSURE_STATE here. Thanks! I didn't know about that one. Quite handy. > >+ auto &urls = aServer.mUrls.Value().GetAsStringSequence(); > Nit, & goes with the type, not variable name. > (and personally I wouldn't use auto. Makes it less clear what kind of object > 'urls' is) & fixed. While I would agree that overuse of auto can cause confusion, I think several factors make it not the case here: 1) it's a named getter, 2) OwningStringOrStringSequence is an unstable dynamically generated mouthful (webidl union member-order dependency), that I'm trying not to learn or depend on. I love this type and what it does, but it seems irrelevant (here) what it's called. 3) this local stack indirection really only exists to stay within the 80-character limit. E.g. if not for the 80-character limit I might have written: nsresult rv = NS_NewURI(getter_AddRefs(url), aServer.mUrls.Value().GetAsStringSequence()[i]); which has the same amount of visible type-information. Compared to that, I think auto improves readability over excessive repetition or line-wrapping.

Incorporate feedback. Carrying forward r=smaug, mt

Rebased. Carrying forward r=smaug, mt Green try (red one is gUM, and we're not touching gUM here) - https://treeherder.mozilla.org/#/jobs?repo=try&revision=8665f780307c

After Bug 1111666.

Documentation needs to note when support for this was added, and that the old .url form is deprecated. Until we've checked on and if needed implemented those corrections, marking as doc update needed.

This page has now been written: https://developer.mozilla.org/en-US/docs/Web/API/RTCIceServer/urls This page has also been written and is labeled as obsolete: https://developer.mozilla.org/en-US/docs/Web/API/RTCIceServer/url The two pages reference each other and explanations are included about what the change means and so on. Also, the main page of the RTCIceServer dictionary has been updated to note the change as well: https://developer.mozilla.org/en-US/docs/Web/API/RTCIceServer And this change is now mentioned on Firefox 37 for developers.