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-]
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
status-firefox22: --- → affected
status-firefox23: --- → affected
OS: Windows NT → Windows 7
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.
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)
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.
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
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.
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
status-firefox22: affected → wontfix
status-firefox23: affected → wontfix
status-firefox25: --- → wontfix
status-firefox26: --- → wontfix
status-firefox27: --- → affected
status-firefox28: --- → affected
Target Milestone: --- → mozilla28
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → 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.