892911 - WebRTC crash [@gsmsdp_negotiate_media_lines]

Status: RESOLVED WONTFIX

(Core :: WebRTC: Signaling, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with hg.mozilla.org/integration/mozilla-inbound/rev/60865db5f5dc

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Christoph Diehl [:posidron]]

Attachment: console.txt

Comment 3

[Adam Roach [:abr]]

Attachment: Check that media section is found before adding rtcp-fb attributes

Comment 4

[Adam Roach [:abr]]

Comment on attachment 774570 [details] [diff] [review]
Check that media section is found before adding rtcp-fb attributes

I beleive the trigger here is the recvonly on the video line, where PC2 doesn't generate a corresponding media section. In any case, the trigger here is that we can reach this point in the code with media == 0. This check catches that situation.

Comment 5

[Eric Rescorla (:ekr)]

Hmm... I note that the error is in setremote. I wonder if perhaps you shouldn't be adding the
rtcp-fb in this case at all. Won't you get another crack at it in createanswer?

Comment 6

[Scoobidiver (away)]

There's a space after @ in the signature so that it shows up in crash stats.

Comment 7

[Adam Roach [:abr]]

Attachment: Check that media section is found before adding rtcp-fb attributes

Comment 8

[Adam Roach [:abr]]

Comment on attachment 774722 [details] [diff] [review]
Check that media section is found before adding rtcp-fb attributes

You're right that this only makes sense when we're crafting an answer. I've adjusted the conditional accordingly. I'd still like to keep the null check for media in place; I can't rule out the possibility that we might get here with a null "media" even in the create_answer case.

Comment 9

[Adam Roach [:abr]]

r=ekr on the original patch via IRC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/75d923fcb71c

Marking [leave-open] because I need to add some mochi tests here.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/75d923fcb71c

Comment 11

[Adam Roach [:abr]]

Comment on attachment 774722 [details] [diff] [review]
Check that media section is found before adding rtcp-fb attributes

Setting r+ from IRC conversation

Comment 12

[Adam Roach [:abr]]

Comment on attachment 774570 [details] [diff] [review]
Check that media section is found before adding rtcp-fb attributes

Okay, this is the one that ekr r+'d (and the one that we landed). Marking as such and requesting aurora uplift:

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Bug 880067

User impact if declined: Browser crashiness

Testing completed (on m-c, etc.): Verified not to crash on attached testcase. Landed on m-i on 7/12, with no attributable regressions since then.

Risk to taking this patch (and alternatives if risky): This is the lowest risk patch you'll ever see -- all it does is check a pointer for NULL before dereferencing it.

String or IDL/UUID changes made by this patch: None

Comment 13

[Adam Roach [:abr]]

Comment on attachment 774722 [details] [diff] [review]
Check that media section is found before adding rtcp-fb attributes

Obsoleting unapproved patch

Comment 14

[Adam Roach [:abr]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/777c69b17263

Comment 15

[Byron Campen [:bwc]]

sdparta removed this function