1312691 - Drop support for RTCIceServer credentialType until "token" is really supported

Status: RESOLVED WONTFIX

(Core :: WebRTC, defect)

Description

[Philip Jägenstedt [:foolip]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36

Steps to reproduce:

new RTCPeerConnection({ iceServers: [{ urls: "turn:turn.example.org", username: "user", credential: "a-token", credentialType: "token" }] })




Actual results:

"token" is not yet implemented. Treating as password. https://bugzil.la/1247616" is logged to the console.


Expected results:

Until the "token" value is actually supported, it is better to not recognize credentialType at all. Supporting it but treating as "password" makes it impossible to later feature detect support for it, which could otherwise be done as such:

// assuming RTCPeerConnection itself is supported
credentialTypeSupported = false;
tokenSupported = false;
try {
  new RTCPeerConnection({ credentialType: "invalid" });
} catch(e) {
  credentialTypeSupported = true;
  try {
    new RTCPeerConnection({ credentialType: "token" });
    tokenSupported = true;
  } catch (e) {}
}

Some of these tests would start to fail, as they will in Blink until supported:
https://github.com/w3c/web-platform-tests/pull/4062
http://w3c-test.org/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html

Comment 1

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

The current behavior was requested in Bug 1247619. Changing this would probably be a breaking change at this point, though if Blink throws on it too that helps.

Ccing Maire for how to handle this.

Comment 2

[Philip Jägenstedt [:foolip]]

Gecko doesn't throw though, it just emits a console message. Throwing could be achieved by supporting only the "password" RTCIceCredentialType. Not sure what to think of that, it seems a bit odd to support an enum with just one value. Between Gecko's current behavior, supporting only "password" or dropping credentialType, which would you like?

Comment 3

[Adam Roach [:abr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #1)
> The current behavior was requested in Bug 1247619. Changing this would
> probably be a breaking change at this point, though if Blink throws on it
> too that helps.

I'm not sure why it would. I just checked the WebRTC 1.0 spec, looking at everywhere it says to throw an exception, and none of them pertain to an unrecognized credentialType value. If we choose to go down the proposed path, we would minimally need to submit a PR against the spec as well as implement a change.

All of that said, the gain from making these changes seems trivial at best. If you deploy a service that uses only token auth for TURN, then the proposed feature detection would just allow the webapp to leave off TURN from the ICE server list. Without the feature detection, the server rejects the request for invalid credentials. In both cases, relayed candidates are unavailable, and the chances of connecting are identical. All that is saved is something on the order of two 100-byte UDP packets.

Perhaps I'm missing something, though -- Philip, what would you actually *do* with the value of "tokenSupported" in your example?

Comment 4

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

It's handled by WebIDL. Enumeration types throw TypeError [1] on unknown values when used as method arguments [2].

[1] https://heycam.github.io/webidl/#es-enumeration
[2] https://heycam.github.io/webidl/#ref-for-dfn-idl-fragment-26

Comment 5

[Philip Jägenstedt [:foolip]]

I actually know preciously little about `credentialType: "token"` even does, I just noticed that Gecko is doing a slightly unusual thing. Maybe there will be a simpler feature detect for tokenSupported so that this doesn't matter, but if not then presumably people will need it in order to do one thing in new browsers and another everywhere else in the transition period where only some browsers support it.

To be clear, this is very far down the list of interop problems in WebRTC, but I wanted to follow up on everything I found when poking at a very small part -- the RTCConfiguration dictionary for the RTCPeerConnection constructor. (Unprefixed RTCPeerConnection soon in Chrome Canary, shout if I did a stupid thing and there's more changes that should happen before unprefixing.)

Comment 6

[Maire Reavy [:mreavy]]

I was debating moving this to "parking-lot", but truly I don't think we should take a patch for this even if one were submitted.  I think Adam's analysis in Comment 3 stands.  I don't see how this moves the needle for the web, and it would be a breaking change.  If a stronger argument for doing this comes up, please reopen this bug and make the argument.  

Thanks, Philip, for filing the bug.  It was a reasonable question to ask, and you followed the right process for doing so.  In the future, you can file bugs directly into Core:WebRTC where we will see them sooner.  Here's the link for that: https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=WebRTC

Comment 7

[Maire Reavy [:mreavy]]

Moving to WebRTC in case this gets reopened in the future.

Comment 8

[Philip Jägenstedt [:foolip]]

Very well, thanks all!