Closed Bug 1396974 Opened 7 years ago Closed 7 years ago

Nightly 57.0a1 does not like WebRTC SDP offer it produces

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: ibc, Assigned: drno)

References

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36

Steps to reproduce:

- Create a PeerConnection and add audio and video (getUserMedia).

- Get the video associated RtpSender and call stop() on it.

- Call pc.createOffer().then((offer) => pc.setLocalDescription(offer)).


Actual results:

pc.setLocalDescription() fails with:

"Invalid description, no ice-ufrag attribute"

Well, it's worth noting that such a offer is created by Firefox itself, and when a m= section has been disabled (via videoRtpSender.stop()) Firefox does NOT add ICE lines into such an inactive m= section.


Expected results:

It should work (as it does in Firefox stable).
To reproduce it, just enter into this URL:

https://v2demo.mediasoup.org

(please, reload the page twice so verbose logs are enabled).

Once you have your webcam activated, click on the webcam icon to remove it. That action will invoke webcamRtpSender.stop() followed by a pc.setLocalDescription() that fails with the error above.

Again: this does NOT happen in Firefox stable.
To clarify, I meant:

"That action will invoke webcamRtpSender.stop() followed by a pc.createOffer() and a corresponding pc.setLocalDescription(offer) that fails with the error above."
Component: General → WebRTC
Component: WebRTC → WebRTC: Signaling
Assignee: nobody → drno
Depends on: 1392745
Keywords: regression
Comment on attachment 8906093 [details]
Bug 1396974: stop bundeling inactive m-sections.

https://reviewboard.mozilla.org/r/177826/#review182906

a=inactive shouldn't prevent bundle on its own. We should avoid bundling _disabled_ m-sections though. I've actually fixed this in my transceivers work; I thought I had messed this up with my patches!
Attachment #8906093 - Flags: review?(docfaraday) → review-
Comment on attachment 8906094 [details]
Bug 1396974: make tests ensure no bundle-only is present in inactive m-swctions.

https://reviewboard.mozilla.org/r/177828/#review182908
Attachment #8906094 - Flags: review?(docfaraday) → review+
Rank: 17
Priority: -- → P1
Comment on attachment 8906093 [details]
Bug 1396974: stop bundeling inactive m-sections.

https://reviewboard.mozilla.org/r/177826/#review182906

Fair enough. And great that this going to be fixed with Transceivers. But I butchered this in 57. As the Transceivers are not likely to land in 57 I think we should fix this somehow in 57. Plus this patch only restores the old behavior (before I landed bug 1392745 disabled m-sections never got an mid set and so they got included in bundles).

Now the reason I did not use MsectionIsDisabled() here is that MsectionIsDisabled() checks the bundle-only attribute, which is only going to be set by this code here. Should I then just check for port == 0 here instead of inactive?
Comment on attachment 8906093 [details]
Bug 1396974: stop bundeling inactive m-sections.

https://reviewboard.mozilla.org/r/177826/#review182906

a=bundle-only should not be set here, so you should get the same result either way. I agree we should go ahead and fix now.
Comment on attachment 8906093 [details]
Bug 1396974: stop bundeling inactive m-sections.

https://reviewboard.mozilla.org/r/177826/#review183916
Attachment #8906093 - Flags: review?(docfaraday) → review+
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/ae71d517c443
stop bundeling inactive m-sections. r=bwc
https://hg.mozilla.org/integration/autoland/rev/06c3517b0f33
make tests ensure no bundle-only is present in inactive m-swctions. r=bwc
You need to log in before you can comment on or make changes to this bug.