822196 - WebRTC crash [@sipcc::PeerConnectionMedia::AddStream]

Status: RESOLVED DUPLICATE of bug 822158

(Core :: WebRTC, defect, P1)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

PeerConnectionMedia.cpp:176
[...]
  // Now see if we already have a stream of this type, since we only
  // allow one of each.
  // TODO([e-mail]): remove this when multiple of each stream
  // is allowed
  PR_Lock(mLocalSourceStreamsLock);
[...]

PeerConnectionImpl.cpp:777
[...]
  nsresult res = mMedia->AddStream(aMediaStream, &stream_id);
[...]


#0 0x108be8fa0 in sipcc::PeerConnectionMedia::AddStream PeerConnectionMedia.cpp:176
#1 0x108bdd86a in sipcc::PeerConnectionImpl::AddStream PeerConnectionImpl.cpp:777
...
#32 0x108bde523 in sipcc::PeerConnectionImpl::ShutdownMedia PeerConnectionImpl.cpp:974
#33 0x108bd8e1c in sipcc::PeerConnectionImpl::CloseInt PeerConnectionImpl.cpp:949
#34 0x108bde1cb in sipcc::PeerConnectionImpl::Close PeerConnectionImpl.cpp:931


@rforbes
SEED: 1355728238.95
Fault was detected on test 131

Tested with m-i changeset: 116243:9d6e95e77855

Comment 1

[Eric Rescorla (:ekr)]

Hmm.... 

The JS is supposed to enforce that after you call Close() you don't get to call AddStream().

Reassigning to jesup, though maybe jib would be better.

Comment 2

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

This would be enforced (likely) in PeerConnectionImpl, or perhaps in PeerConnection.js

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

From the callstack, the code and Bug 822158, my analysis is that this race-condition happens when the window is destroyed at the same time that nICEr completes it's initial gathering stage (IceGatheringCompleted, which is a getting-started stage, not a shutdown stage).

In response to the window being torn down, PeerConnection.js calls _pc.close() (which to jump ahead, never comes back).

At the same time, the prolific signaling thread dispatches an IceGatheringCompleted runnable asynchronously to the main thread. The main thread has time to spare, because it happens to be waiting for its own SYNC PeerConnectionMedia::ShutdownMediaTransport runnable to come back from, ironically, the signaling thread, at this point.

So the main thread runs IceGatheringCompleted while waiting for ShutdownMediaTransport.

Now, IceGatheringCompleted_m calls (and this call is #ifdef'ed MOZILLA_INTERNAL_API for some reason) IPeerConnectionObserver::OnStateChange, which is a JS hook, with kIceState=kIceWaiting.

So from a JS perspective, the net result of all of this is that PeerConnection.js gets an 'onStateChange:'-callback from within _pc.close(), which seems unexpected. This JS callback seems to be "starting" things in the case of kIceWaiting, and I assume it is responsible for eventually calling the AddStream() that crashes, though I wasn't able to follow the JS code further to prove it (it seemed wrong already so I stopped here).

I suppose we could fix the JS to anticipate this odd sequence, though this seems unreasonable.

The best fix may be to squelch the sending of OnStateChange from PC in this case (i.e. detect that PC is in shutdown). I'll try that next unless someone knows of a reason why that would be bad.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

After discussing with ekr, we believe this test is already in there, thanks to Bug 822158, which landed 12/23 (after the symptom here).

cdiehl, if you could re-test that would be great, then if it works I'll close this as a duplicate. Thanks.

Comment 5

[Christoph Diehl [:posidron]]

> cdiehl, if you could re-test that would be great, then if it works I'll
> close this as a duplicate. Thanks.

I am not seeing this crash anymore with m-i changeset: 117317:4983b42d58c9 and m-c changeset: 117373:38407b98003b