1825514 - Crash in [@ nsCOMPtr<T>::operator= | mozilla::ThreadEventTarget::Dispatch]

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

Consider this a followup to bug 1817724 to evaluate the results. There were no fixes to the crashes in bug 1817724, not even hypothetical. But the frame callback will behave differently so I'm hoping it will result in somewhat different symptoms.

+++ This bug was initially created as a clone of Bug #1817724 +++

+++ This bug was initially created as a clone of Bug #1811641 +++

Crash report: https://crash-stats.mozilla.org/report/index/8ad89e28-c397-49ab-bff6-616b70230119

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0  XUL  nsCOMPtr<nsIThreadObserver>::operator=  xpcom/base/nsCOMPtr.h:663
0  XUL  mozilla::ThreadEventQueue::PutEventInternal  xpcom/threads/ThreadEventQueue.cpp:140
0  XUL  mozilla::ThreadEventQueue::PutEvent  xpcom/threads/ThreadEventQueue.cpp:73
0  XUL  mozilla::ThreadEventTarget::Dispatch  xpcom/threads/ThreadEventTarget.cpp:122
0  XUL  nsThread::Dispatch  xpcom/threads/nsThread.cpp:676
1  XUL  nsIEventTarget::Dispatch  dist/include/nsIEventTarget.h:42
1  XUL  mozilla::camera::CamerasParent::RecvStartCapture const  dom/media/systemservices/CamerasParent.cpp:970
1  XUL  mozilla::media::LambdaRunnable<mozilla::camera::CamerasParent::RecvStartCapture  dom/media/systemservices/MediaUtils.h:77
2  XUL  nsThread::ProcessNextEvent  xpcom/ds/nsTObserverArray.h:89
3  XUL  NS_ProcessNextEvent  xpcom/threads/nsThreadUtils.cpp:473

This signature is pretty bad, but I opened a half dozen of these crashes and they all had CamerasParent::RecvStartCapture on the stack and are all on Mac OS, so I'm going to guess that this is fallout from bug 1806605.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1806605

Comment 2

[Andreas Pehrson [:pehrsons]]

I found an issue based on this crash report and its stack:

0 libsystem_pthread.dylib _pthread_cond_updateval
1 libsystem_pthread.dylib pthread_cond_signal
2 libmozglue.dylib mozilla::detail::ConditionVariableImpl::notify_one() mozglue/misc/ConditionVariable_posix.cpp:93
3 XUL __59-[RTCCameraVideoCapturer stopCaptureWithCompletionHandler:]_block_invoke third_party/libwebrtc/sdk/objc/components/capturer/RTCCameraVideoCapturer.m:220

This indicates something has gone south with the monitor in the completionHandler block we pass to stopCaptureWithCompletionHandler, i.e. here.

The obvious issue is that we set done = true without holding the lock which lets the calling thread stop waiting for the Notify() and run to completion, basically destroying the monitor before or while the completion handler is in Monitor::Notify(). Who knows where it could drive off to then.

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1825514 - In VideoCaptureAvFoundation, lock the monitor before notifying it. r?webrtc-reviewers

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/606555f7b50c
In VideoCaptureAvFoundation, lock the monitor before notifying it. r=webrtc-reviewers,jesup,dbaker

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9328850 [details]
Bug 1825514 - In VideoCaptureAvFoundation, lock the monitor before notifying it. r?webrtc-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: This is a hypthetical fix (to a real potential issue), but if this is indeed the proper fix for the crashes tracked in this bug, uplifting now will let us save one release cycle before knowing (and before shipping), since this backend is only enabled in nightly and early beta (and only beta really gives us enough crash reports to know for sure).
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): Trivially fixing a classic bug of not locking a monitor before Notify().
• String changes made/needed:
• Is Android affected?: No

Comment 6

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/606555f7b50c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9328850 [details]
Bug 1825514 - In VideoCaptureAvFoundation, lock the monitor before notifying it. r?webrtc-reviewers

Approved for 113.0b5. We should be able to see what happens to the crash rate for b5 & b6 before the early beta period ends this cycle.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/56fb19ef32b6