829461 - WebRTC crash [@nsDOMMediaStream::GetHintContents]

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

I am seeing this crash only from time to time while trying to fuzz the SDP.

Tested with m-c changeset: 118260:f60b87eed1ac

Comment 1

[Christoph Diehl [:posidron]]

Attachment: NSPR_LOG

Comment 2

[Adam Roach [:abr]]

(In reply to Christoph Diehl [:cdiehl] from comment #0)
> Created attachment 700894 [details]
> callstack
> 
> I am seeing this crash only from time to time while trying to fuzz the SDP.

Can you attach the template you're using? Is there any particular seed that triggers this crash?

Comment 3

[Christoph Diehl [:posidron]]

Hi Adam, I am using the same template as in https://bugzilla.mozilla.org/show_bug.cgi?id=828147 there is no particular seed to reproduce this issue, it happens randomly from time to time.

Comment 4

[Adam Roach [:abr]]

Okay, reading the code, it looks like you've managed to trigger some path in which the remote stream pointer (type RemoteSourceStreamInfo) is non-null, but calling its GetMediaStream() method returns a null.  I think the most straightforward fix would be to treat this set of circumstances equivalent to the remote stream being null.

Comment 5

[Adam Roach [:abr]]

Assuming we're not seeing memory corruption, this condition appears to arise from a race wherein RemoteSourceStreamInfo::Detatch() is being called before the REMOTESTREAMADD event comes back from the SIPCC layer. Given that this can only happen if the associated stream is going away, I believe it's safe to simply not process the event under these circumstances.

Comment 6

[Adam Roach [:abr]]

Attachment: Check for NULL media stream

Comment 7

[Adam Roach [:abr]]

Christoph: can you try applying the attached patch and see if it prevents this crash in your fuzzing?

Comment 8

[Christoph Diehl [:posidron]]

Till now I haven't seen that crash again, although it didn't pop up that much before.

Comment 9

[Adam Roach [:abr]]

Christoph: Okay, thanks for checking. I'm pretty comfortable with my analysis of what's going on here, so I'm going to go ahead and get this patch landed.

Comment 10

[Ethan Hugg [:ehugg]]

Comment on attachment 703050 [details] [diff] [review]
Check for NULL media stream

Review of attachment 703050 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +176,5 @@
>              stream = mRemoteStream->GetMediaStream();
> +          }
> +
> +          if (!stream) {
> +            CSFLogErrorS(logTag, __FUNCTION__ << " GetMediaStream returned NULL");

Doesn't need changing, but I wanted to remind you than these stream version log functions only work with DEBUG on.  Streams were deemed to be a bad thing for release.  So if you are testing  and looking for this log message you'll need to be an a debug build.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +161,5 @@
>  class LocalSourceStreamInfo {
>  public:
>    LocalSourceStreamInfo(nsDOMMediaStream* aMediaStream)
> +    : mMediaStream(aMediaStream) {
> +      PR_ASSERT(aMediaStream);

I believe MOZ_ASSERT is the preferred one to use here

@@ +200,5 @@
>   public:
>    RemoteSourceStreamInfo(nsDOMMediaStream* aMediaStream) :
>      mMediaStream(already_AddRefed<nsDOMMediaStream>(aMediaStream)),
> +    mPipelines() {
> +      PR_ASSERT(aMediaStream);

MOZ_ASSERT

Comment 11

[Adam Roach [:abr]]

(In reply to Ethan Hugg [:ehugg] from comment #10)

> Doesn't need changing, but I wanted to remind you than these stream version
> log functions only work with DEBUG on.  Streams were deemed to be a bad
> thing for release.  So if you are testing  and looking for this log message
> you'll need to be an a debug build.

Thanks for the heads-up. I think having these only in debug is fine for now.

Comment 12

[Adam Roach [:abr]]

Attachment: Check for NULL media stream

Comment 13

[Adam Roach [:abr]]

Comment on attachment 703574 [details] [diff] [review]
Check for NULL media stream

Carrying r+ forward from ehugg

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea6b3f0cdd25

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/ea6b3f0cdd25