Closed Bug 825526 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla20

People

(Reporter: jsmith, Assigned: jesup)

Details

(Keywords: crash, testcase, Whiteboard: [getUserMedia][blocking-gum+])

Crash Data

Attachments

(2 files)

Attached file 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.
Keywords: testcase
Whiteboard: [getUserMedia][blocking-gum+]
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
Assignee: nobody → rjesup
Priority: -- → P1
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
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 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.
Attachment #696693 - Flags: review?(tterribe)
Flags: in-testsuite?
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)
Attachment #696693 - Flags: review?(tterribe) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/b3993179ea52
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
Verified on 1/25 - not seeing a crash.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.