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

VERIFIED FIXED in Firefox 23

Status

()

Core
WebRTC: Signaling
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Kahmyong Moon, Assigned: ehugg)

Tracking

Trunk
mozilla25
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox23 verified, firefox24 verified, firefox25 verified)

Details

(Whiteboard: [WebRTC][blocking-webrtc-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Flags: needinfo?(rjesup)
(Reporter)

Comment 2

5 years ago
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
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?
Flags: needinfo?(rjesup)

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [WebRTC][blocking-webrtc-][webrtc-uplift]
(Assignee)

Comment 4

5 years ago
Created attachment 769889 [details] [diff] [review]
Change Datachannel m-line from SCTP/DTLS to DTLS/SCTP
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #769889 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Assignee: nobody → ethanhugg
(Assignee)

Comment 7

5 years ago
Created attachment 770472 [details] [diff] [review]
Make Datachannel DTLS/SCTP line forward-compatible with FF24 - target FF23
(Assignee)

Comment 8

5 years ago
Created attachment 770473 [details] [diff] [review]
Change Datachannel m-line from SCTP/DTLS to DTLS/SCTP - target FF24
(Assignee)

Updated

5 years ago
Attachment #770472 - Flags: review?(rjesup)
(Assignee)

Updated

5 years ago
Attachment #770473 - Flags: review?(rjesup)

Updated

5 years ago
Attachment #770472 - Flags: review?(rjesup) → review+

Updated

5 years ago
Attachment #770473 - Flags: review?(rjesup) → review+
(Assignee)

Comment 9

5 years ago
Try run to see if I missed updating any tests:

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

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc-][webrtc-uplift] → [WebRTC][blocking-webrtc-][webrtc-uplift][leave-open]
(Assignee)

Comment 11

5 years ago
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.
Attachment #770472 - Flags: approval-mozilla-beta?
Attachment #770472 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 16

5 years ago
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
Attachment #770473 - Flags: approval-mozilla-aurora?
(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.

Updated

5 years ago
Attachment #770473 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 18

5 years ago
>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.
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d66e748a49b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC][blocking-webrtc-][webrtc-uplift][leave-open] → [WebRTC][blocking-webrtc-][webrtc-uplift]
status-firefox23: --- → fixed
status-firefox24: --- → fixed
status-firefox25: --- → fixed
Target Milestone: --- → mozilla25
Whiteboard: [WebRTC][blocking-webrtc-][webrtc-uplift] → [WebRTC][blocking-webrtc-]
Kahmyong Moon, can you please check Firefox 23, 24, and 25 to verify this is fixed across all branches? Thank you.
Flags: needinfo?(kahmyong.moon)
(Reporter)

Comment 22

5 years ago
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).
Flags: needinfo?(kahmyong.moon)
(In reply to Kahmyong Moon from comment #22)
> I verified all releases behave as outlined in comment 6

Thank you very much.
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
status-firefox24: fixed → verified
status-firefox25: fixed → verified
You need to log in before you can comment on or make changes to this bug.