Closed Bug 1115998 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: WebRTC: Networking, defect)

37 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: ibc, Assigned: jib)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → jib
Component: WebRTC → WebRTC: Networking
Yes, passing an array in the .url field is a late addition to the spec that we need to add support for.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Summary: mozRTCPeerConnection: NS_ERROR_UNEXPECTED when multiple TURN URLs in pcConfig.url → Add support for spec RTCOptions.iceServers.urls (plural) array form rather than deprecated .url
Also add a better error message since I'm sure the confusion will be common.
Summary: Add support for spec RTCOptions.iceServers.urls (plural) array form rather than deprecated .url → Add support for spec RTCConfiguration.iceServers.urls (plural) array form rather than deprecated .url
(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'
    }]
});
Depends on: 1116225
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).
No longer depends on: 1116225
Depends on: 1103153
Also update tests to validate DOMExceptions (Bug 1111666) around iceServer validation (see URL).
Attachment #8542651 - Flags: review?(martin.thomson)
Attachment #8542651 - Flags: review?(drno)
Error names are here http://w3c.github.io/webrtc-pc/#dom-peerconnection-updateice

Patch clobbers, so no longer depends on Bug 1103153.
Depends on: 1111666
No longer depends on: 1103153
Attachment #8542651 - Flags: review?(bugs)
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() ?
Attachment #8542651 - Flags: review?(martin.thomson) → review+
Incorporate feedback. Carrying forward r=mt.
Attachment #8542651 - Attachment is obsolete: true
Attachment #8542651 - Flags: review?(drno)
Attachment #8542651 - Flags: review?(bugs)
Attachment #8542982 - Flags: review?(bugs)
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)
Attachment #8542982 - Flags: review?(bugs) → review+
(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
Attachment #8542982 - Attachment is obsolete: true
Attachment #8544824 - Flags: review+
Depends on: 1107953
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
Attachment #8544824 - Attachment is obsolete: true
Attachment #8551810 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a227e6b5d0be
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Keywords: dev-doc-needed
Blocks: 1050930
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.
You need to log in before you can comment on or make changes to this bug.