crash in webrtc::videocapturemodule::DeviceInfoDS::GetDeviceInfo

VERIFIED FIXED in Firefox 28

Status

()

--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jsmith, Assigned: cruceru.adrian)

Tracking

({crash})

22 Branch
mozilla28
x86
Windows 7
crash
Points:
---

Firefox Tracking Flags

(firefox22 wontfix, firefox23 wontfix, firefox25 wontfix, firefox26 wontfix, firefox27 affected, firefox28 verified)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
This bug was filed from the Socorro interface and is 
report bp-7fbc5a99-ad89-49c0-b8fb-858db2130528 .
============================================================= 

Frame 	Module 	Signature 	Source
0 	xul.dll 	webrtc::videocapturemodule::DeviceInfoDS::GetDeviceInfo 	media/webrtc/trunk/webrtc/modules/video_capture/windows/device_info_ds.cc:192
1 	xul.dll 	webrtc::videocapturemodule::DeviceInfoDS::GetDeviceName 	media/webrtc/trunk/webrtc/modules/video_capture/windows/device_info_ds.cc:160
2 	xul.dll 	webrtc::ViEInputManager::GetDeviceName 	media/webrtc/trunk/webrtc/video_engine/vie_input_manager.cc:89
3 	xul.dll 	mozilla::MediaEngineWebRTC::EnumerateVideoDevices 	content/media/webrtc/MediaEngineWebRTC.cpp:109
4 	xul.dll 	mozilla::GetUserMediaDevicesRunnable::Run 	dom/media/MediaManager.cpp:831
5 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
6 	xul.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:265
7 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:395
8 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:90
9 	msvcr100.dll 	_callthreadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314
10 	msvcr100.dll 	_threadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292
11 	kernel32.dll 	BaseThreadInitThunk 	
12 	ntdll.dll 	__RtlUserThreadStart 	
13 	ntdll.dll 	_RtlUserThreadStart
Not sure how this can happen; _dsMonikerDevEnum must be null, but I don't see how we could get here if that's the case.  Or maybe it's reporting the wrong line number....  Also, we create _dsMonikerDevEnum right above, and check if it was created, and dereference it before getting here, and don't touch it inside the loop.

Only 1 report, on FF22.

More info needed to make any further progress.
Whiteboard: [WebRTC] [blocking-webrtc-]
Bas - can you think of any way this could crash like this?  I don't know these interfaces well enough to speculate, but it seems odd to me - but we now have a handful of null-deref crashes there.
Flags: needinfo?(bas)
(Assignee)

Comment 4

5 years ago
A possible cause: 
- "_dsMonikerDevEnum" is a class member and used in multiple functions. (and it doesn't need to be)

We can have a race condition where, while iterating through an enum another function may invalidate it.
i.e: While "GetDeviceInfo" is running someone calls "GetDeviceFilter"

Note:
- I'm not sure if this is possible and this is the first bug I've looked into :)
- If this is plausible and I can provide a patch let me know (we'd just move the variable on the stack on each function)
(Assignee)

Comment 5

5 years ago
Created attachment 829751 [details] [diff] [review]
Proposed patch to fix race condition

A possible fix, please review and let me know if I can help test / do anything further.
Flags: needinfo?
Comment on attachment 829751 [details] [diff] [review]
Proposed patch to fix race condition

Bas, can you review this?  If that's a problem, I can find someone else.  Thanks!

Adrian: thanks!!  Not being a windows person, it wasn't entirely clear to me what was going on here when I looked at it before.  Great to have you as a contributor.
Attachment #829751 - Flags: review?(bas)
Assigning to Adrian

bas: ping
Assignee: nobody → cruceru.adrian
Flags: needinfo?
Comment on attachment 829751 [details] [diff] [review]
Proposed patch to fix race condition

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

I'm not too knowledgeable on this code, but the patch looks ok to me.
Attachment #829751 - Flags: review?(bas) → review+
adrian: congratulations, you have a patch ready to land!  The next thing to do is to resolve any nits pointed out (in this case none), and mark the bug in Keywords with "checkin-needed".  This flags it to me (and to the Sheriffs in #developers) that the patch is ready to check in, and someone with the privileges to do so will land it.

One thing that helps is if your .hgrc tells hg what your name and email is, so that gets recorded in the patch, and so the person checking it in doesn't have to manually tell hg who authored the patch.  You can set them manually with "hg qref --user "user <email>"", etc. (Also the bzexport extension to hg is very handy for directly exporting patches from mercurial mq's to bugzilla).  You don't need to update the Author/etc in order to ask for checkin, but it makes it easier on the person handling it.
Flags: needinfo?(cruceru.adrian)

Updated

5 years ago
Flags: needinfo?(bas)
(Assignee)

Comment 10

5 years ago
Hi Randell,

Thanks a lot for the help!, I've added "checkin-needed" and configured my name/email for future patches. 

I can re-upload the patch if anyone asks but I'm guessing the new patch would need to be reviewed again to be submitted? 

Anyway, thanks again,
Adrian
Flags: needinfo?(cruceru.adrian)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd2b651b3a6
status-firefox22: affected → wontfix
status-firefox23: affected → wontfix
status-firefox25: --- → wontfix
status-firefox26: --- → wontfix
status-firefox27: --- → affected
status-firefox28: --- → affected
Keywords: checkin-needed
Target Milestone: --- → mozilla28
https://hg.mozilla.org/mozilla-central/rev/3dd2b651b3a6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox28: affected → fixed
There are only 7 crashes on FF 24 in Socorro in the last month.
Considering verified fixed.
Status: RESOLVED → VERIFIED
status-firefox28: fixed → verified
You need to log in before you can comment on or make changes to this bug.