1115998 - Add support for spec RTCConfiguration.iceServers.urls (plural) array form rather than deprecated .url

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Iñaki Baz Castillo]

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.

Comment 1

[Iñaki Baz Castillo]

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.

Comment 2

[Iñaki Baz Castillo]

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.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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'
    }]
});

Comment 7

[Iñaki Baz Castillo]

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).

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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).

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: support RTCIceServer.urls (plural) array form

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

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Error names are here http://w3c.github.io/webrtc-pc/#dom-peerconnection-updateice

Patch clobbers, so no longer depends on Bug 1103153.

Comment 11

[Martin Thomson [:mt:]]

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() ?

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: support RTCIceServer.urls (plural) array form (2) r=mt

Incorporate feedback. Carrying forward r=mt.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: support RTCIceServer.urls (plural) array form (3) r=smaug, mt

Incorporate feedback. Carrying forward r=smaug, mt

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: support RTCIceServer.urls (plural) array form (4) 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

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

After Bug 1111666.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a227e6b5d0be

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a227e6b5d0be

Comment 20

[Eric Shepherd [:sheppy]]

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.

Comment 21

[Eric Shepherd [:sheppy]]

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.