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

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50aab2682a06a8fd7ad1c8b06798f360f11d733e

Comment 2

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5b6d9b1786293a9b5ac98b958ad5850afc8e889

Comment 3

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12fd013776c262db2bf6198d4ecb11d20757b10c

Comment 4

[Byron Campen [:bwc]]

Attachment: Bug 1479853: Prevent more than one VideoConduit having the same remote SSRC.

Comment 5

[Phabricator Automation]

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

Comment 6

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4953bed88db1c5e1ffdf3caac606feef2377f2e5

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6eac5ca54df4
Prevent more than one VideoConduit having the same remote SSRC. r=ng

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/6eac5ca54df4

Comment 10

[Andreas Pehrson [:pehrsons]]

Should we perhaps attempt an ESR uplift here Byron?

Comment 13

[Byron Campen [:bwc]]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[Nils Ohlmeier [:drno]]

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?

Comment 16

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bb928e10d3f67fe1b74845cc15925fa544dd067

Comment 17

[Byron Campen [:bwc]]

Attachment: Prevent more than one VideoConduit having the same remote SSRC. (esr60 backport)

Comment 18

[Byron Campen [:bwc]]

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:

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 20

[Ryan VanderMeulen [: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).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/4faeb696dd06