Closed Bug 892911 Opened 7 years ago Closed 5 years ago

WebRTC crash [@gsmsdp_negotiate_media_lines]

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox24 --- fixed
firefox25 --- fixed

People

(Reporter: posidron, Assigned: abr)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [leave-open])

Crash Data

Attachments

(4 files, 1 obsolete file)

Attached file testcase
Tested with hg.mozilla.org/integration/mozilla-inbound/rev/60865db5f5dc
Attached file callstack
Attached file console.txt
Assignee: nobody → adam
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.
Attachment #774570 - Flags: review?(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?
There's a space after @ in the signature so that it shows up in crash stats.
Crash Signature: [@ gsmsdp_negotiate_media_lines]
Attachment #774570 - Attachment is obsolete: true
Attachment #774570 - Flags: review?(ekr)
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.
Attachment #774722 - Flags: review?(ekr)
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.
Whiteboard: [leave-open]
Comment on attachment 774722 [details] [diff] [review]
Check that media section is found before adding rtcp-fb attributes

Setting r+ from IRC conversation
Attachment #774722 - Flags: review?(ekr) → review+
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
Attachment #774570 - Attachment is obsolete: false
Attachment #774570 - Flags: review+
Attachment #774570 - Flags: approval-mozilla-aurora?
Comment on attachment 774722 [details] [diff] [review]
Check that media section is found before adding rtcp-fb attributes

Obsoleting unapproved patch
Attachment #774722 - Attachment is obsolete: true
Attachment #774722 - Flags: review+
Attachment #774570 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-testsuite?
Depends on: sdparta
sdparta removed this function
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.