Closed
Bug 837523
Opened 12 years ago
Closed 12 years ago
Crash in gsm_sdp.c:4602 due to assertion failure: pc_stream_id == 0 when having a local PC call over two different PCs
Categories
(Core :: WebRTC: Signaling, defect, P1)
Core
WebRTC: Signaling
Tracking
()
VERIFIED
FIXED
mozilla22
People
(Reporter: jsmith, Assigned: abr)
Details
(Keywords: crash, testcase, Whiteboard: [WebRTC][blocking-webrtc+][qa-])
Attachments
(3 files, 1 obsolete file)
Setup: 1. Debug build of Nightly 2. Add the patch from bug 837458 3. Add the attached mochitest to your makefile 4. Run it Expected: We shouldn't crash. Actual: We crash at the end of the 2nd handshake for pc local --> pc remote 2 with: Assertion failure: pc_stream_id == 0 at gsm_sdp.c:4602
Reporter | ||
Comment 2•12 years ago
|
||
Comment 3•12 years ago
|
||
The underlying problem is the attempt to rehandshake and that PC isn't handling rehandshake properly. This shouldn't crash, but what we need to do is disable rehandshake for now.
Reporter | ||
Comment 5•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #4) > A simplified crashtest would be nice to have here. I don't think that's necessary nor the right test to be written here. The mochitest here would be something that would be the thing to check in once the other refactoring patch is complete. That would cover the case mentioned here.
Updated•12 years ago
|
Assignee: nobody → adam
Assignee | ||
Updated•12 years ago
|
Priority: -- → P1
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #718105 -
Flags: review?(ethanhugg)
Assignee | ||
Comment 8•12 years ago
|
||
Let's make sure I didn't break anything here... https://tbpl.mozilla.org/?tree=Try&rev=a67f3291d60a
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Comment 9•12 years ago
|
||
Comment on attachment 718105 [details] [diff] [review] Additional checks for unsupported app behavior Review of attachment 718105 [details] [diff] [review]: ----------------------------------------------------------------- Look good to me. Built/tested on Linux64, no new warnings. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +964,5 @@ > + DOMMediaStream* stream = static_cast<DOMMediaStream*>(aMediaStream); > + uint32_t hints = stream->GetHintContents(); > + > + // XXX Remove this check once addStream has an error callback > + // available and/or we have pliumbing to handle multiple plumbing, same below. @@ +1017,5 @@ > uint32_t hints = stream->GetHintContents(); > > if (hints & DOMMediaStream::HINT_CONTENTS_AUDIO) { > mCall->removeStream(stream_id, 0, AUDIO); > + mNumAudioStreams--; I would MOZ_ASSSERT(mNumAudioStreams > 0) here even thought I can't yet see a way it could be zero at this point.
Attachment #718105 -
Flags: review?(ethanhugg) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #718105 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 718222 [details] [diff] [review] Additional checks for unsupported app behavior Carrying forward r+ from ehugg. Changes based on review. Also, added null check to fix crashtest crashes on 64-bit debug builds.
Attachment #718222 -
Flags: review+
Assignee | ||
Comment 12•12 years ago
|
||
New try for new patch (double-checking crashtest fix): https://tbpl.mozilla.org/?tree=Try&rev=20bb58affe0c
Assignee | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cbb040ed6da
Reporter | ||
Comment 14•12 years ago
|
||
*looks at patch* We really should be a bit more careful not overusing the NS_ERROR_FAILURE flag. From the developer's perspective, that tells them nothing. I don't think that falls in alignment with the W3C Spec here for error handling either - http://dev.w3.org/2011/webrtc/editor/webrtc.html#error-handling. You should be throwing RTCError exceptions in this case with useful information - a name and a message. If it's unsupported, then throw an exception that says that. That way, we at least give a clear indication to the developer that this is an error case, why it's an error case, etc.
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #14) > *looks at patch* > > We really should be a bit more careful not overusing the NS_ERROR_FAILURE > flag. From the developer's perspective, that tells them nothing. I don't > think that falls in alignment with the W3C Spec here for error handling > either - http://dev.w3.org/2011/webrtc/editor/webrtc.html#error-handling. > You should be throwing RTCError exceptions in this case with useful > information - a name and a message. If it's unsupported, then throw an > exception that says that. That way, we at least give a clear indication to > the developer that this is an error case, why it's an error case, etc. Jason: Error handling is a known shortcoming right now, and one that I consider important -- see my comments on Bug 834270. Attempting to throw exceptions piecemeal right now doesn't help the situation either, for reasons that I haven't had the opportunity to troubleshoot yet; see bug 834933. We *do* all recognize that the error handling situation is pretty horrible right now, and we have bugs open to address those shortcomings (and, in fact, error handling is the number-one-priority task I'm going to dive into once I've dispatched all my [blocking-webrtc+] bugs). That said, fixing the error reporting is a pretty big task, I'd rather not hold up fixing this crasher bug on something that will probably take days or weeks to finish.
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9cbb040ed6da
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Reporter | ||
Updated•12 years ago
|
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-]
Comment 17•12 years ago
|
||
Works fine with latest mc checkout 123218:b8b5a44ab168.
Status: RESOLVED → VERIFIED
Reporter | ||
Updated•12 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•