1552755 - Crash in [@ AsyncShutdownTimeout | profile-before-change | CamerasParent 1,CamerasParent 2]

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Gabriele Svelto [:gsvelto]]

This bug is for crash report bp-0e96c027-4b06-4c13-8f51-507450190519.

Top 10 frames of crashing thread:

0 mozglue.dll mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:33
1 xul.dll static void Abort xpcom/base/nsDebugImpl.cpp:439
2 xul.dll NS_DebugBreak xpcom/base/nsDebugImpl.cpp
3 xul.dll nsresult nsDebugImpl::Abort xpcom/base/nsDebugImpl.cpp:133
4 xul.dll XPTC__InvokebyIndex 
5  @0x257149b837f 
6 xul.dll round 
7 xul.dll round 
8 xul.dll static bool XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1157
9 xul.dll static bool XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:943

Comment 1

[Andreas Pehrson [:pehrsons]]

Dan, can you take a look at this? Seems like a classic mutex deadlock in the DirectShow backend. Thread 50 is locking mutexes on its way down into webrtc::videocapturemodule::CaptureSinkFilter ([1], then [2]) and thread 56 is locking them in reverse order on its way up from webrtc::videocapturemodule::CaptureSinkFilter ([3], then [4]).

[1] https://hg.mozilla.org/mozilla-central/annotate/9b2f851979cb8d0dd0cd2618656eddee32e4f143/media/webrtc/trunk/webrtc/modules/video_capture/windows/video_capture_ds.cc#l154
[2] https://hg.mozilla.org/mozilla-central/annotate/9b2f851979cb8d0dd0cd2618656eddee32e4f143/media/webrtc/trunk/webrtc/modules/video_capture/windows/sink_filter_ds.cc#l393
[3] https://hg.mozilla.org/mozilla-central/annotate/9b2f851979cb8d0dd0cd2618656eddee32e4f143/media/webrtc/trunk/webrtc/modules/video_capture/windows/sink_filter_ds.cc#l319
[4] https://hg.mozilla.org/mozilla-central/annotate/9b2f851979cb8d0dd0cd2618656eddee32e4f143/media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc#l140

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1552755 - Update video_capture/windows code to tip of webrtc.org; r=ng!

This updates just video_capture/windows to the tip of webrtc.org, from commit
8581877121f58435d683c2a39a73e72e5dcd558a.

This removes the locking which was leading to the deadlock in Bug 1552755. It
also removes the reliance upon the DirectShow example code which lead to our
local implementations of BaseFilter, BaseInputPin, BasePin, and MediaType.
These can now be removed.

Comment 3

[Dan Minor [:dminor]]

Attachment: Bug 1552755 - Add ToHex to stringutils.h; r=ng!

This was added in commit 74395345e8d2fecd504cb687eb79deee4ef394c3, but this
only cherry picks the ToHex part of that commit.

Depends on D33336

Comment 4

[Dan Minor [:dminor]]

Attachment: Bug 1552755 - Small fixes to backport the updated video capture code; r=ng!

A few small changes are required to backport the updated windows capture code
to our version of webrtc.org. These are largely path changes, but
DetachFromThread has been renamed to Detach, and they seem to have a working
overload for ostream operator<< for VideoType that I have not been able to
track down. It is also necessary to add our local modification for pid_t back
to the GetDeviceName method, but it is not used for video capture anyways.

Depends on D33337

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1552755 - Update gn generated json files; r=ng!

Depends on D33338

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1552755 - Update moz.build files; r=ng!

Depends on D33339

Comment 7

[Dan Minor [:dminor]]

Try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7ecd883183209900763ef7e8341e72c4ef0cc8b
I've done the usual local testing.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1967aabb055
Update video_capture/windows code to tip of webrtc.org; r=ng
https://hg.mozilla.org/integration/autoland/rev/0f0fdb21b674
Add ToHex to stringutils.h; r=ng
https://hg.mozilla.org/integration/autoland/rev/c2c056771ee9
Small fixes to backport the updated video capture code; r=ng
https://hg.mozilla.org/integration/autoland/rev/d28872da6016
Update gn generated json files; r=ng
https://hg.mozilla.org/integration/autoland/rev/e9685aeaa59a
Update moz.build files; r=ng

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a1967aabb055
https://hg.mozilla.org/mozilla-central/rev/0f0fdb21b674
https://hg.mozilla.org/mozilla-central/rev/c2c056771ee9
https://hg.mozilla.org/mozilla-central/rev/d28872da6016
https://hg.mozilla.org/mozilla-central/rev/e9685aeaa59a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 9069073 [details]
Bug 1552755 - Update moz.build files; r=ng!

Beta/Release Uplift Approval Request

• User impact if declined: This should prevent a shutdown hang due to a deadlock that results when shutting down video capture at the same time a new frame is being delivered. This removes one of the locks that was causing problems.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a low risk import code from upstream webrtc.org. It initially landed there in early April so there is a good chance that any problems would have subsequently been detected and fixed upstream prior to us importing it.
• String changes made/needed: None

Comment 12

[Dan Minor [:dminor]]

I see there are still some shutdown hangs with this signature. They do not seem to be occurring in the same place as before, so I think they are a separate problem.

Comment 13

[Julien Cristau [:jcristau] (back April 22)]

I don't suppose there's a more targeted fix we could do for beta?

Comment 14

[Dan Minor [:dminor]]

(In reply to Julien Cristau [:jcristau] from comment #13)

I don't suppose there's a more targeted fix we could do for beta?

Because it is an import of upstream code it is all or nothing I'm afraid. Trying to break out just the part of their changes that removed the locking would be more risky than taking the whole thing in my opinion.

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

OK I guess I'd rather live with this bug then, it doesn't seem worse than bug 1407415 that we had previously... :/