1887703 - Crash in [@ gfxWindowsPlatform::RecordContentDeviceFailure]

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[BugBot [:suhaib / :marco/ :calixte]]

Crash report: https://crash-stats.mozilla.org/report/index/f99b2a7e-72fc-45f4-aa08-fae570240322

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(isSome())

Top 10 frames of crashing thread:

0  xul.dll  gfxWindowsPlatform::RecordContentDeviceFailure  mfbt/Maybe.h:783
0  xul.dll  mozilla::gfx::DeviceManagerDx::CreateContentDevice  gfx/thebes/DeviceManagerDx.cpp:1021
1  xul.dll  mozilla::gfx::DeviceManagerDx::CreateContentDevicesLocked  gfx/thebes/DeviceManagerDx.cpp:643
2  xul.dll  mozilla::gfx::DeviceManagerDx::CreateContentDevices  gfx/thebes/DeviceManagerDx.cpp:629
3  xul.dll  mozilla::RDDParent::RecvInitVideoBridge  dom/media/ipc/RDDParent.cpp:216
4  xul.dll  mozilla::PRDDParent::OnMessageReceived  ipc/ipdl/PRDDParent.cpp:824
5  xul.dll  mozilla::ipc::MessageChannel::DispatchAsyncMessage  ipc/glue/MessageChannel.cpp:1818
5  xul.dll  mozilla::ipc::MessageChannel::DispatchMessage  ipc/glue/MessageChannel.cpp:1737
5  xul.dll  mozilla::ipc::MessageChannel::RunMessage  ipc/glue/MessageChannel.cpp:1530
5  xul.dll  mozilla::ipc::MessageChannel::MessageTask::Run  ipc/glue/MessageChannel.cpp:1628

By querying Nightly crashes reported within the last 2 months, here are some insights about the signature:

• First crash report: 2024-03-22
• Process type: RDD
• Is startup crash: No
• Has user comments: No
• Is null crash: No

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:bhood, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

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

• Top 10 desktop browser crashes on nightly

For more information, please visit BugBot documentation.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

The critical log shows things going wrong:

|[0][GFX1-]: Fallback WR to SW-WR + D3D11 (t=212.35) |[1][GFX1-]: Failed to connect GPU process (t=212.35) |[2][GFX1-]: [D3D11] failed to get compositor device. (t=212.66) |[3][GFX1-]: Failed to initialize CompositorD3D11 for SWGL: FEATURE_FAILURE_D3D11_NO_DEVICE (t=212.66) |[4][GFX1-]: [D3D11] failed to get compositor device. (t=273.13) |[5][GFX1-]: Failed to initialize CompositorD3D11 for SWGL: FEATURE_FAILURE_D3D11_NO_DEVICE (t=273.13) |[6][GFX1-]: [D3D11] failed to get compositor device. (t=377.35) |[7][GFX1-]: Failed to initialize CompositorD3D11 for SWGL: FEATURE_FAILURE_D3D11_NO_DEVICE (t=377.35) |[8][GFX1-]: RenderCompositorSWGL failed mapping default framebuffer, no dt (t=378.6) |[9][GFX1-]: [D3D11] failed to get compositor device. (t=385.31) |[10][GFX1-]: Failed to initialize CompositorD3D11 for SWGL: FEATURE_FAILURE_D3D11_NO_DEVICE (t=385.31) 

Comment 4

[Brad Werth [:bradwerth]]

Theory: the weird call stack (a release assert on mContentDevice = device) may have something to do with the guarantee that a lock is held before accessing mContentDevice. The call stacks don't seem to show a stack where that lock isn't held, but the function definitions don't enforce the lock (by taking a const MutexAutoLock& aProofOfLock param). Might be an improvement to add those protections.

Comment 5

[Brad Werth [:bradwerth]]

Much better theory: mDeviceStatus is a Maybe, and it is unconditionally dereferenced at the beginning of DeviceManagerDx::CreateContentDevice. I'll build a patch to fix this.

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1887703: Make DeviceManagerDx always check mDeviceStatus before dereferencing it.

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1887703: Upgrade an assert in DeviceManagerDx::CreateContentDevicesLocked to MOZ_RELEASE_ASSERT.

If this is the source of the crash, this assert will change the crash
signature, but that will pinpoint the cause. In such a case, applying
D208682 should fix the crash.

Comment 8

[Brad Werth [:bradwerth]]

Marking leave-open while we see if the landing of D208694 affects the crash signature.

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b89431ee0f24

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit BugBot documentation.

Comment 11

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #8)

Marking leave-open while we see if the landing of D208694 affects the crash signature.

It doesn't appear that mDeviceStatus is NULL in this call stack, or D208694 would have changed the signature. So there's something else going on. I'm putting this back into triage (and taking myself off the Bug) so we can re-discuss.

Comment 12

[Jim Mathies [:jimm]]

This is occurring in the RDD process quite a bit.

https://mozilla.github.io/process-top-crashes/rdd_release.html
https://sql.telemetry.mozilla.org/queries/78917/source?p_channel=release#196128

Comment 13

[Jeff Muizelaar [:jrmuizel]]

It looks like we still have bad debug info:
e.g.

0 	xul.dll 	gfxWindowsPlatform::RecordContentDeviceFailure(mozilla::gfx::TelemetryDeviceCode) 	mfbt/Maybe.h:953 	inlined

This looks like another instance of the problem I ran into here: https://github.com/mstange/samply/issues/5

Markus wrote some analysis here: https://github.com/mstange/samply/issues/5#issuecomment-1101746227

Serge, do you have time to take a look at what might be going on here?

Comment 14

[[:sergesanspaille]]

Jeff, I'd be happy to help, but I'm pretty unsure about how to reproduce. It would be very helpful if you could provide direction to pinpoint at the wrong debug info from a fresh build. I could start working on it from there. I'll still give it a try this morning though.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

This is actually over 60% of our RDD crashes on release, or 10% of our total crash volume. I'm triaging it back to S2 based on that, but feel free to disagree as long as you include the data from the previous sentence in your assessment :-)

Comment 16

[Andrew McCreight [:mccr8]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #15)

This is actually over 60% of our RDD crashes on release, or 10% of our total crash volume. I'm triaging it back to S2 based on that, but feel free to disagree as long as you include the data from the previous sentence in your assessment :-)

Where are you seeing that? Looking at crash stats, I barely see any crashes for this signature. 79 in the last week, and 43% of those are Nightly. Is that from crash pings or something?

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Is that from crash pings or something?

Yes. Crash stats are never very reliable because they're a biased sample, but they're useless for anything that isn't the main process or a content process, because almost no-one is opting in to sending those crash reports. So if you get any crash reports, odds are it's a topcrasher already.

See the announcement in moz.dev.platform about the crash ping dashboards.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit BugBot documentation.

Comment 19

[Andrew Osmond [:aosmond] (he/him)]

I think during init we hit this path, where we reset mDeviceStatus but might successfully get a new pointer for mAdapter:
https://searchfox.org/mozilla-central/rev/0b189f017bc9d48b62012205c5b9f8a8b560497b/gfx/thebes/DeviceManagerDx.cpp#717

Then we crash here:
https://searchfox.org/mozilla-central/rev/0b189f017bc9d48b62012205c5b9f8a8b560497b/gfx/thebes/DeviceManagerDx.cpp#990

Comment 20

[Bob Hood [:bhood]]

Jeff, are you able to help Serge per his request in comment #14?

Comment 21

[Brad Werth [:bradwerth]]

Thanks to Andrew's analysis in Comment 19, I think I can build a patch to fix this.

Comment 22

[Brad Werth [:bradwerth]]

Attachment: Bug 1887703 Part 3: Protect more cases where mDeviceStatus is dereferenced, and avoid double-locks.

This adds a method IsWARPLocked to cover the cases where the lock is
already held, and the DeviceManagerDx methods are preferred over the
direct access to the Device (to handle this case correctly).

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7682f855507a
Part 3: Protect more cases where mDeviceStatus is dereferenced, and avoid double-locks. r=aosmond

Comment 24

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7682f855507a

Comment 25

[Bob Hood [:bhood]]

Flagging as stalled while we observe the effects of the patch. Please remove the keyword if work begins again on the report.

Comment 26

[Brad Werth [:bradwerth]]

Can't tell yet if this is completely sorted out -- I'm not great at interpreting the crash reports to see if the patch has been applied. But no crashes in Nightly since April 10.

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

Since the crash volume is low (less than 15 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit BugBot documentation.

Comment 28

[Brad Werth [:bradwerth]]

I believe this is now fixed. No crashes in Nightly for the past month. If I'm mistaken here, I'm believe this Bug will be automatically re-opened, and I am happy to keep working on it.

Comment 29

[Andrew McCreight [:mccr8]]

A bot will reopen intermittent failures if they reoccur and get flagged, but not crash-stats bugs like this. That being said, it sounds perfectly fine to close this.

Comment 30

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit BugBot documentation.