1732416 - Crash in [@ mozilla::PDMFactory::CreatePDMs] from accessing pref media.decoder-doctor.wmf-disabled-is-failure

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/a4ffbe9e-5486-481c-99a8-1bf8f0210923

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(IsAtomic<bool>::value || NS_IsMainThread()) (Non-atomic static pref 'media.decoder-doctor.wmf-disabled-is-failure' being accessed on background thread by getter)

Top 10 frames of crashing thread:

0 xul.dll mozilla::PDMFactory::CreatePDMs dom/media/platforms/PDMFactory.cpp:440
1 xul.dll mozilla::AllocationWrapper::CreateDecoder::<unnamed-tag>::operator const dom/media/platforms/AllocationPolicy.cpp:215
2 xul.dll static mozilla::MozPromise<RefPtr<mozilla::AllocPolicy::Token>, bool, 1>::InvokeMethod<`lambda at /builds/worker/checkouts/gecko/dom/media/platforms/AllocationPolicy.cpp:141:7', RefPtr<mozilla::MozPromise<RefPtr<mozilla::AllocPolicy::Token>, bool, 1> >  xpcom/threads/MozPromise.h:630
3 xul.dll mozilla::MozPromise<RefPtr<mozilla::AllocPolicy::Token>, bool, 1>::ThenValue<`lambda at /builds/worker/checkouts/gecko/dom/media/platforms/AllocationPolicy.cpp:205:15', `lambda at /builds/worker/checkouts/gecko/dom/media/platforms/AllocationPolicy.cpp:232:15'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:846
4 xul.dll mozilla::MozPromise<RefPtr<mozilla::AllocPolicy::Token>, bool, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:487
5 xul.dll mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:208
6 xul.dll nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:303
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1142
8 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:300
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:324

This looks like a thread safety check on a preference getter. There are a few crashes, but all from the same user, so maybe their have some weird prefs set.

Comment 1

[Andrew McCreight [:mccr8]]

Maybe media.decoder-doctor.wmf-disabled-is-failure just needs to be made RelaxedAtomicBool like media.wmf.enabled and the other WMF prefs?

Comment 2

[Andrew McCreight [:mccr8]]

media.wmf.use-sync-texture and media.wmf.vp9.enabled also aren't threadsafe, FWIW.

Comment 3

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Think we missed this due to the auto severity setting causing our triage tools to pass over it. Let's get this sorted.

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

(In reply to Andrew McCreight [:mccr8] from comment #2)

media.wmf.use-sync-texture and media.wmf.vp9.enabled also aren't threadsafe, FWIW.

media.wmf.use-sync-texture is only mirrored once, does that still pose a risk being non-atomic? media.wmf.vp9.enabled was fixed in bug 1760804.

Patch incoming to fix DD pref.

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1732416 - Change media.decoder-doctor.wmf-disabled-is-failure to atomic bool.

This pref should be a RelaxedAtomicBool to avoid races.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2db19214f439
Change media.decoder-doctor.wmf-disabled-is-failure to atomic bool. r=alwu

Comment 7

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/2db19214f439

Comment 8

[Dianna Smith [:diannaS]]

do you think this is something you want to uplift to beta? if so I would prefer to get this in before early beta finishes.

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

(In reply to Dianna Smith [:diannaS] from comment #8)

do you think this is something you want to uplift to beta? if so I would prefer to get this in before early beta finishes.

It can't hurt. Simple fix that may save us some pain. Will request uplift.

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 9271145 [details]
Bug 1732416 - Change media.decoder-doctor.wmf-disabled-is-failure to atomic bool.

Beta/Release Uplift Approval Request

• User impact if declined: A (diagnostic assert) crash can be triggered within the media pipeline. This crash prevents graceful error handling and leads to a bad user experience.
• 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): Low risk as this is a single line change to make pref atomic.
• String changes made/needed: None

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9271145 [details]
Bug 1732416 - Change media.decoder-doctor.wmf-disabled-is-failure to atomic bool.

Approved for 100.0b6

Comment 12

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a19401cb90d2