1750859 - Crash in [@ gfxWindowsPlatform::EnsureDevicesInitialized]

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

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

15 crashes from 5 different devices over the last 1.5 months

Crash report: https://crash-stats.mozilla.org/report/index/8f89dcfa-fe6a-4c36-88e5-a9b650220118

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(!IsWin32kLockedDown())

Top 10 frames of crashing thread:

0 xul.dll gfxWindowsPlatform::EnsureDevicesInitialized gfx/thebes/gfxWindowsPlatform.cpp:1273
1 xul.dll mozilla::layers::WebRenderBridgeChild::GetForMedia gfx/layers/wr/WebRenderBridgeChild.cpp:554
2 xul.dll mozilla::MediaDecoder::NotifyOwnerActivityChanged dom/media/MediaDecoder.cpp:178
3 xul.dll mozilla::dom::HTMLMediaElement::NotifyOwnerDocumentActivityChanged dom/html/HTMLMediaElement.cpp:6428
4 xul.dll mozilla::dom::NotifyActivityChangedCallback dom/base/Document.cpp:7498
5 xul.dll mozilla::dom::Document::EnumerateActivityObservers dom/base/Document.cpp:12898
6 xul.dll mozilla::dom::Document::UpdateVisibilityState dom/base/Document.cpp:15125
7 xul.dll mozilla::detail::RunnableMethodImpl<mozilla::MediaFormatReader*, void  xpcom/threads/nsThreadUtils.h:1200
8 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:144
9 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:771

Comment 1

[Chris Martin [:cmartin]]

Attachment: Bug 1750859 - Assert that all decoders are remote to RDD during Win32k Lockdown

Win32k Lockdown will not work without all decoders remoted, so user
configurations which ask for that combination are unsupported. This asserts
that the user isn't asking us for this unsupported behavior.

Comment 2

[Chris Martin [:cmartin]]

From my analysis, it seems that the only way that these call stacks could exist is if PDMFactory::AllDecodersAreRemote() returns false, which could only happen if one of the media.rdd-<codec>.enabled prefs was set to the non-default value of false.

There doesn't seem to exist any code within Firefox that changes these prefs to false, so it seems that the user set it themselves; however; that is not reflected in the telemetry in these crash reports. It's possible I'm overlooking something, but it seems like the most likely scenario here is that the crash reports are missing information.

My changelist above will still cause a crash in this case, but at least now the crash will explicitly say "You're doing something unsupported", which hopefully provides a hint to the users or people debugging that this is an unsupported use-case, and so we're not going to fix it because we can't - They need to choose between Win32k Lockdown or decoding-in-content.

Comment 3

[Tom Ritter [:tjr]]

Not all prefs get reported in Telemetry by default; here is the list - I'm not sure what the criteria for a crash report it, or if it's the same list.

For all the other invalid pref settings we disabled win32k silently rather than crashing; shouldn't we do that here too?

Comment 4

[Tom Ritter [:tjr]]

Attachment: Bug 1750859: If not all decoders are remoted, you're disqualified from win32k r?bobowen

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7006c345ee00
If not all decoders are remoted, you're disqualified from win32k r=bobowen

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7006c345ee00

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 9264430 [details]
Bug 1750859: If not all decoders are remoted, you're disqualified from win32k r?bobowen

Beta/Release Uplift Approval Request

• User impact if declined: We will ensure the beta branch has all the correct cases for handling win32k lockdown and enable us to run experiments there
• 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):
• String changes made/needed:

Comment 8

[Pascal Chevrel:pascalc]

Comment on attachment 9264430 [details]
Bug 1750859: If not all decoders are remoted, you're disqualified from win32k r?bobowen

Approved 98 beta 7, thanks.

Comment 9

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/1d7807fc9d81