2065072 - Crash in [@ mozilla::MediaTrackGraphImpl::OnGraphThread]

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

Crash report: https://crash-stats.mozilla.org/report/index/a4ac8c69-7b93-48e5-8e5d-9a1ae0260819

Crash Reason:

EXCEPTION_ACCESS_VIOLATION_READ at 0x0000000000000000

Top 10 frames:

0  xul.dll  mozilla::MediaTrackGraphImpl::OnGraphThread() const  dom/media/MediaTrackGraph.cpp:1127
1  xul.dll  mozilla::ipc::MessageChannel::WorkerTargetShutdownTask::TargetShutdown()  ipc/glue/MessageChannel.cpp:2533
2  xul.dll  mozilla::(anonymous namespace)::MediaTrackGraphShutDownRunnable::Run()  dom/media/MediaTrackGraph.cpp:1930
3  xul.dll  mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run()  xpcom/threads/TaskDispatcher.h:264
4  xul.dll  mozilla::XPCOMThreadWrapper::Runner::Run()  xpcom/threads/AbstractThread.cpp:213
5  xul.dll  mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail...  xpcom/threads/TaskController.cpp:1360
6  xul.dll  NS_ProcessNextEvent(nsIThread*, bool)  xpcom/threads/nsThreadUtils.cpp:471
7  xul.dll  mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)  ipc/glue/MessagePump.cpp:83
8  xul.dll  MessageLoop::RunHandler()  ipc/chromium/src/base/message_loop.cc:364
9  xul.dll  nsBaseAppShell::Run()  widget/nsBaseAppShell.cpp:151

There are 2 crashes (from 2 installations) in nightly 156 with buildid 20260818092026.

Clouseau analysis (automated, 97% worth investigating — a calibrated estimate that this is worth someone's time, not that the changeset below caused it). The mechanism below fits the evidence but is not proven end-to-end:

1b7fbc53f0e0 (bug 1993981) adds a shutdown-task loop to MediaTrackGraphShutDownRunnable::Run() that runs registered nsITargetShutdownTasks AFTER mGraph->SetCurrentDriver(nullptr) has already nulled mDriver. Because a sibling changeset in the same bug (e7ad1bf72931) makes MediaTrackGraphImpl implement AbstractThread/nsISerialEventTarget (IsCurrentThreadIn() { return OnGraphThread(); }) and support shutdown tasks, a mozilla::ipc::MessageChannel::WorkerTargetShutdownTask opened directly on the graph (the AudioWorklet native-message-passing capability this bug introduces) calls back into OnGraphThread() via mTarget->IsOnCurrentThread(), dereferencing the now-null mDriver at dom/media/MediaTrackGraph.cpp:1127.

Fault address 0x0 with mov rax, qword [rcx] matches a null this-pointer virtual-call read on mDriver; the crashing thread's stack (MediaTrackGraphShutDownRunnable::Run -> WorkerTargetShutdownTask::TargetShutdown -> OnGraphThread) matches the traced call path exactly; and the process thread list's MediaTrackGrph,SHDRCV entry (the ,SHDRCV suffix is appended by nsThread once a thread has been asked to shut down) corroborates that the graph thread was itself mid-shutdown when the main thread hit this race.

Suspected regressor: 1b7fbc53f0e0 (gh) (bug 1993981) by Andreas Pehrson.

Code references:

• dom/media/MediaTrackGraph.cpp:1930
• mozilla::MediaTrackGraphImpl::OnGraphThread
• nsThread

What the automated skeptic pass checked (its own words — a pass means the check succeeded, which is not always support for the conclusion):

• unverifiable hardware_artifact — n=2 report sample is too small to statistically rule hardware error in or out; treated as neutral rather than discounting the (independently well-grounded) software mechanism.
• pass mechanism — Independent agent re-derived and confirmed every edge (OnGraphThread unchanged/old, mDriver nulled at line 1896 pre-existing, new shutdown-task loop from 1b7fbc53f0e0, new AbstractThread wiring from e7ad1bf72931, MessageChannel::Open registering on arbitrary nsISerialEventTarget, virtual-dispatch shim in AbstractThread.cpp) via its own searchfox/source reads.
• pass pre_existing_unrelated_mdriver_null — Skeptic confirmed this exact code path was unreachable before bug 1993981: RegisterShutdownTask returned NS_ERROR_NOT_IMPLEMENTED prior to 1b7fbc53f0e0, which trips MOZ_ASSERT(rv != NS_ERROR_NOT_IMPLEMENTED) in MessageChannel::Open -- so no WorkerTargetShutdownTask could have been registered against MediaTrackGraphImpl before this landing.

:pehrsons, can you have a look please?

Filed automatically by Clouseau, which analyses nightly crashes with an LLM. Nothing above was written or checked by a human. Please close it as INVALID if it is wrong — that is useful feedback, not a nuisance.

Comment 1

[BugBot [:suhaib / :marco]]

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

Comment 2

[Calixte Denizet (:calixte)]

Edited:
I removed the comment here which was just a duplicate report. Sorry for the noise.

Comment 3

[Andreas Pehrson [:pehrsons]]

No need for it to post again.

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 2065072 - Make the graph's IsCurrentThreadIn() allowed during shutdown. r?karlt

Since bug 1993981 the graph is the nsISerialEventTarget that an ipc::MessageChannel
opened on the graph thread binds to, so the graph owns that channel's target
shutdown task. Those tasks were run from MediaTrackGraphShutDownRunnable, on the
main thread, after SetCurrentDriver(nullptr) had already run.
WorkerTargetShutdownTask::TargetShutdown() release-asserts on IsOnCurrentThread(),
which went through OnGraphThread() and dereferenced the now-null driver.

Run the shutdown tasks at the end of the graph's final iteration instead, on the
graph thread, as nsThread::ThreadFunc does once its event loop has exited.

Also delete the worklet's JSContext after the graph's SerialEventTargetGuard has
been dropped. That teardown closes the worklet's ipc::BackgroundChild and can
open a new one, e.g. through ~UniqueMessagePortId, and such a connection must be
owned by the underlying thread, which still processes events, rather than by the
graph, which will not run again.

IsCurrentThreadIn() now follows the graph's OnGraphThreadOrNotRunning()
convention, so that the remaining main thread path, for a graph that never ran an
iteration, does not touch the driver. RegisterShutdownTask() and
UnregisterShutdownTask() report NS_ERROR_UNEXPECTED once the tasks have run, as
ThreadEventQueue does.

Comment 5

[BugBot [:suhaib / :marco]]

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

Comment 6

[Donal Meehan [:dmeehan]]

The regressor Bug 1993981 was reverted from Beta for Fx156

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 2065072 - Let the graph identify its thread while shutting down. r?karlt

Since bug 1993981 the graph is the nsISerialEventTarget that an
ipc::MessageChannel opened on the graph thread binds to, so the graph owns that
channel's target shutdown task. MediaTrackGraphShutDownRunnable runs those
tasks on the main thread, after SetCurrentDriver(nullptr).
WorkerTargetShutdownTask::TargetShutdown() release-asserts on
IsOnCurrentThread(), which went through OnGraphThread() and dereferenced the
now-null driver.

Answer IsCurrentThreadIn() with OnGraphThreadOrNotRunning() instead, the
convention used throughout the class: the graph thread while the driver runs,
and the main thread once it does not. That matches where the graph's state is
owned, so a channel bound to the graph can be torn down from the main thread,
and it never touches mDriver in the not-running case.

Also tear down the worklet's JSContext after the graph's SerialEventTargetGuard
has been dropped, rather than from UpdateMainThreadState(). That teardown
closes the worklet's ipc::BackgroundChild, and deleting the JSContext can start
a new one -- ~UniqueMessagePortId calls MessagePort::ForceClose(), which calls
BackgroundChild::GetOrCreateForCurrentThread() (see bug 1955768 comment 9).
Inside the guard that connection binds to the graph and is left in the graph
thread's thread-local storage, where ChildImpl::ThreadLocalDestructor() closes
it as the thread exits. By then IsCurrentThreadIn() no longer accepts the graph
thread, so MessageChannel::Close() would fail AssertWorkerThread(). Outside the
guard it binds to the underlying thread, which is still its own worker thread
at that point.

Comment 8

[BugBot [:suhaib / :marco]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 AArch64 and ARM crashes on nightly

:pehrsons, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 2065072 - Force the AudioWorklet global scope port closed on main thread. r?karlt

Only ConstructGlobalScope() consumes mGlobalScopePortIdentifier, and it runs
only for addModule(). Touching audioWorklet without calling addModule()
therefore leaves the identifier live for ~AudioWorkletImpl, which runs on the
graph thread because NotifyWorkletFinished() releases the global scope there.

~UniqueMessagePortId then force-closes the port, opening a BackgroundChild bound
to the graph. That connection is closed as the GraphRunner thread exits, by
which time ipc::MessageChannel::AssertWorkerThread() no longer accepts the graph
thread.

Destroy the identifier on the main thread instead. The new hook runs after the
global scope has gone, so ConstructGlobalScope() can no longer race for it.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/4bb758aba4ce
https://hg.mozilla.org/integration/autoland/rev/7bbb87b80643
Make the graph's IsCurrentThreadIn() allowed during shutdown. r=karlt
https://github.com/mozilla-firefox/firefox/commit/15a89220f808
https://hg.mozilla.org/integration/autoland/rev/a96abb7ba9d3
Force the AudioWorklet global scope port closed on main thread. r=karlt,media-playback-reviewers
https://github.com/mozilla-firefox/firefox/commit/ef816cd2e70d
https://hg.mozilla.org/integration/autoland/rev/6f2b1812ed2f
apply code formatting via Lando

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/7bbb87b80643
https://hg.mozilla.org/mozilla-central/rev/a96abb7ba9d3
https://hg.mozilla.org/mozilla-central/rev/6f2b1812ed2f