1313339 - The DXVA blocklist should rely on DLLs loaded in the process, and not look for them in System32 (leads to a lot of crashes previously tracked in bug 1299520)

Status: VERIFIED FIXED

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

Description

[Marco Castelluccio [:marco]]

When making a blocklist decision, we're currently checking for the existence of the DLLs in System32 (https://dxr.mozilla.org/mozilla-central/rev/3f4c3a3cabaf94958834d3a8935adfb4a887942d/dom/media/platforms/wmf/WMFVideoMFTManager.cpp#232).

We should instead rely on the DLLs that are loaded in the Firefox process, because sometimes the DLLs are loaded in the Firefox process but are not in System32.

Another option would be to stop using the DLLs and use the driver version like the gfx blocklist does.

Comment 1

[Marco Castelluccio [:marco]]

This might also fix the issue that we're blocking D3D11 or D3D9 for some users with multiple GPUs, even if they're using the GPU that shouldn't be blocked.

Comment 2

[[:philipp]]

i think we went the path of blocking based on dlls, because there are also third-party modules which aren't graphics drivers hooking into firefox and causing crashiness with dxva (bug 1268905, bug 1273406).

Comment 3

[Marco Castelluccio [:marco]]

(In reply to [:philipp] from comment #2)
> i think we went the path of blocking based on dlls, because there are also
> third-party modules which aren't graphics drivers hooking into firefox and
> causing crashiness with dxva (bug 1268905, bug 1273406).

Interesting, the second option isn't viable then (or it would require having both a gfx driver version blocklist and DLL blocklist).

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Marco Castelluccio [:marco] from comment #0)
> Another option would be to stop using the DLLs and use the driver version
> like the gfx blocklist does.

I intended to eventually fold the D3D blocklist into the gfx one (bug 1282335), which would hopefully solve this issue here.
I'll see which one I can achieve quicker...
(Leaving bug unassigned for now, if someone else wants to take care of it before I can get to it.)

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

By the way, it seems that bug 1299520, which is marked resolved, but is still getting ~500 crashes a day is marked that way because this bug is supposed to track and deal with those crashes.  It isn't quite obvious from the report itself, so I thought I'd mention it.

Comment 6

[[:philipp]]

...maybe we can pump up the bug's priority based on this :-)

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch

Something like this should work, if the modules are loaded before FindDXVABlacklistedDLL is called.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8821146 [details] [diff] [review]
Patch

Review of attachment 8821146 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for working on this, I haven't had time for it. And I was hoping bug 1282335 would make this one here unneeded, but obviously it hasn't happened (yet) either.

At a glance, it seems reasonable to me; good idea to try and find DLLs based on what we've actually loaded.
I don't know about module loading order, and not sure who would know.

One way to test would be to remove the 'ConstructSystem32Path' code path, and see if the expected blocking still happens.

And I'd say that this solution, even if we're not entirely sure it would work 100% of the time, would be better than nothing, until we can get the real fix in!

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp
@@ +199,5 @@
>  
> +  HANDLE hProcess = GetCurrentProcess();
> +  HMODULE hMods[1024];
> +  unsigned int modulesNum = 0;
> +  if (hProcess != NULL) 

Missing '{'.

Comment 9

[Marco Castelluccio [:marco]]

I've tested it on my machine and it seems to work.

On my machine, with an Intel and a NVIDIA GPU, the 'ConstructSystem32Path' path checks both the Intel and the NVIDIA driver, the new code path only checks the driver that is loaded (in my case, the Intel one).
By keeping the 'ConstructSystem32Path' path, we can ensure that the blocklisting is at least as strict as it currently is (and more, when the driver is not in System32).

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 11

[Marco Castelluccio [:marco]]

Comment on attachment 8821500 [details] [diff] [review]
Patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cd6805d1ba46da2c99f65e653d16ed9a3f302ed

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79598bf91283
Also use modules loaded in the process to decide about DXVA blocklisting. r=gerald

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/79598bf91283

Comment 14

[Marco Castelluccio [:marco]]

Comment on attachment 8821500 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: D3D11 DXVA
[User impact if declined]: D3D11 DXVA should be disabled for some drivers but isn't because we fail to detect them. This can lead to users crashing while playing videos (e.g. with signature "NDXGI::CDevice::SignalSynchronizationObjectCB", currently #19 top browser crash and #5 top content crash).
[Is this code covered by automated tests?]: Not sure if the blocklist code is covered by automated tests, but I would think so.
[Has the fix been verified in Nightly?]: Yes, but the crash is too low volume on Nightly to verify that it's completely gone.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Very low risk.
[Why is the change risky/not risky?]: Previously, the blocklist code was only looking for driver DLLs in the System32 directory. Now, it uses both the System32 directory and the list of DLLs loaded in the process. Thus, the blocklist is strictly more restrictive than before.
[String changes made/needed]: None.

Comment 15

[Marco Castelluccio [:marco]]

Attachment: Patch for Beta

The first patch applies cleanly to Aurora. This one is for Beta (the difference is negligible, just a difference in the name of a variable).

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8821500 [details] [diff] [review]
Patch

Super promising improvement for blocklisting and crash prevention!
Let's uplift this to aurora!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c577d43a2444

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8822180 [details] [diff] [review]
Patch for Beta

Crash fix for beta 51.

Comment 19

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

https://hg.mozilla.org/releases/mozilla-beta/rev/922d900d176dea524073efb1dda0c0d089f17ea8

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8821500 [details] [diff] [review]
Patch

Cleaning up the approval flags (the patch for beta already landed)

Comment 21

[Marco Castelluccio [:marco]]

It looks like the problem is fixed, see bug 1299520 comment 89.