2007754 - Crash in [@ wgpu_hal::dx12::Adapter::expose]

Status: RESOLVED FIXED

(Core :: Graphics: WebGPU, defect, P1)

Description

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

58 crashes from 15 different installs of Firefox 148.0a1 on Windows 10 and 11. At least some run old hardware, the recent D3D feature levels might not be supported.

Crash report: https://crash-stats.mozilla.org/report/index/45a2cb7f-5a9f-4608-a842-93ba80251225

MOZ_CRASH Reason:

internal error: entered unreachable code

Top 10 frames:

0  xul.dll  MOZ_Crash(char const*, int, char const*)  mfbt/Assertions.h:375
0  xul.dll  RustMozCrash(char const*, int, char const*)  mozglue/static/rust/wrappers.cpp:18
1  xul.dll  mozglue_static::panic_hook(std::panic::PanicHookInfo*)  mozglue/static/rust/lib.rs:99
2  xul.dll  core::ops::function::Fn::call<void (*)(ref$<std::panic::PanicHookInfo>), tupl...  /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/ops/function.rs:253
3  xul.dll  std::panic::PanicHookInfo::new()  library/alloc/src/boxed.rs:1985
3  xul.dll  std::panicking::rust_panic_with_hook()  library/std/src/panicking.rs:841
4  xul.dll  std::panicking::begin_panic_handler::closure$0()  library/std/src/panicking.rs:699
5  xul.dll  std::sys::backtrace::__rust_end_short_backtrace<std::panicking::begin_panic_h...  library/std/src/sys/backtrace.rs:174
6  xul.dll  std::panicking::begin_panic_handler()  library/std/src/panicking.rs:697
7  xul.dll  core::panicking::panic_fmt()  library/core/src/panicking.rs:75

Comment 1

[Bob Hood [:bhood]]

Since it's a panic, it's not exploitable, so setting an initial Severity of S3. Please adjust if needed.

Comment 2

[BugBot [:suhaib / :marco]]

The bug is marked as tracked for firefox148 (nightly). We have limited time to fix this, the soft freeze is in 13 days. However, the bug still isn't assigned and has low severity.

:bhood, could you please find an assignee and increase the severity for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 3

[Gabriele Svelto [:gsvelto]]

Adding a Windows signature for this crash.

Comment 4

[BugBot [:suhaib / :marco]]

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

• Top 10 desktop browser crashes on nightly

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

For more information, please visit BugBot documentation.

Comment 5

[BugBot [:suhaib / :marco]]

This is a reminder regarding comment #2!

The bug is marked as tracked for firefox148 (nightly). We have limited time to fix this, the soft freeze is in 7 days. However, the bug still isn't assigned.

Comment 6

[Erich Gubler [:ErichDonGubler] (he/him)]

Looking through the associated crashes, the vast majority are hitting the unreachable!() branch in third_party/rust/wgpu-hal/src/dx12/adapter.rs:121-128 (which Sebastian alluded to); for convenience, that and some preceding code are what query the D3D12 feature level in an early stage of capability negotiation:

        // Detect the highest supported feature level.
        let d3d_feature_level = [
            Direct3D::D3D_FEATURE_LEVEL_12_2,
            Direct3D::D3D_FEATURE_LEVEL_12_1,
            Direct3D::D3D_FEATURE_LEVEL_12_0,
            Direct3D::D3D_FEATURE_LEVEL_11_1,
            Direct3D::D3D_FEATURE_LEVEL_11_0,
        ];
        let mut device_levels = Direct3D12::D3D12_FEATURE_DATA_FEATURE_LEVELS {
            NumFeatureLevels: d3d_feature_level.len() as u32,
            pFeatureLevelsRequested: d3d_feature_level.as_ptr().cast(),
            MaxSupportedFeatureLevel: Default::default(),
        };
        unsafe {
            device.CheckFeatureSupport(
                Direct3D12::D3D12_FEATURE_FEATURE_LEVELS,
                <*mut _>::cast(&mut device_levels),
                size_of_val(&device_levels) as u32,
            )
        }
        .unwrap();
        let max_feature_level = match device_levels.MaxSupportedFeatureLevel {
            Direct3D::D3D_FEATURE_LEVEL_11_0 => FeatureLevel::_11_0,
            Direct3D::D3D_FEATURE_LEVEL_11_1 => FeatureLevel::_11_1,
            Direct3D::D3D_FEATURE_LEVEL_12_0 => FeatureLevel::_12_0,
            Direct3D::D3D_FEATURE_LEVEL_12_1 => FeatureLevel::_12_1,
            Direct3D::D3D_FEATURE_LEVEL_12_2 => FeatureLevel::_12_2,
            _ => unreachable!(),
        };

This must mean that the device_levels.MaxSupportedFeatureLevel is returning some other value, but that seems contrary to the documentation of the return value for ID3D12Device::CheckFeatureSupport:

Returns S_OK if successful. Returns E_INVALIDARG if an unsupported data type is passed to the pFeatureSupportData parameter or if a size mismatch is detected for the FeatureSupportDataSize parameter.

I would expect that the provided d3d_feature_level [sic] array would constrain the returned max feature level to be one of its members, or, if the environment doesn't support any of them, to cause the function to return E_INVALIDARG.

My current set of hypotheses for what's going wrong are as follows:

1. We should be expecting the function to succeed, and for MaxFeatureLevel to remain set to its initial value (i.e., Default::default()).
2. The current implementation should be returning E_INVALIDARG, and happens to do something else.

Regardless of which hypothesis is correct, we unfortunately still are saddled with working around this, and the fix is probably the same between them.

My suggested next steps are:

1. Try to determine what value(s) MaxFeatureLevel is actually set to (with a small timebox, maybe a couple of weeks at most). If the set is fixed, and we can reliably detect it, we should add a branch returning an error in the same way the rest of capability negotiation may fail if required functionality isn't supported. Otherwise, we may need to consider simply changing the unreachable!() branch to return an error in this way.

2. Reach out to Microsoft to determine what the expected behavior is supposed to be.

   Once clarified, I'd like to see MSDN documentation updated to reflect this somehow, though I'm not sure what changes to make to achieve better clarity yet.

Comment 7

[Erich Gubler [:ErichDonGubler] (he/him)]

I just tested on a Windows machine of mine that doesn't support Direct3D::D3D_FEATURE_LEVEL_12_2 by uncommenting all but the Direct3D::D3D_FEATURE_LEVEL_12_2 member of the d3d_feature_level array I mentioned in comment 6. In that case, CheckFeatureSupport does return an E_INVALIDARG. 🫠 This supports hypothesis (2) from comment 6. I wonder if this is a bug (i.e., hypothesis (1)) that got fixed in newer versions of DX12 than what user machines currently have?

Comment 8

[Erich Gubler [:ErichDonGubler] (he/him)]

I'm going to assign this to :teoxoy. The regressor was upstream work done by him to provide telemetry capabilities in wgpu for bug 1980057 (which, to be clear, is not yet implemented, so I think the current regressor makes the most sense). I've also filed wgpu#8803 to track this upstream.

I'm guessing that the new crashing environments are also the ones we were trying to get more details about with that bug.

Comment 9

[Jim Blandy :jimb]

One thing I noticed is that, if for some reason CheckFeatureLevel decides not to store anything in device_levels.MaxSupportedFeatureLevel, then we'll hit that unreachable!.

I think this is because Default::default(), which is D3D_FEATURE_LEVEL::default, refers to the derived implementation here:

#[repr(transparent)]
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
pub struct D3D_FEATURE_LEVEL(pub i32);

So D3D_FEATURE_LEVEL::default returns D3D_FEATURE_LEVEL(0), which is not covered by the match statement.

Comment 10

[Erich Gubler [:ErichDonGubler] (he/him)]

:jimb: See this comment for wgpu#8806:

We don't have 100% certainty that the case we're trying to handle is the actual behavior causing crashes here (viz., we're not sure that 0 is actually the value remaining in MaxSupportFeatureLevel). I believe this is the case, but it would be best if we could find a way to reproduce this before merging.

If you feel that this is worth merging and seeing if this resolves this empirically, then SGTM.

Comment 11

[Jim Blandy :jimb]

Seems worth a try.

Comment 12

[Teodor Tanasoaia [:teoxoy]]

I put up a comment describing what I saw in the crash stats: https://github.com/gfx-rs/wgpu/pull/8806#issuecomment-3723457930.
I'll put up a PR solving the issues and adding telemetry in those 2 cases as well.

Comment 13

[Teodor Tanasoaia [:teoxoy]]

https://github.com/gfx-rs/wgpu/pull/8845 landed, we now need to wait for a revendor.

Comment 14

[Teodor Tanasoaia [:teoxoy]]

Attachment: Bug 2007754 - Update `wgpu` to revision a3164af1e1922052966e00147be12ea7e2a5bb22. r=#webgpu-reviewers

Comment 15

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: GPU process crashes in select configurations. Bug is labeled as topcrash.
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: The patch avoids us crashing in 2 instances with minimal code changes.
• String changes made/needed: No
• Is Android affected?: no

Comment 16

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: GPU process crashes in select configurations. Bug is labeled as topcrash.
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: The patch avoids us crashing in 2 instances with minimal code changes. There is also a link to the CTS try run that should come back green.
• String changes made/needed: No
• Is Android affected?: no

Comment 17

[BugBot [:suhaib / :marco]]

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

Comment 18

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/11ca06b45dc5
https://hg.mozilla.org/releases/mozilla-beta/rev/98e9ce007623

Comment 19

[Donal Meehan [:dmeehan]]

Resolving this since the patch for firefox-main landed under Bug 2009723