886134 - SDP for data channel uses SCTP/DTLS instead of DTLS/SCTP

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Kahmyong Moon]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130622 Firefox/23.0 (Nightly/Aurora)
Build ID: 20130622004018

Steps to reproduce:

Using the WebRTC JavaScript API:

1. Created a mozRTCPeerConnection.
2. Created a data channel (createDataChannel("label")).
3. Created an offer (createOffer(success, failure)).
4. Logged the offered SDP to console.log.


Actual results:

The returned SDP contained this m= line:

  m=application 58604 SCTP/DTLS 5000


Expected results:

The returned SDP should have contained this m= line:

  m=application 58604 DTLS/SCTP 5000

In an SDP, SCTP/DTLS indicates DTLS over SCTP (analogous to TCP/TLS), while DTLS/SCTP indicates SCTP over DTLS (which is what WebRTC uses):

  http://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-03

This was corrected going between draft-ietf-mmusic-sctp-sdp-00 and draft-ietf-mmusic-sctp-sdp-01.

Comment 1

[Ethan Hugg [:ehugg]]

I don't see an easy way of changing this without breaking back-compat.  Perhaps we should first push a hack that recognized both DTLS/SCTP and SCTP/DTLS (somewhere near sdp_token.c:1192).  Then when that releases we could switch to sending DTLS/SCTP.  

The other option would be to just switch at a time when we are already breaking back-compat.

Also, I looked at the SDP created by the latest Chrome Canary (30.0.1550.2) and it looks like this:

m=application 1 RTP/SAVPF 101

So it currently doesn't do either, so nothing to match up with there.

Comment 2

[Kahmyong Moon]

Chrome Canary 30 does use DTLS/SCTP in its offer now (I don't think it did in Chrome 29), but to enable SCTP instead of RTP, I think you need to turn on SCTP data channels in chrome://flags, and I think enable DtlsSrtpKeyAgreement in the RTCPeerConnection. (I haven't gotten SCTP data channels to work for me on Chrome, so that's about as far as I've gotten with that.)

This seems to be the relevant code in Chrome:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjingle/source/talk/session/media/mediasession.cc&q=DTLS/SCTP&sq=package:chromium&l=64

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

So we have a couple of options.  But basically, the only real option is to (for a time) allow SCTP/DTLS as a synonym for DTLS/SCTP when answering, and only accept one datachannel connection (good anyways), preferring the DTLS/SCTP one.  When offering, we offer *two* m=application lines, and have one be SCTP/DTLS and one DTLS/SCTP (for at least one release cycle).

Existing FF22's *should* (and we'll need to check) only accept the SCTP/DTLS m=application line.  Chrome and FF23 (assuming uplift soon) and up will accept only the DTLS/SCTP m=application line.  The only incompatibility would be the one we have now (FF22 <-> Chrome), unless we can get them to implement the same backwards-compat hack.  (obviously, sites/polyfills like apprtc can transform this, and given the other differences generally will make this a minor issue.)

One significant downside would be an additional m= line needing a port (temporarily), unless we cheat and use the same port for both for the lifetime of this hack.

Or we just totally break cross-version compat, which is an (ugly) option, but an option.

Comments?

Comment 4

[Ethan Hugg [:ehugg]]

Attachment: Change Datachannel m-line from SCTP/DTLS to DTLS/SCTP

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 769889 [details] [diff] [review]
Change Datachannel m-line from SCTP/DTLS to DTLS/SCTP

Review of attachment 769889 [details] [diff] [review]:
-----------------------------------------------------------------

Here's a starter patch that changes this but is not compatible with current code.  I could easily change it to recognize SCTP/DTLS and equivalent to DTLS/SCTP when parsing, but sending two m=application lines seems a bit crazy.  We should discuss at our next meeting.

Comment 6

[Ethan Hugg [:ehugg]]

From today's meeting here is the plan of record as I understand it:

For Firefox 23 I will make a patch that recognizes DTLS/SCTP as well as SCTP/DTLS but still emits SCTP/DTLS. so would still be compatible with 22.

For Firefox 24 I will make a patch that recognizes both as well but emits DTLS/SCTP.  So it should work with Firefox 23 but not Firefox 22.

Please comment if you think this is incorrect.  I should have the patches for this ready this week.

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: Make Datachannel DTLS/SCTP line forward-compatible with FF24 - target FF23

Comment 8

[Ethan Hugg [:ehugg]]

Attachment: Change Datachannel m-line from SCTP/DTLS to DTLS/SCTP - target FF24

Comment 9

[Ethan Hugg [:ehugg]]

Try run to see if I missed updating any tests:

https://tbpl.mozilla.org/?tree=Try&rev=b9aecfe28e09

Comment 10

[Ethan Hugg [:ehugg]]

Small patch for FF23 landed - 
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9967574468

Comment 11

[Ethan Hugg [:ehugg]]

Comment on attachment 770472 [details] [diff] [review]
Make Datachannel DTLS/SCTP line forward-compatible with FF24 - target FF23

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Needed by the other patch in this bug.  This is a patch so that the Datachannel can be forward-compatible to the version after.  The idea is that FF22 should work with FF23 and FF23 should work with FF24, even though FF22 will not work with FF24.

User impact if declined:
If declined we will need to postpone this fix for one more version at least.  This would also delay our interop with Chrome.
 
Testing completed (on m-c, etc.):
On M-I, should be on M-C tomorrow.  This code will not be exercised unless a Datachannel connection is attempted to a FF that has the other patch on this bug.
 
Risk to taking this patch (and alternatives if risky):
Should be no risk to Datachannels for 22 and 23, the biggest risk is that it doesn't work with the other patch that is slated for 24 which would delay that one, but still not break 23.

String or IDL/UUID changes made by this patch:
none.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6b9967574468

Comment 13

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f466f26ab2f9

Comment 14

[Ethan Hugg [:ehugg]]

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

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Ethan Hugg [:ehugg]]

Comment on attachment 770473 [details] [diff] [review]
Change Datachannel m-line from SCTP/DTLS to DTLS/SCTP - target FF24

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
The specification now says that the m=application line should be 'DTLS/SCTP'.  We need this to be compliant to the spec and to interop with Chrome.

User impact if declined: 
If declined this would delay our compliance and Chrome interop for another version cycle.  Also would leave Aurora unable to interop with Nightly.

Testing completed (on m-c, etc.): 
On M-C, tested against Beta which has the smaller compatibility patch.

Risk to taking this patch (and alternatives if risky): 
The primary risk is incompatibility with other versions of Firefox.  We are expecting it to be incompatible with 22 but should work with all versions 23+

String or IDL/UUID changes made by this patch:
none

Comment 17

[bhavana bajaj [:bajaj]]

(In reply to Ethan Hugg [:ehugg] from comment #16)
> Comment on attachment 770473 [details] [diff] [review]
> Change Datachannel m-line from SCTP/DTLS to DTLS/SCTP - target FF24
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> The specification now says that the m=application line should be
> 'DTLS/SCTP'.  We need this to be compliant to the spec and to interop with
> Chrome.
> 
> User impact if declined: 
> If declined this would delay our compliance and Chrome interop for another
> version cycle.  Also would leave Aurora unable to interop with Nightly.
> 
> Testing completed (on m-c, etc.): 
> On M-C, tested against Beta which has the smaller compatibility patch.
> 
> Risk to taking this patch (and alternatives if risky): 
> The primary risk is incompatibility with other versions of Firefox.  We are
> expecting it to be incompatible with 22 but should work with all versions 23+
> 
> String or IDL/UUID changes made by this patch:
> none

Keeping the user Impact described , we are ok to take the patch .Although, can you please explain the incompatibility situation a bit more ? Would WebRTC be totally incompatible with Fx22 or will a few apps that are supported will be unusable or are certain features broken ? If something which is very basic will be broken we can help release note it and pass it to our other support channels as needed.

Comment 18

[Ethan Hugg [:ehugg]]

>Keeping the user Impact described , we are ok to take the patch .Although, can 
>you please explain the incompatibility situation a bit more ? Would WebRTC be 
>totally incompatible with Fx22 or will a few apps that are supported will be 
>unusable or are certain features broken ? If something which is very basic will 
>be broken we can help release note it and pass it to our other support channels 
>as needed.

With this patch FF24 will not connect Datachannels with FF22 and earlier.    WebRTC Audio and Video should still connect, just not the Datachannel. FF23 will work with both 22 and 24.

This brings up a new problem for FF since we now have a need for compatibility across versions.  The consensus so far has been to attempt compatibility between sequential versions.  It is not possible to maintain compatibility with all old versions and still stay up-do-date with the WebRTC specifications.

Comment 19

[Ethan Hugg [:ehugg]]

Actually I need to clarify my last comment.  If a WebRTC connection is attempted between 22 and 24 that requests audio/video and Datachannels, it will fail with the error return from setRemoteDescription.  If a WebRTC connection is attempted with just audio and/or video it should still work.

This means, however, that many existing demos will not work between FF22 and FF24 even if they do not feature the datachannel to the user.

Comment 20

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d66e748a49b

Comment 21

[u279076]

Kahmyong Moon, can you please check Firefox 23, 24, and 25 to verify this is fixed across all branches? Thank you.

Comment 22

[Kahmyong Moon]

Sure. I checked release (23.0.1), beta (24.0b10), Aurora (25.0a2 2013-09-13), and Nightly (26.0a1 2013-09-13), and I verified all releases behave as outlined in comment 6 (23 uses SCTP/DTLS while 24 and later use DTLS/SCTP, 23 and later accept both).

Comment 23

[u279076]

(In reply to Kahmyong Moon from comment #22)
> I verified all releases behave as outlined in comment 6

Thank you very much.