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)
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).
Reporter | ||
Comment 1•7 years ago
|
||
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.
Reporter | ||
Comment 2•7 years ago
|
||
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."
Updated•7 years ago
|
Component: General → WebRTC
Assignee | ||
Updated•7 years ago
|
Component: WebRTC → WebRTC: Signaling
Assignee | ||
Comment 3•7 years ago
|
||
Mozregression points to this https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=adff388db8bc1a8f445babcb470d1dde2d6709f3&tochange=58633245c138d58d9788eb1946a9de971d67cef8
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → drno
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Depends on: 1392745
Keywords: regression
Comment 6•7 years ago
|
||
mozreview-review |
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 7•7 years ago
|
||
mozreview-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+
Updated•7 years ago
|
Rank: 17
Priority: -- → P1
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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 9•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
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+
Comment 13•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae71d517c443 https://hg.mozilla.org/mozilla-central/rev/06c3517b0f33
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Blocks: 1392745
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
No longer depends on: 1392745
You need to log in
before you can comment on or make changes to this bug.
Description
•