Closed
Bug 820538
Opened 11 years ago
Closed 11 years ago
WebRTC crash [@sipcc::PeerConnectionMedia::AddTransportFlow]
Categories
(Core :: WebRTC: Networking, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: posidron, Assigned: ekr)
References
Details
(Keywords: crash, Whiteboard: [WebRTC] [blocking-webrtc+] [qa-])
Crash Data
Attachments
(4 files, 1 obsolete file)
This happened during fuzzing the SDP after the "big lock" patch landed. The signature looks new to me. Tested with m-i changeset: 115674:740c8dd81b04
Reporter | ||
Comment 1•11 years ago
|
||
This is the last known SDP for that crash. Not sure how helpful this though. Since I can't reproduce it with the same packet.
Updated•11 years ago
|
Priority: -- → P1
Whiteboard: [WebRTC] [blocking-webrtc+]
Comment 2•11 years ago
|
||
I would think this is bug 820671, but it obviously got past earlier pc->media() calls in VcmCreateTransportFlow before getting to AddTransportFlow. Perhaps a combination of bug 820671 and bug 792175. ekr, what do you think? If so, this should be closed as it's not repeatable (and if we're wrong it should recur).
Assignee: nobody → ekr
Assignee | ||
Comment 3•11 years ago
|
||
Ostensibly this is post 792175. That said, it's on the main thread and so is the destruction of the media object, so I think it probably is 820671. I would close and see if it comes back
Comment 4•11 years ago
|
||
Marking this as fixed because we believe this is bug 820671, which was fixed after this report.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 5•11 years ago
|
||
void AddTransportFlow(int aIndex, bool aRtcp, mozilla::RefPtr<mozilla::TransportFlow> aFlow) { int index_inner = aIndex * 2 + (aRtcp ? 1 : 0); mTransportFlows[index_inner] = aFlow; // <- Crash } Tested with m-c changeset: 117036:f5ed2691d901
Reporter | ||
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 6•11 years ago
|
||
I notice that there is an NS_DISPATCH_SYNC in this function. We may be getting reentrancy there. A lame check would be to add a check for media() before the AddTransportFlow to see if it has been destroyed. A better improvement would be to restructure things so that the SYNC comes at the end or better yet is an ASYNC
Comment 7•11 years ago
|
||
pc->media() returns an nsRefPtr here, and it's operator->() does an NS_PRECONDITION(mRawPtr != 0, "You can't dereference a NULL nsRefPtr with operator->()."); so wouldn't we see that message in the logs if the media was destroyed at this point?
Assignee | ||
Comment 8•11 years ago
|
||
We might... But all I see here is a callstack, so I don't know. Based on reading the code, it appears that it's at least possible for this precondition not to cause a crash, but just a log message. cdiehl: can we get the complete log?
Flags: needinfo?(cdiehl)
Reporter | ||
Comment 9•11 years ago
|
||
> cdiehl: can we get the complete log?
Since the crash is not reproducible with a single test - not so easily. I am not seeing the crash right now on a regular basis but if I do, then I will try to manage to get a log.
Flags: needinfo?(cdiehl)
Comment 10•11 years ago
|
||
Added patch in any case. Can't hurt given the sync dispatch.
Attachment #697926 -
Flags: review?(rjesup)
Comment 11•11 years ago
|
||
Comment on attachment 697926 [details] [diff] [review] Media pointer check after NS_DISPATCH_SYNC Review of attachment 697926 [details] [diff] [review]: ----------------------------------------------------------------- check with ekr about if it makes sense to return NULL in this case. Otherwise, r+ (and r+ if you add flow = nullptr; if ekr says so) ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp @@ +2669,5 @@ > return NULL; > } > + if (pc->media().get()) { // test because of NS_DISPATCH_SYNC above > + pc->media()->AddTransportFlow(level, rtcp, flow); > + } Should we release the flow and return NULL in this case?
Attachment #697926 -
Flags: review?(rjesup) → review+
Updated•11 years ago
|
Flags: needinfo?(ekr)
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 697926 [details] [diff] [review] Media pointer check after NS_DISPATCH_SYNC Review of attachment 697926 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp @@ +2669,5 @@ > return NULL; > } > + if (pc->media().get()) { // test because of NS_DISPATCH_SYNC above > + pc->media()->AddTransportFlow(level, rtcp, flow); > + } IMO yes. The way you get into this state is if you have closed the PC and at this point you want the vcm code to fail.
Flags: needinfo?(ekr)
Assignee | ||
Comment 13•11 years ago
|
||
I don't think you need to release the flow explicitly, though. It's in a refptr, so it will just auto-release, right?
Comment 14•11 years ago
|
||
Agree. Changed to return NULL in shutdown situation.
Attachment #697926 -
Attachment is obsolete: true
Attachment #698210 -
Flags: review+
Attachment #698210 -
Flags: checkin?(rjesup)
Comment 15•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaede79d4d42
Target Milestone: --- → mozilla20
Updated•11 years ago
|
Attachment #698210 -
Flags: checkin?(rjesup) → checkin+
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eaede79d4d42
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Whiteboard: [WebRTC] [blocking-webrtc+] → [WebRTC] [blocking-webrtc+] [qa-]
Updated•11 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•