Closed Bug 820978 Opened 11 years ago Closed 11 years ago

Crash when closing Firefox while gUM() call is active [@ PR_Unlock][@ mozilla::MediaEngineWebRTC::EnumerateAudioDevices]

Categories

(Core :: WebRTC, defect, P1)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox19 --- disabled
firefox20 --- verified
firefox-esr17 --- unaffected
b2g18 --- disabled

People

(Reporter: whimboo, Assigned: jesup)

Details

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

Crash Data

Attachments

(1 file, 1 obsolete file)

As I have mentioned on bug 818801 it takes a long time until the gUM notification comes up. When I quit Firefox before that time it crashes.

Steps:
1. Open attachment 689100 [details]
2. Click start
3. Quit Firefox immediately

Report: bp-b8fa9756-26b7-4af8-930a-18b402121212

Stack:

0 	libnspr4.dylib 	PR_Unlock 	ptsynch.c:199
1 	XUL 	mozilla::MediaEngineWebRTC::EnumerateAudioDevices 	Mutex.h:83
2 	XUL 	mozilla::GetUserMediaDevicesRunnable::Run 	MediaManager.cpp:650
3 	XUL 	nsThread::ProcessNextEvent 	nsThread.cpp:627
4 	XUL 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:221
5 	XUL 	nsThread::ThreadFunc 	nsThread.cpp:265
6 	libnspr4.dylib 	_pt_root 	ptthread.c:156
7 	libsystem_c.dylib 	libsystem_c.dylib@0x4e8be 	
8 	libsystem_c.dylib 	libsystem_c.dylib@0x51b74 	
9 	libnspr4.dylib 	libnspr4.dylib@0x1dacf
Whiteboard: [WebRTC] → [getUserMedia]
Crash Signature: [@ PR_Unlock] [@ mozilla::MediaEngineWebRTC::EnumerateAudioDevices] → [@ PR_Unlock | mozilla::MediaEngineWebRTC::EnumerateAudioDevices(nsTArray<nsRefPtr<mozilla::MediaEngineAudioSource>, nsTArrayDefaultAllocator>*)]
(In reply to Henrik Skupin (:whimboo) from comment #0)

Updated Steps:
1. Open attachment 689100 [details]
2. Click the start button twice (right after each other)
3. Quit Firefox immediately

With those I can reproduce the crash all the time.
With a debug build I get the following stack. Scoobidiver, what would be the correct signature here?

Program received signal SIGABRT, Aborted.
0x00007fff8c792ce2 in __pthread_kill ()
(gdb) bt
#0  0x00007fff8c792ce2 in __pthread_kill ()
#1  0x00007fff894a57d2 in pthread_kill ()
#2  0x00007fff89496a7a in abort ()
#3  0x000000010008261b in PR_Assert (s=<value temporarily unavailable, due to optimizations>, file=<value temporarily unavailable, due to optimizations>, ln=<value temporarily unavailable, due to optimizations>) at /Volumes/data/code/firefox/nightly/nsprpub/pr/src/io/prlog.c:554
#4  0x000000010009560b in __inline_memset_chk [inlined] () at /usr/include/secure/_string.h:162
#5  0x000000010009560b in PR_DestroyLock (lock=0x124ab5d60) at /Volumes/data/code/firefox/nightly/nsprpub/pr/src/pthreads/ptsynch.c:164
#6  0x0000000101bdb286 in mozilla::Mutex::~Mutex () at /Volumes/data/code/firefox/obj/debug/dist/include/mozilla/Mutex.h:62
#7  0x0000000101bdb286 in mozilla::MediaEngineWebRTC::~MediaEngineWebRTC (this=0x120646cf0) at Mutex.h:58
#8  0x0000000101bdb1fe in mozilla::MediaEngineWebRTC::~MediaEngineWebRTC () at /Volumes/data/code/firefox/obj/debug/dist/include/MediaEngineWebRTC.h:237
#9  0x0000000101bdb1fe in mozilla::MediaEngineWebRTC::~MediaEngineWebRTC (this=0x120646cf0) at mozalloc.h:237
#10 0x0000000101bde386 in mozilla::MediaManager::~MediaManager (this=0x107aec8a0) at MediaManager.h:332
11 0x0000000101bd8274 in nsAutoRefCnt::operator= () at /Volumes/data/code/firefox/obj/debug/dist/include/nsISupportsImpl.h:331
#12 0x0000000101bd8274 in mozilla::MediaManager::Release (this=0x107aec8a0) at mozalloc.h:688
#13 0x00000001028a22af in ReleaseObjects (aElement=<value temporarily unavailable, due to optimizations>) at nsCOMArray.cpp:134
#14 0x00000001028a6bef in nsVoidArray::EnumerateForwards (this=0x7fff5fbfeb20, aFunc=0x1028a22a0 <ReleaseObjects>, aData=0x0) at nsVoidArray.cpp:690
#15 0x00000001028a1ebe in nsAutoVoidArray::~nsAutoVoidArray () at /Volumes/data/code/firefox/nightly/xpcom/glue/nsVoidArray.h:144
#16 0x00000001028a1ebe in nsAutoVoidArray::~nsAutoVoidArray () at nsCOMArray.cpp:145
#17 0x00000001028a1ebe in nsCOMArray_base::Clear () at /Volumes/data/code/firefox/nightly/xpcom/glue/nsCOMArray.h:171
#18 0x00000001028a1ebe in nsCOMArray_base::~nsCOMArray_base (this=0x7fff5fbfeb90) at nsCOMArray.cpp:25
#19 0x00000001028c46c7 in nsObserverList::NotifyObservers (this=<value temporarily unavailable, due to optimizations>, aSubject=<value temporarily unavailable, due to optimizations>, aTopic=<value temporarily unavailable, due to optimizations>, someData=<value temporarily unavailable, due to optimizations>) at /Volumes/data/code/firefox/nightly/xpcom/ds/nsObserverList.cpp:101
#20 0x00000001028c5934 in nsTHashtable<nsObserverList>::GetEntry () at /Volumes/data/code/firefox/obj/debug/dist/include/nsTHashtable.h:161
#21 0x00000001028c5934 in nsObserverService::NotifyObservers (this=0x1079203c0, aSubject=0x10012b598, aTopic=0x1035c1c58 "xpcom-shutdown", someData=0x0) at /Volumes/data/code/firefox/nightly/xpcom/ds/nsObserverService.cpp:164
Assignee: nobody → rjesup
Whiteboard: [getUserMedia] → [getUserMedia][blocking-gum+]
Priority: -- → P1
I'm not seeing an assert, but I did get a deadlock apparently:

(gdb) where
#0  __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:136
#1  0x0000003667809c8c in _L_lock_1024 () from /lib64/libpthread.so.0
#2  0x0000003667809c35 in __pthread_mutex_lock (mutex=0x7fffda858ed0) at pthread_mutex_lock.c:105
#3  0x00007ffff7adec2e in PR_Lock (lock=0x7fffda858ed0)
    at ../../../../../nsprpub/pr/src/pthreads/ptsynch.c:174
#4  0x00007ffff3800882 in mozilla::Mutex::Lock (this=0x7fffd01fb4c8)
    at ../../../xpcom/glue/BlockingResourceBase.cpp:228
#5  0x00007ffff1ba94f8 in mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock (this=0x7fffcd9f7ba0, 
    aLock=..., _notifier=...) at ../../dist/include/mozilla/Mutex.h:153
#6  0x00007ffff2c1e3e0 in mozilla::MediaEngineWebRTC::EnumerateAudioDevices (this=0x7fffd01fb4c0, 
    aASources=0x7fffcd9f7c50) at ../../../../content/media/webrtc/MediaEngineWebRTC.cpp:142
#7  0x00007ffff291c779 in mozilla::GetUserMediaDevicesRunnable::Run (this=0x7fffd83fdfd0)
    at ../../../dom/media/MediaManager.cpp:658
Status: NEW → ASSIGNED
Closing as security-sensitive, as in frame 7:

#7  0x00007ffff291c779 in mozilla::GetUserMediaDevicesRunnable::Run (this=0x7fffd83fdfd0)
    at ../../../dom/media/MediaManager.cpp:658
658	    manager->GetBackend()->EnumerateAudioDevices(&audioSources);
(gdb) p *manager
$6 = {<nsIMediaManagerService> = {<nsISupports> = {_vptr.nsISupports = 
    0x5a5a5a5a5a5a5a5a}, <No data fields>}, <nsIObserver> = {<nsISupports> = {_vptr.nsISupports = 
    0x5a5a5a5a5a5a5a5a}, <No data fields>}, mRefCnt = {mValue = 1515870810}, _mOwningThread = {
    mThread = 0x5a5a5a5a5a5a5a5a}, mActiveWindows = 

This says we're using freed memory for a function call (though in this case we deadlocked first, and the manager object was freed while we were waiting, but very likely the timing could be tweaked to expose the problem).
Group: core-security
Attachment #695503 - Flags: review?(tterribe)
Comment on attachment 695503 [details] [diff] [review]
keep reference to MediaManager singleton

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

I think you need to do this for GetUserMediaStreamRunnable, ErrorCallbackRunnable, and SuccessCallbackRunnable as well. Otherwise they can potentially re-create the singleton after it's set to nsnull in the shutdown observer.
Attachment #695503 - Flags: review?(tterribe) → review-
Attachment #695503 - Attachment is obsolete: true
Comment on attachment 696044 [details] [diff] [review]
keep reference to MediaManager singleton

Now does it for all of them.
Attachment #696044 - Flags: review?(tterribe)
Comment on attachment 696044 [details] [diff] [review]
keep reference to MediaManager singleton

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

LGTM.
Attachment #696044 - Flags: review?(tterribe) → review+
Was merged to m-c but not marked
https://hg.mozilla.org/integration/mozilla-inbound/rev/43187d64eea3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Keywords: verifyme
Verified on 12/30 with specified test case steps.
Status: RESOLVED → VERIFIED
Keywords: verifyme
(In reply to Randell Jesup [:jesup] from comment #11)
> Was merged to m-c but not marked
> https://hg.mozilla.org/integration/mozilla-inbound/rev/43187d64eea3

I believe you meant to add the changelog from m-c which is:
http://hg.mozilla.org/mozilla-central/rev/43187d64eea3
Flags: in-testsuite-
Flags: in-qa-testsuite?
Jason, please add my name when requesting in-qa-testsuite. Thanks.
Flags: in-qa-testsuite? → in-qa-testsuite?(hskupin)
As a WebRTC, this is disabled in 19 and earlier behind a pref, so 19 and earlier are unaffected, right?
Marking this as '19 disabled.'
Whiteboard: [getUserMedia][blocking-gum+] → [getUserMedia][blocking-gum+][adv-main20-]
Group: core-security
Nils, can you have a look and check which type of test would be good here? Not sure if a crashtest works with the requirements we need for webrtc code.
Flags: in-qa-testsuite?(hskupin) → in-qa-testsuite?(drno)
Oh, I see it's for a shutdown crash, so Mozmill would then be necessary. Nils, do you think it's worth creating one?
Flags: needinfo?(drno)
Flags: in-qa-testsuite?(drno)
Flags: in-qa-testsuite?
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=799142#c54 for my request of writing one Mozmill test to cover these scenarios.
Flags: needinfo?(drno)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: