Closed Bug 1325991 Opened 7 years ago Closed 7 years ago

Firefox emits a=bundle-only with a non-zero port

Categories

(Core :: WebRTC: Signaling, defect, P2)

Product:
Core
Component:
WebRTC: Signaling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: ekr, Assigned: mjf)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1325991 - sections with bundle-only should have port set to 0.
59 bytes, text/x-review-board-request
drno
: review+
Details 
This is not a specified behavior per:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-bundle-negotiation-36#section-6

"The usage of the 'bundle-only' attribute is only defined for a
bundled "m=" line with a zero port value, within an offer.  Other
usage is unspecified."

See:
v=0

o=mozilla...THIS_IS_SDPARTA-53.0a1 3889973888745482888 0 IN IP4 0.0.0.0

s=-

t=0 0

a=sendrecv

a=fingerprint:sha-256 AE:62:33:3E:4E:31:C6:77:1A:33:F5:EB:B2:D7:4B:83:55:42:A0:33:B7:88:04:2F:8C:11:A2:B9:4B:01:DE:36

a=group:BUNDLE sdparta_0 sdparta_1

a=ice-options:trickle

a=msid-semantic:WMS *

m=audio 29675 UDP/TLS/RTP/SAVPF 109 9 0 8 101

c=IN IP4 74.125.39.35

a=candidate:0 1 UDP 2122121471 10.252.34.38 61206 typ host

a=candidate:6 1 UDP 2122187007 2620:101:80fc:232:6e40:8ff:fe92:b842 61207 typ host

a=candidate:12 1 UDP 2122252543 2620:101:80fc:232:45f6:aa00:92fb:2781 61208 typ host

a=candidate:0 2 UDP 2122121470 10.252.34.38 63659 typ host

a=candidate:6 2 UDP 2122187006 2620:101:80fc:232:6e40:8ff:fe92:b842 63660 typ host

a=candidate:12 2 UDP 2122252542 2620:101:80fc:232:45f6:aa00:92fb:2781 63661 typ host

a=candidate:2 2 UDP 1685921534 63.245.221.34 6647 typ srflx raddr 10.252.34.38 rport 63659

a=candidate:3 2 UDP 92086014 216.58.219.30 28927 typ relay raddr 216.58.219.30 rport 28927

a=candidate:2 1 UDP 1685921535 63.245.221.34 31979 typ srflx raddr 10.252.34.38 rport 61206

a=candidate:3 1 UDP 92086015 74.125.39.35 29675 typ relay raddr 74.125.39.35 rport 29675

a=candidate:18 1 UDP 8200191 74.125.39.35 30000 typ relay raddr 74.125.39.35 rport 30000

a=candidate:1 2 UDP 1685921790 63.245.221.34 6647 typ srflx raddr 10.252.34.38 rport 63659

a=candidate:1 1 UDP 1685921791 63.245.221.34 31979 typ srflx raddr 10.252.34.38 rport 61206

a=sendrecv

a=end-of-candidates

a=extmap:1/sendonly urn:ietf:params:rtp-hdrext:ssrc-audio-level

a=fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=1

a=fmtp:101 0-15

a=ice-pwd:7f4d33622599b9226e4623b7fd258e3a

a=ice-ufrag:cfb391d1

a=mid:sdparta_0

a=msid:{72f03dc3-f67d-0147-99c9-b81f023d5047} {97c41d60-df12-b84c-9db9-75a4418f5217}

a=rtcp:28927 IN IP4 216.58.219.30

a=rtcp-mux

a=rtpmap:109 opus/48000/2

a=rtpmap:9 G722/8000/1

a=rtpmap:0 PCMU/8000

a=rtpmap:8 PCMA/8000

a=rtpmap:101 telephone-event/8000

a=setup:actpass

a=ssrc:1198910356 cname:{24ddd714-bd42-0849-8673-a2c3bf126ff2}

m=video 30452 UDP/TLS/RTP/SAVPF 121 120 126 97

c=IN IP4 74.125.39.35

a=bundle-only

a=candidate:0 1 UDP 2122121471 10.252.34.38 50887 typ host

a=candidate:6 1 UDP 2122187007 2620:101:80fc:232:6e40:8ff:fe92:b842 50888 typ host

a=candidate:12 1 UDP 2122252543 2620:101:80fc:232:45f6:aa00:92fb:2781 50889 typ host

a=candidate:0 2 UDP 2122121470 10.252.34.38 53022 typ host

a=candidate:6 2 UDP 2122187006 2620:101:80fc:232:6e40:8ff:fe92:b842 53023 typ host

a=candidate:12 2 UDP 2122252542 2620:101:80fc:232:45f6:aa00:92fb:2781 53024 typ host

a=candidate:2 2 UDP 1685921534 63.245.221.34 22095 typ srflx raddr 10.252.34.38 rport 53022

a=candidate:3 2 UDP 92086014 216.58.219.30 30077 typ relay raddr 216.58.219.30 rport 30077

a=candidate:18 1 UDP 8200191 74.125.39.35 30452 typ relay raddr 74.125.39.35 rport 30452

a=candidate:1 1 UDP 1685921791 63.245.221.34 29135 typ srflx raddr 10.252.34.38 rport 50887

a=candidate:18 2 UDP 8200190 74.125.39.35 27662 typ relay raddr 74.125.39.35 rport 27662

a=candidate:1 2 UDP 1685921790 63.245.221.34 22095 typ srflx raddr 10.252.34.38 rport 53022

a=sendrecv

a=end-of-candidates

a=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1

a=fmtp:97 profile-level-id=42e01f;level-asymmetry-allowed=1

a=fmtp:120 max-fs=12288;max-fr=60

a=fmtp:121 max-fs=12288;max-fr=60

a=ice-pwd:7f4d33622599b9226e4623b7fd258e3a

a=ice-ufrag:cfb391d1

a=mid:sdparta_1

a=msid:{72f03dc3-f67d-0147-99c9-b81f023d5047} {87bf3f62-974a-264c-8e40-d750595222a8}

a=rtcp:30077 IN IP4 216.58.219.30

a=rtcp-fb:120 nack

a=rtcp-fb:120 nack pli

a=rtcp-fb:120 ccm fir

a=rtcp-fb:120 goog-remb

a=rtcp-fb:121 nack

a=rtcp-fb:121 nack pli

a=rtcp-fb:121 ccm fir

a=rtcp-fb:121 goog-remb

a=rtcp-fb:126 nack

a=rtcp-fb:126 nack pli

a=rtcp-fb:126 ccm fir

a=rtcp-fb:126 goog-remb

a=rtcp-fb:97 nack

a=rtcp-fb:97 nack pli

a=rtcp-fb:97 ccm fir

a=rtcp-fb:97 goog-remb

a=rtcp-mux

a=rtpmap:120 VP8/90000

a=rtpmap:121 VP9/90000

a=rtpmap:126 H264/90000

a=rtpmap:97 H264/90000

a=setup:actpass

a=ssrc:1889605585 cname:{24ddd714-bd42-0849-8673-a2c3bf126ff2}
Byron, FYI
Flags: needinfo?(docfaraday)
Looks like the code causing this http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp#2309
was actually done by Michael.

I'm guessing this http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/media/webrtc/signaling/src/sdp/SdpHelper.cpp#373 was either never prepared to handle bundle policies or got refactored one time to often.
Flags: needinfo?(docfaraday) → needinfo?(mfroman)
backlog: --- → webrtc/webaudio+
Rank: 25
Priority: -- → P2
OK, its actually causing an interop failure with Chrome, so I suspect you need a higher priority than this.
This seems to have made it as far as Beta. I just tested Beta vs. Canary on apprtc and we're getting failures. Needinfoing mreavy for escalation
Flags: needinfo?(mreavy)
deadbeef: FYI, any chrome changes you made between chrome 57.0.2950.4 (still works) and 57.0.2966.0 (broken)?
Assignee: nobody → mfroman
Rank: 25 → 10
Flags: needinfo?(mreavy)
Priority: P2 → P1
(In reply to Nils Ohlmeier [:drno] from comment #2)
> Looks like the code causing this
> http://searchfox.org/mozilla-central/rev/
> 51aa673e28802fe6ea89f4793721fc55334a6ac8/media/webrtc/signaling/src/jsep/
> JsepSessionImpl.cpp#2309
> was actually done by Michael.
> 
> I'm guessing this
> http://searchfox.org/mozilla-central/rev/
> 51aa673e28802fe6ea89f4793721fc55334a6ac8/media/webrtc/signaling/src/sdp/
> SdpHelper.cpp#373 was either never prepared to handle bundle policies or got
> refactored one time to often.

We never handled setting the port to 0 for sections with the bundle-only attribute during CreateOffer.
Flags: needinfo?(mfroman)
Comment on attachment 8822607 [details]
Bug 1325991 - sections with bundle-only should have port set to 0.

https://reviewboard.mozilla.org/r/101476/#review101988

Drive-by r+; LGTM.  You can wait on drno if you want confirmation
Attachment #8822607 - Flags: review+
Comment on attachment 8822607 [details]
Bug 1325991 - sections with bundle-only should have port set to 0.

https://reviewboard.mozilla.org/r/101476/#review101990

LGTM once we have the additional unit test coverage (see below).

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:606
(Diff revision 1)
>  
>        if (useBundleOnly) {
>          attrs.SetAttribute(
>              new SdpFlagAttribute(SdpAttribute::kBundleOnlyAttribute));
> +        // Set port to 0 for sections with bundle-only attribute. (mjf)
> +        sdp->GetMediaSection(i).SetPort(0);

I think we really need to cover this with tests. So lets please add more asserts here http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/media/webrtc/signaling/gtest/jsep_session_unittest.cpp#4152 to ensure the bundle master port is not zero and all bundle-only ports are zero.
Attachment #8822607 - Flags: review?(drno) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e4e55af56102
sections with bundle-only should have port set to 0. r=drno,jesup
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e4e55af56102
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Re comment #5: Yes, I added code that starts parsing "a=bundle-only", and interprets "a=bundle-only" with a nonzero port as malformed SDP. I can relax this parsing, since this use of bundle-only is only "unspecified", not disallowed.

But on that note: previous versions of Chrome will *not* parse "a=bundle-only" at all. So if they see "bundle-only" with a zero port, they'll interpret that media description as rejected, like a non-bundle-aware endpoint. That's why I implemented the parsing first, and planned to actually start using a zero port a few Chrome versions later.
We've discussed this more and decided that this fix should be backed out until Chrome stable is fully bundle-aware.  What is the best way to go about this?
Flags: needinfo?(philringnalda)
That will be in M57, which is estimated to be stable on March 14th, FYI.
So you're backing something out of Gecko 53, which ships April 18th, until Chrome catches up more than a month before that?

If that's actually what you want to do, you can either push the backout to mozilla-inbound yourself if you have L3 access, or get a friend who does have L3 access to do it, or attach it as a moz-review patch and get someone to 'review' it and land it to autoland for you, or find the sheriff on duty on IRC (generally easiest to tell who it is in #sheriffs) and get them to back it out for you.
Flags: needinfo?(philringnalda)
(In reply to Phil Ringnalda (:philor) from comment #16)
> So you're backing something out of Gecko 53, which ships April 18th, until
> Chrome catches up more than a month before that?

A couple of issues: Chrome updates aren't instantaneous.  Leaving this in will break interop between FF Nightly and Chrome Release until at least March 14th (longer in some cases), and progressively break Aurora 53 and Beta 53 against Chrome Release.  Also, there's no real downside now to backing it out.
Reopening after backout - we'll return to this after Chrome's change has made it to release.
Status: RESOLVED → REOPENED
Rank: 10 → 21
Priority: P1 → P2
Resolution: FIXED → ---
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e038230119a5
Backed out changeset e4e55af56102 on request from mjf
backed out on request from mjf
Since we're well beyond Mar 14th, and the previous (backed-out) fix now works with Chrome, let's reapply the fix.
Attachment #8822607 - Attachment is obsolete: true
Comment on attachment 8865583 [details]
Bug 1325991 - sections with bundle-only should have port set to 0.

https://reviewboard.mozilla.org/r/137206/#review140278
Attachment #8865583 - Flags: review?(drno) → review+
I tested this patch with the current release version of Chrome, with Firefox Nightly, and with Firefox release across appr.tc, appear.in, talky.io, and jitsi.  The only service that adds the bundle-only attribute is appr.tc.  In all the permutations, the call worked properly.

Nils, based on these tests are we ready to land this after a rebase?
Flags: needinfo?(drno)
(In reply to Michael Froman [:mjf] from comment #24)
> I tested this patch with the current release version of Chrome, with Firefox
> Nightly, and with Firefox release across appr.tc, appear.in, talky.io, and
> jitsi.  The only service that adds the bundle-only attribute is appr.tc.  In
> all the permutations, the call worked properly.
> 
> Nils, based on these tests are we ready to land this after a rebase?

Sounds like we are good to land.
Thanks for checking thoroughly.
Flags: needinfo?(drno)
Keywords: checkin-needed
status-firefox53: fixed → wontfix
status-firefox54: --- → affected
status-firefox55: --- → affected
Target Milestone: mozilla53 → ---
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/308939ba890c
sections with bundle-only should have port set to 0. r=drno
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/308939ba890c
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Mark 54 won't fix as this is late for Beta54.
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: