876876 - crash in webrtc::videocapturemodule::DeviceInfoDS::GetDeviceInfo

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Jason Smith [:jsmith]]

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

Comment 1

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

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.

Comment 2

[Scoobidiver (away)]

More reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=webrtc%3A%3Avideocapturemodule%3A%3ADeviceInfoDS%3A%3AGetDeviceInfo%28unsigned+int%2C+char*%2C+unsigned+int%2C+char*%2C+unsigned+int%2C+char*%2C+unsigned+int%29

Comment 3

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

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.

Comment 4

[Adrian Cruceru]

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)

Comment 5

[Adrian Cruceru]

Attachment: Proposed patch to fix race condition

A possible fix, please review and let me know if I can help test / do anything further.

Comment 6

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

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.

Comment 7

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

Assigning to Adrian

bas: ping

Comment 8

[Bas Schouten (:bas.schouten)]

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.

Comment 9

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

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.

Comment 10

[Adrian Cruceru]

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

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd2b651b3a6

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3dd2b651b3a6

Comment 13

[Paul Silaghi, QA [:pauly]]

There are only 7 crashes on FF 24 in Socorro in the last month.
Considering verified fixed.