837523 - Crash in gsm_sdp.c:4602 due to assertion failure: pc_stream_id == 0 when having a local PC call over two different PCs

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

Attachment: Mochitest

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

Comment 1

[Jason Smith [:jsmith]]

100% reproducible btw with the mochitest given.

Comment 2

[Jason Smith [:jsmith]]

Attachment: Output

Comment 3

[Eric Rescorla (:ekr)]

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.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

A simplified crashtest would be nice to have here.

Comment 5

[Jason Smith [:jsmith]]

(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.

Comment 6

[Christoph Diehl [:posidron]]

This is blocking a bit the fuzzing process of JSEP.

Comment 7

[Adam Roach [:abr]]

Attachment: Additional checks for unsupported app behavior

Comment 8

[Adam Roach [:abr]]

Let's make sure I didn't break anything here...
https://tbpl.mozilla.org/?tree=Try&rev=a67f3291d60a

Comment 9

[Ethan Hugg [:ehugg]]

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.

Comment 10

[Adam Roach [:abr]]

Attachment: Additional checks for unsupported app behavior

Comment 11

[Adam Roach [:abr]]

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.

Comment 12

[Adam Roach [:abr]]

New try for new patch (double-checking crashtest fix):
https://tbpl.mozilla.org/?tree=Try&rev=20bb58affe0c

Comment 13

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cbb040ed6da

Comment 14

[Jason Smith [:jsmith]]

*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.

Comment 15

[Adam Roach [:abr]]

(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

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9cbb040ed6da

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Works fine with latest mc checkout 123218:b8b5a44ab168.