1683822 - Crash in [@ mozilla::detail::MutexImpl::~MutexImpl | mozilla::AudioCallbackDriver::~AudioCallbackDriver]

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Crash report: https://crash-stats.mozilla.org/report/index/f16151a3-a57b-4dce-98a6-b95c10201218

MOZ_CRASH Reason: MOZ_CRASH(mozilla::detail::MutexImpl::~MutexImpl: pthread_mutex_destroy failed)

Top 10 frames of crashing thread:

0 libmozglue.dylib mozilla::detail::MutexImpl::~MutexImpl mozglue/misc/Mutex_posix.cpp:90
1 XUL mozilla::AudioCallbackDriver::~AudioCallbackDriver dom/media/GraphDriver.cpp:545
2 XUL mozilla::AudioCallbackDriver::~AudioCallbackDriver dom/media/GraphDriver.cpp:536
3 XUL mozilla::AudioCallbackDriver::Release dom/media/GraphDriver.h:560
4 XUL mozilla::detail::RunnableFunction<mozilla::AudioCallbackDriver::DeviceChangedCallback xpcom/threads/nsThreadUtils.h:568
5 XUL mozilla::Runnable::Release xpcom/threads/nsThreadUtils.cpp:65
6 XUL nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:301
7 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1197
8 libsystem_malloc.dylib _malloc_zone_calloc 
9 libobjc.A.dylib cache_getImp 

It seems like we're attempting to destroy a mutex that's still being held. You can see the crashing stack in thread 45 above, at the same time the stack in thread 0 looks like this:

0 libsystem_kernel.dylib kevent_id
1 libdispatch.dylib _dispatch_event_loop_wait_for_ownership
2 libdispatch.dylib __DISPATCH_WAIT_FOR_QUEUE__
3 libdispatch.dylib _dispatch_sync_f_slow
4 XUL core::ptr::drop_in_place src/libcore/ptr/mod.rs:177
5 XUL cubeb_backend::capi::capi_stream_destroy third_party/rust/cubeb-backend/src/capi.rs:174
6 XUL mozilla::AudioCallbackDriver::~AudioCallbackDriver() dom/media/GraphDriver.cpp:545
7 XUL mozilla::AudioCallbackDriver::~AudioCallbackDriver() dom/media/GraphDriver.cpp:536
8 XUL mozilla::AudioCallbackDriver::Release() dom/media/GraphDriver.h:560
9 XUL mozilla::(anonymous namespace)::MediaTrackGraphShutDownRunnable::Run() dom/media/MediaTrackGraph.cpp:1586
10 XUL mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() xpcom/threads/TaskDispatcher.h:228

So the main thread was waiting to get a lock on the mutex suggesting that it was already held by some other thread. Maybe thread 45 itself?

Comment 1

[Alastor Wu [:alwu]]

Hi, paul,
It crashed in the graph driver, and all of them happened on MacOS. Do you get any idea what's happened?
Thank you.

Comment 2

[Paul Adenot (:padenot)]

This is related to the fallback driver probably, it's the only lock there is when using an AudioCallbackDriver. This also looks like a shutdown crash ?

Comment 3

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

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 4

[Andreas Pehrson [:pehrsons]]

Looks to me like a double-destroy by two different threads.

This is in release, so we don't hit RefPtr asserts.

Something like:

• Main: ACD::Release()
• Main: ACD::~ACD()
• Main ~ACD() blocked on IO, destroying the cubeb stream (and clearing the DeviceChangeCallback)
• AudioIPC: DeviceChangeCallback() -> ACD::AddRef()
• AudioIPC: Dispatch to Background
• Background: ACD::Release()
• Background: ACD::~ACD()

Do we have a racy situation wrt releasing the cubeb stream? If the above is correct we should be able to mitigate this if we unhook the DeviceChangeCallback before we engage the dtor.

Comment 5

[Paul Adenot (:padenot)]

It seems to me that we shouldn't be doing cubeb_stream_destroy on the main thread either, but your suggestion also would work.

We need to support device change events in the mock cubeb backend to test this.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1683822 - Teach mockcubeb to do device change callbacks.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Bug 1683822 - Unregister the device changed callback before destroying an AudioCallbackDriver. r?pehrsons

Depends on D101177

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 1683822 - Test WIP.

Depends on D101178

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7dd0f81f5ea
Unregister the device changed callback before destroying an AudioCallbackDriver. r=pehrsons

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a7dd0f81f5ea

Comment 11

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

The patch landed in nightly and beta is affected.
:padenot, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 9196089 [details]
Bug 1683822 - Unregister the device changed callback before destroying an AudioCallbackDriver. r?pehrsons

Beta/Release Uplift Approval Request

• User impact if declined: A few crashes a day on macOS
• 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): It's clear that this is the right thing to do, but it's too low traffic and the timing is too hard to reproduce to have a test ready right now, this is worked on in 1700592.
• String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9196089 [details]
Bug 1683822 - Unregister the device changed callback before destroying an AudioCallbackDriver. r?pehrsons

Approved for 88.0b5.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9666ad107c07