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

VERIFIED FIXED in Firefox 22

Status

()

Core
WebRTC
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: jesup)

Tracking

(Blocks: 1 bug, {crash, testcase, verifyme})

Trunk
mozilla24
crash, testcase, verifyme
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox22 verified, firefox23 verified, firefox24 verified)

Details

(Whiteboard: [WebRTC][blocking-webrtc-], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 754983 [details]
testcase (usually crashes within 10 seconds)

With:
  user_pref("media.peerconnection.enabled", true);
Confirmed on my Win 7 machine.
https://crash-stats.mozilla.com/report/index/bp-9695cf61-5322-455d-876d-8281b2130528
Crash Signature: [@ mozilla::dom::MediaStreamList::Length()]
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

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Comment 5

5 years ago
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
Whiteboard: [WebRTC][blocking-webrtc-][webrtc-uplift]
(Assignee)

Comment 6

5 years ago
Created attachment 755025 [details] [diff] [review]
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)
(Assignee)

Comment 7

5 years ago
Created attachment 755037 [details] [diff] [review]
check if PeerConnection is closed before accessing MediaStreamList
(Assignee)

Updated

5 years ago
Attachment #755025 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #755037 - Flags: review?(ekr)

Comment 8

5 years ago
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.
Attachment #755037 - Flags: review?(ekr) → review+
(Assignee)

Comment 9

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

Fix is very safe; we should consider Beta uplift
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
Target Milestone: --- → mozilla24
https://hg.mozilla.org/mozilla-central/rev/324169b939ae
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Flags: in-testsuite?
Keywords: verifyme
(Assignee)

Comment 11

5 years ago
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
Attachment #755037 - Flags: approval-mozilla-beta?
Attachment #755037 - Flags: approval-mozilla-aurora?

Updated

5 years ago
status-firefox24: affected → fixed
lgtm on 6/1 trunk build - test case isn't crashing after about 20 runs.
Status: RESOLVED → VERIFIED
status-firefox24: fixed → verified
Keywords: verifyme

Updated

5 years ago
Attachment #755037 - Flags: approval-mozilla-beta?
Attachment #755037 - Flags: approval-mozilla-beta+
Attachment #755037 - Flags: approval-mozilla-aurora?
Attachment #755037 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc-][webrtc-uplift] → [WebRTC][blocking-webrtc-]
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
status-firefox22: fixed → verified
Keywords: verifyme

Updated

5 years ago
QA Contact: jsmith
Verified fixed with Firefox 23 beta 1 (build ID: 20130625125232) on : Ubuntu 12.10 32bit, Mac OSX 10.8.3 and Win 7 64bit.
status-firefox23: fixed → verified
You need to log in before you can comment on or make changes to this bug.