1680410 - Hit MOZ_CRASH(WorkletGlobalScope not thread-safe) at /builds/worker/checkouts/gecko/xpcom/base/nsISupportsImpl.cpp:41

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Tyson Smith [:tsmith]]

This was first found while fuzzing m-c 20201112-9a0fb6731557. Still working on a test case.

Hit MOZ_CRASH(WorkletGlobalScope not thread-safe) at src/xpcom/base/nsISupportsImpl.cpp:41

#0 0x7f008703b69a in MOZ_Crash /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:254:3
#1 0x7f008703b69a in nsAutoOwningThread::AssertCurrentThreadOwnsMe(char const*) const src/xpcom/base/nsISupportsImpl.cpp:41:5
#2 0x7f008b45ce28 in AssertOwnership<35> /builds/worker/workspace/obj-build/dist/include/nsISupportsImpl.h:60:5
#3 0x7f008b45ce28 in mozilla::dom::WorkletGlobalScope::Release() src/dom/worklet/WorkletGlobalScope.cpp:32:1
#4 0x7f008ab8d6c0 in Release src/dom/media/webaudio/AudioWorkletGlobalScope.cpp:33:1
#5 0x7f008ab8d6c0 in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:50:40
#6 0x7f008ab8d6c0 in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:381:36
#7 0x7f008ab8d6c0 in ~RefPtr /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:81:7
#8 0x7f008ab8d6c0 in mozilla::dom::WorkletNodeEngine::~WorkletNodeEngine() src/dom/media/webaudio/AudioWorkletNode.cpp:53:7
#9 0x7f008ab8da5d in mozilla::dom::WorkletNodeEngine::~WorkletNodeEngine() src/dom/media/webaudio/AudioWorkletNode.cpp:53:7
#10 0x7f008ab655d3 in operator() /builds/worker/workspace/obj-build/dist/include/mozilla/UniquePtr.h:460:5
#11 0x7f008ab655d3 in reset /builds/worker/workspace/obj-build/dist/include/mozilla/UniquePtr.h:302:7
#12 0x7f008ab655d3 in ~UniquePtr /builds/worker/workspace/obj-build/dist/include/mozilla/UniquePtr.h:253:18
#13 0x7f008ab655d3 in mozilla::AudioNodeTrack::~AudioNodeTrack() src/dom/media/webaudio/AudioNodeTrack.cpp:55:1
#14 0x7f008ab66aad in mozilla::AudioNodeTrack::~AudioNodeTrack() src/dom/media/webaudio/AudioNodeTrack.cpp:52:35
#15 0x7f008ab9029d in Release src/dom/media/MediaTrackGraph.h:265:3
#16 0x7f008ab9029d in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:50:40
#17 0x7f008ab9029d in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:381:36
#18 0x7f008ab9029d in ~RefPtr /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:81:7
#19 0x7f008ab9029d in ~PlayingRefChangeHandler src/dom/media/webaudio/PlayingRefChangeHandler.h:16:7
#20 0x7f008ab9029d in mozilla::dom::PlayingRefChangeHandler::~PlayingRefChangeHandler() src/dom/media/webaudio/PlayingRefChangeHandler.h:16:7
#21 0x7f00871088a7 in mozilla::Runnable::Release() src/xpcom/threads/nsThreadUtils.cpp:68:1
#22 0x7f0087009517 in ~nsCOMPtr src/xpcom/base/nsCOMPtr.h:452:7
#23 0x7f0087009517 in Destruct src/xpcom/ds/nsTArray.h:645:45
#24 0x7f0087009517 in DestructRange src/xpcom/ds/nsTArray.h:2379:7
#25 0x7f0087009517 in ClearAndRetainStorage src/xpcom/ds/nsTArray.h:1457:5
#26 0x7f0087009517 in nsTArray_Impl<nsCOMPtr<nsIRunnable>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() src/xpcom/ds/nsTArray.h:1012:7
#27 0x7f008a84ef27 in mozilla::MediaTrackGraphImpl::RunInStableState(bool) src/dom/media/MediaTrackGraph.cpp:1808:1
#28 0x7f008a86d6b2 in mozilla::(anonymous namespace)::MediaTrackGraphStableStateRunnable::Run() src/dom/media/MediaTrackGraph.cpp:1649:15
#29 0x7f00870ffcbc in mozilla::XPCOMThreadWrapper::Runner::Run() src/xpcom/threads/AbstractThread.cpp:194:25
#30 0x7f00870ff79f in mozilla::RunnableTask::Run() src/xpcom/threads/TaskController.cpp:450:16
#31 0x7f00870fde0a in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) src/xpcom/threads/TaskController.cpp:720:26
#32 0x7f00870fceb4 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) src/xpcom/threads/TaskController.cpp:579:15
#33 0x7f00870fd067 in mozilla::TaskController::ProcessPendingMTTask(bool) src/xpcom/threads/TaskController.cpp:373:36
#34 0x7f0087102ff6 in operator() src/xpcom/threads/TaskController.cpp:120:37
#35 0x7f0087102ff6 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_3>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:577:5
#36 0x7f0087114577 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1197:14
#37 0x7f008711a2ba in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:513:10
#38 0x7f0087a07af6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:87:21
#39 0x7f0087979bd3 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:334:10
#40 0x7f0087979aed in RunHandler src/ipc/chromium/src/base/message_loop.cc:327:3
#41 0x7f0087979aed in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:309:3
#42 0x7f008b670028 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
#43 0x7f008ce79353 in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:913:20
#44 0x7f0087a088b9 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:237:9
#45 0x7f0087979bd3 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:334:10
#46 0x7f0087979aed in RunHandler src/ipc/chromium/src/base/message_loop.cc:327:3
#47 0x7f0087979aed in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:309:3
#48 0x7f008ce78f38 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:744:34
#49 0x555c0ba1ea27 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#50 0x555c0ba1ea27 in main src/browser/app/nsBrowserApp.cpp:304:18
#51 0x7f00a264fb96 in __libc_start_main /build/glibc-2ORdQG/glibc-2.27/csu/../csu/libc-start.c:310
#52 0x555c0b9fc7d9 in _start (/home/twsmith/workspace/browsers/m-c-20201112094441-fuzzing-debug/firefox-bin+0x147d9)

Comment 1

[Karl Tomlinson (:karlt)]

Thank you for finding this. I wonder whether there are some other assertions that would fail before this point.
Was this an --enable-debug build?

Comment 2

[Tyson Smith [:tsmith]]

(In reply to Karl Tomlinson (:karlt) from comment #1)

Was this an --enable-debug build?

Yes this was found with a debug build. I am working on getting a rr trace to open a Pernosco session atm.

Comment 3

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/Hh0hmV_rSyKeg0y7YoFBFg/index.html

Comment 4

[Karl Tomlinson (:karlt)]

The fault is that WorkletNodeEngine is not handling all cases where the track is released off graph thread.

The pernosco trace was very helpful, thank you, in identifying how we got here.

A. main thread

1. Window of audiocontext is closed.
2. graph thread JS interrupt requested.
3. GC, which triggers MediaTrack::Destroy().

B. graph thread

1. PlayingRefChangeHandler is created with reference to track.
2. Track destroy control message releases graph ref to track (in separate iteration to PlayingRefChangeHandler construction).
3. UpdateMainThreadState() notices mForceShutDownReceived.

C. main thread

1. PlayingRefChangeHandler releases track.

In regular use, usually, either

1. The last track reference is released in B2, or
2. NotifyForcedShutdown() is called on the track in B3.

B2 means that NotifyForcedShutdown() is not called on the track.

I haven't succeeded in constructing a reproducing test case due to difficulties in arranging for B2 to happen before B3, and C1 after B2.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: Bug 1680410 extend forced shutdown track notification to include the usual destroy process r?padenot

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

extend forced shutdown track notification to include the usual destroy process r=padenot
https://hg.mozilla.org/integration/autoland/rev/188d502975aa87caf192363cf0ff1cafcd2a242a
https://hg.mozilla.org/mozilla-central/rev/188d502975aa

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Hi Karl, does this affect ESR78 also? Should we consider backporting if so? Looks like it'd need a bit of rebasing if yes.

Comment 8

[Karl Tomlinson (:karlt)]

Yes, ESR78 is affected.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: 1680410-esr.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-moderate.
User impact if declined: sec-moderate. Potential UaF under unusual race conditions.
Fix Landed on Version: 85
Risk to taking this patch (and alternatives if risky):
This releases some objects and clears their pointers. It should be safe because the objects are used only on the same thread.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9193414 [details] [diff] [review]
1680410-esr.patch

Approved for 78.7esr.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/b57ff697a775