Closed Bug 1479853 Opened 6 years ago Closed 6 years ago

SSRC switching logic can hit non-debug asserts in webrtc.org

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

Product:
Core
Component:
WebRTC: Audio/Video
Version:
60 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed
firefox63 --- fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

Crash Data

Attachments

(2 files)

Bug 1479853: Prevent more than one VideoConduit having the same remote SSRC.
46 bytes, text/x-phabricator-request
ng
: review+
RyanVM
: approval-mozilla-esr60-
Details | Review
Prevent more than one VideoConduit having the same remote SSRC. (esr60 backport)
15.31 KB, patch
RyanVM
: approval-mozilla-esr60+
Details | Diff | Splinter Review 
Our SSRC switching logic in VideoConduit makes it pretty easy for an RTP endpoint to (nullptr) crash our release builds. jitsi does this inadvertently (and somewhat intermittently), see https://bugzilla.mozilla.org/show_bug.cgi?id=1472321#c31.

The gist is that every time we see an unsignaled SSRC change, we try to change the SSRC on the VideoConduit, but we do not ensure that no other VideoConduit is using that new SSRC. If more than one VideoConduit is using the same SSRC, this violates invariants down in webrtc.org, some of which are enforced by non-debug assertions.

Even if we were to remove the unsignaled SSRC logic, we're just going to run into this problem again when we support demux using RTP MID.

We will need to fix this by checking if any conduit is using a given SSRC before setting that SSRC on any other conduit, probably.
Rank: 19
Priority: -- → P2
Comment on attachment 8997556 [details]
Bug 1479853: Prevent more than one VideoConduit having the same remote SSRC.

Nico Grunbaum [:ng] has approved the revision.

https://phabricator.services.mozilla.com/D2738
Attachment #8997556 - Flags: review+
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6eac5ca54df4
Prevent more than one VideoConduit having the same remote SSRC. r=ng
https://hg.mozilla.org/mozilla-central/rev/6eac5ca54df4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee: nobody → docfaraday
Should we perhaps attempt an ESR uplift here Byron?
Crash Signature: [@ mozalloc_abort | abort | webrtc::internal::Call::DestroyVideoReceiveStream ] [@ abort | webrtc::internal::Call::DestroyVideoReceiveStream ] [@ mozalloc_abort | abort | webrtc::internal::Call::DestroyVideoSendStream ] [@ ReceivePort::WaitForMessage |…
Flags: needinfo?(docfaraday)
Comment on attachment 8997556 [details]
Bug 1479853: Prevent more than one VideoConduit having the same remote SSRC.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

   This makes it pretty easy for a site to crash tabs at will, and some sites do this accidentally.

User impact if declined: 

   Lots of potential annoyance.

Fix Landed on Version:

   63

Risk to taking this patch (and alternatives if risky): 

   Fairly low.

String or UUID changes made by this patch: 

   None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(docfaraday)
Attachment #8997556 - Flags: approval-mozilla-esr60?
Comment on attachment 8997556 [details]
Bug 1479853: Prevent more than one VideoConduit having the same remote SSRC.

This doesn't graft cleanly to ESR60. Please attach a rebased patch and re-nominate for approval.
Attachment #8997556 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60-
Byron can you please rebase you patch for ESR and re-request uplift as this seems to be the number one crasher for WebRTC on ESR?
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday) → needinfo?(ryanvm)
Comment on attachment 9020359 [details] [diff] [review]
Prevent more than one VideoConduit having the same remote SSRC. (esr60 backport)

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:
Attachment #9020359 - Flags: approval-mozilla-esr60?
An approval request is enough, no need to NI. That said, it'll need to into 60.4 at this point since we already shipped 60.3 earlier this week.
status-firefox-esr60: --- → affected
Flags: needinfo?(ryanvm)
Comment on attachment 9020359 [details] [diff] [review]
Prevent more than one VideoConduit having the same remote SSRC. (esr60 backport)

Approving for 60.3.1esr (or 60.4.0esr, whatever we happen to ship next).
Attachment #9020359 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: