876856 - Crash [@ mozilla::dom::MediaStreamList::Length] after closing a peer connection

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Jesse Ruderman]

Attachment: testcase (usually crashes within 10 seconds)

With:
  user_pref("media.peerconnection.enabled", true);

Comment 1

[Jason Smith [:jsmith]]

Confirmed on my Win 7 machine.

Comment 2

[Jason Smith [:jsmith]]

https://crash-stats.mozilla.com/report/index/bp-9695cf61-5322-455d-876d-8281b2130528

Comment 3

[Jason Smith [:jsmith]]

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::MediaStreamList::Length 	media/webrtc/signaling/src/peerconnection/MediaStreamList.cpp:78
1 	xul.dll 	mozilla::dom::MediaStreamListBinding::get_length 	obj-firefox/dom/bindings/MediaStreamListBinding.cpp:26
2 	xul.dll 	mozilla::dom::MediaStreamListBinding::genericGetter 	obj-firefox/dom/bindings/MediaStreamListBinding.cpp:62
3 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:441
4 	mozjs.dll 	JS_ForwardGetPropertyTo 	js/src/jsapi.cpp:4277
5 	xul.dll 	mozilla::dom::GetPropertyOnPrototype 	dom/bindings/BindingUtils.cpp:1261
6 	xul.dll 	mozilla::dom::MediaStreamListBinding::DOMProxyHandler::get 	obj-firefox/dom/bindings/MediaStreamListBinding.cpp:367
7 	mozjs.dll 	proxy_GetGeneric 	js/src/jsproxy.cpp:2811
8 	mozjs.dll 	js::GetPropertyOperation 	js/src/jsinterpinlines.h:290
9 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2067
10 	mozjs.dll 	js::ion::CanEnter 	js/src/ion/Ion.cpp:1695
11 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:332
12 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:441
13 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5871
14 	xul.dll 	mozilla::dom::Function::Call 	obj-firefox/dom/bindings/FunctionBinding.cpp:41
15 	xul.dll 	mozilla::dom::Function::Call<nsCOMPtr<nsISupports> > 	obj-firefox/dist/include/mozilla/dom/FunctionBinding.h:52
16 	xul.dll 	nsGlobalWindow::RunTimeoutHandler 	dom/base/nsGlobalWindow.cpp:10176
17 	xul.dll 	nsGlobalWindow::RunTimeout 	dom/base/nsGlobalWindow.cpp:10414
18 	xul.dll 	nsGlobalWindow::TimerCallback 	dom/base/nsGlobalWindow.cpp:10661
19 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:547
20 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:634
21 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
22 	xul.dll 	NS_ProcessNextEvent 	obj-firefox/xpcom/build/nsThreadUtils.cpp:238
23 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:82
24 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:212
25 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:186
26 	xul.dll 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
27 	xul.dll 	nsAppShell::Run 	widget/windows/nsAppShell.cpp:113
28 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:269
29 	xul.dll 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3872
30 	xul.dll 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3939
31 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4151
32 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp:272
33 	firefox.exe 	NS_internal_main 	browser/app/nsBrowserApp.cpp:632
34 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:105
35 	firefox.exe 	__tmainCRTStartup 	crtexe.c:552
36 	kernel32.dll 	BaseThreadInitThunk 	
37 	ntdll.dll 	__RtlUserThreadStart 	
38 	ntdll.dll 	_RtlUserThreadStart

Comment 4

[Jason Smith [:jsmith]]

http://hg.mozilla.org/mozilla-central/annotate/6a739f41d1ba/media/webrtc/signaling/src/peerconnection/MediaStreamList.cpp#l78

Comment 5

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

The MediaStreamList code isn't checking to see if the PeerConnection has been closed (which causes the MediaStream lists and rest of the PCMedia object to be released).

Blocking- as this is a safe non-deref failure in an edge case.  We'd likely uplift this fix to Aurora; the fix should be safe enough we could consider Beta uplift

Comment 6

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

Attachment: check if PeerConnection is closed before accessing MediaStreamList

This fix appears to work and throws warnings as expected (probably we should remove the warnings before landing)

Comment 7

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

Attachment: check if PeerConnection is closed before accessing MediaStreamList

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 755037 [details] [diff] [review]
check if PeerConnection is closed before accessing MediaStreamList

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

This looks OK, but I tend to agree you should get rid of the NS_WARNINGs.

Comment 9

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

https://hg.mozilla.org/projects/cypress/rev/324169b939ae
With NS_WARNINGS removed

Fix is very safe; we should consider Beta uplift

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/324169b939ae

Comment 11

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

Comment on attachment 755037 [details] [diff] [review]
check if PeerConnection is closed before accessing MediaStreamList

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Race conditions that can cause crashes (null derefs)

Testing completed (on m-c, etc.): on m-c.  

Risk to taking this patch (and alternatives if risky): None.  Adds error checking and safe error returns.

String or IDL/UUID changes made by this patch: none

Comment 12

[Jason Smith [:jsmith]]

lgtm on 6/1 trunk build - test case isn't crashing after about 20 runs.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7c64168fea7b
https://hg.mozilla.org/releases/mozilla-beta/rev/10caf14d9013

Comment 14

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 22 beta 4:
Mozilla/5.0 (Windows NT 6.1; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0
Build ID: 20130605070403

Comment 15

[Manuela Muntean [Away]]

Verified fixed with Firefox 23 beta 1 (build ID: 20130625125232) on : Ubuntu 12.10 32bit, Mac OSX 10.8.3 and Win 7 64bit.