TURN responses with bandwidth attr result in failure

RESOLVED FIXED in Firefox 53

Status

()

Core
WebRTC: Networking
P1
normal
Rank:
19
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: drno, Assigned: drno)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox51 wontfix, firefox52 wontfix, firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

10 months ago
Apparently the Bandwidth attribute falls into the category of comprehension required attributes. Therefore when we receive that attribute, which we don't support, we mark the transaction as failed and try again until the TURN client transaction times out.

I don't like that this apparently causes interop issues: http://stackoverflow.com/questions/42120698/webrtc-missing-relay-candidates-in-firefox
On the other hand the server sends us a "reserved" attribute (0x10) according to IANA http://www.iana.org/assignments/stun-parameters/stun-parameters.xhtml#stun-parameters-4 which falls into the category o comprehension required.

Byron, Ekr: what are your thoughts on this?
Flags: needinfo?(ekr)
Flags: needinfo?(docfaraday)
(Assignee)

Updated

10 months ago
backlog: --- → webrtc/webaudio+
Rank: 19
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected

Comment 2

10 months ago
Is the attribute advisory or mandatory. If the former, let's just add it to our table of known attributes, parse it, and skip.
Flags: needinfo?(ekr)

Comment 3

10 months ago
So, looking at draft-thomson-mmusic-rtcweb-bw-consent-00 (which is way expired, btw), that attribute is comprehension optional. This sounds like the other agent is just flat out broken in at least two different ways.

Martin, since you worked on the spec on BANDWIDTH, what's your take?
Flags: needinfo?(docfaraday) → needinfo?(martin.thomson)
(Assignee)

Comment 4

10 months ago
Byron I think you meant draft-thomson-tram-turn-bandwidth-01 which is indeed way expired.
But it is a good point that the draft specifies the attribute to be comprehension-optional. Which appears to be wrong to me, because with type 0x0010 it is in the 0x0000-0x7FFF range for comprehension-required from RFC 5389 https://tools.ietf.org/html/rfc5389#section-15.

IMHO the attribute should have been put in the > 0x8000 range by the draft.
Since the attribute apparently is used in deployed (spec violating?) servers I guess the best we can do is what Ekr suggested to add it to the list of known attributes and skip it.

Comment 5

10 months ago
That's probably reasonable.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

10 months ago
Just manually confirmed that adding the attribute with the patch makes Firefox work with the viagenie.ca coturn installation from the stackoverflow question.

Comment 8

10 months ago
mozreview-review
Comment on attachment 8836210 [details]
Bug 1338384: added STUN bandwidth attribute.

https://reviewboard.mozilla.org/r/111666/#review112962
Attachment #8836210 - Flags: review?(docfaraday) → review+

Comment 9

10 months ago
Yep, seems like you have the answer you need.  And damn, I can barely remember that.  The reason I chose that codepoint is that Microsoft were using that specific value in their TURN implementation.
Flags: needinfo?(martin.thomson)

Comment 10

10 months ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/18699bcc1c2c
added STUN bandwidth attribute. r=bwc

Comment 11

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/18699bcc1c2c
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 12

10 months ago
Too late for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
(Assignee)

Comment 13

10 months ago
Comment on attachment 8836210 [details]
Bug 1338384: added STUN bandwidth attribute.

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression. Problem exist since we Firefox supports WebRTC.
[User impact if declined]: Turn allocations with some WebRTC services fails, so calls might not get established.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It is pretty low risk as the patch only whitelists a single attribute ID in our STUN parser.
[String changes made/needed]: N/A
Attachment #8836210 - Flags: approval-mozilla-aurora?

Comment 14

10 months ago
Comment on attachment 8836210 [details]
Bug 1338384: added STUN bandwidth attribute.

Fix a WebRTC issue. Aurora53+.
Attachment #8836210 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 15

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/244f8f9fdc51
status-firefox53: affected → fixed
status-firefox52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.