825526 - crash in RtlpWaitOnCriticalSection | RtlpDeCommitFreeBlock | PR_Lock | mozilla::SourceMediaStream::Finish() when quitting firefox with gum video only not attached to a media element

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Jason Smith [:jsmith]]

Attachment: Test Case

This bug was filed from the Socorro interface and is 
report bp-35e5ddb4-32f3-4e98-ba18-c3ab72121230 .
============================================================= 

Steps:

1. Load the attached test case html file
2. Select "Turn on local video 2 with gum video only"
3. Allow permissions to your camera
4. Quit Firefox

Expected:

The camera should be released - no crash should occur.

Actual:

A crash on shutdown is seen.

Comment 1

[Jason Smith [:jsmith]]

0	ntdll.dll	RtlpWaitOnCriticalSection	
1	ntdll.dll	RtlpDeCommitFreeBlock	
2	nspr4.dll	PR_Lock	nsprpub/pr/src/threads/combined/prulock.c:201
3	xul.dll	mozilla::SourceMediaStream::Finish	obj-firefox/dist/include/MediaStreamGraph.h:556

Comment 2

[Jason Smith [:jsmith]]

Another way to reproduce this:

1. Go to http://mozilla.github.com/webrtc-landing/pc_test.html
2. Select Start - wait until hip hip horray is seen
3. Reload the page

Result - You get the same crash

Comment 3

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

Attachment: Improve lifetime control of SourceMediaStream in gUM

I suspect this patch solves this problem as well as bug 825325.  I've tried a bunch with this patch with no problems, but without this patch I also don't have problems with pc_test.html.  I pushed a try build for you to test:  https://tbpl.mozilla.org/?tree=Try&rev=11d8c2ea3989

Comment 4

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

Comment on attachment 696693 [details] [diff] [review]
Improve lifetime control of SourceMediaStream in gUM

This also fixes bug 825325.  The changes are much smaller than the diff appears; I re-ordered MediaOperationRunnable and GetUserMediaCallbackMediaStreamListener.  The main change was to pass a reference to the listener instead of a raw sourcemediastream, and when the runnable needs the SourceMediaStream it calls mListener->GetSourceStream()

While I was unable to reproduce on Linux, on Mac I was able to repro this at will, and with the patch (which applies on top of bug 811757 and the bug 825514 patches) I was unable to cause the crash.

Comment 5

[Anant Narayanan [:anant]]

Comment on attachment 696693 [details] [diff] [review]
Improve lifetime control of SourceMediaStream in gUM

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

Looks good to me!

::: dom/media/MediaManager.cpp
@@ +1104,5 @@
> +{
> +  nsRefPtr<MediaOperationRunnable> runnable;
> +  // We can't take a chance on blocking here, so proxy this to another
> +  // thread.
> +  // Pass a ref to us (which is threadsafe) so it can query us for the 

Nit: trailing space

@@ +1107,5 @@
> +  // thread.
> +  // Pass a ref to us (which is threadsafe) so it can query us for the 
> +  // source stream info.
> +  runnable = new MediaOperationRunnable(MEDIA_STOP,
> +                                        this, mAudioSource, mVideoSource);

Doesn't the earlier comment about passing a raw pointer across still apply? (bug 825235)

Comment 6

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

(In reply to Anant Narayanan [:anant] from comment #5)

> > +  // Pass a ref to us (which is threadsafe) so it can query us for the 
> > +  // source stream info.
> > +  runnable = new MediaOperationRunnable(MEDIA_STOP,
> > +                                        this, mAudioSource, mVideoSource);
> 
> Doesn't the earlier comment about passing a raw pointer across still apply?
> (bug 825235)

'this' (the listener) gets refcounted, unlike the old SourceMediaStream

Thanks!

Comment 7

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

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Jason Smith [:jsmith]]

Verified on 1/25 - not seeing a crash.