1826688 - Crash in [@ servo_arc::impl$44::deref] with mozilla::a11y::StyleInfo::Display() on the stack

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Looks like this started in Fx111.

Crash report: https://crash-stats.mozilla.org/report/index/1eca3faa-c589-4087-abc1-46e020230405

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  servo_arc::impl$44::deref  servo/components/servo_arc/lib.rs:1118
0  xul.dll  style::gecko_bindings::structs::root::ServoComputedData::get_box  i686-pc-windows-msvc/release/build/style-2d9d766621675013/out/gecko_properties.rs:579
0  xul.dll  style::gecko_properties::ComputedValues::clone_display  i686-pc-windows-msvc/release/build/style-2d9d766621675013/out/properties.rs:65369
0  xul.dll  style::gecko_properties::ComputedValues::get_resolved_value  i686-pc-windows-msvc/release/build/style-2d9d766621675013/out/properties.rs:68956
1  xul.dll  geckoservo::glue::Servo_GetPropertyValue  servo/ports/geckolib/glue.rs:6804
2  xul.dll  mozilla::ComputedStyle::GetComputedPropertyValue const  layout/style/ComputedStyle.h:69
2  xul.dll  mozilla::a11y::StyleInfo::Display  accessible/base/StyleInfo.cpp:23
3  xul.dll  mozilla::a11y::LocalAccessible::DisplayStyle const  accessible/generic/LocalAccessible.cpp:3929
4  xul.dll  mozilla::a11y::BlockRule::Match  accessible/base/TextLeafRange.cpp:430
5  xul.dll  mozilla::a11y::Pivot::SearchBackward  accessible/base/Pivot.cpp:88

Comment 1

[Mike Conley (:mconley) (:⚙️)]

We might have some STR here, based on the accessibility review from Nathan for the new migration wizard:

With NVDA on, hovering the Cancel button in the Import Data dialog crashes Nightly.

Selecting the Cancel button with keyboard navigation doesn't crash the browser.
Hovering the Cancel button with NVDA off does not crash Nightly.
This only happens with NVDA running. Setting accessibility.force_disabled to -1 in about:config isn’t enough to trigger the problem.
crash report

Similar to the above: with NVDA on, hovering the Done button after a successful import crashes Nightly.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

StyleInfo seems to expect its mComputedStyle member to be non-null, but I don't think that's guaranteed and in fact the A11y code already has some workarounds here.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1826688 - Shrink down StyleInfo. r=jamie,morgan

Most of the methods have only one caller that guarantees it has a
primary frame (good!).

But others didn't quite guarantee that.

Comment 4

[James Teh [:Jamie]]

I always assumed a crash here was due to a11y using StyleInfo inappropriately; i.e. layout expects that mComputedStyle should not be null if you want to mess with StyleInfo. This does raise the question of why mComputedStyle is null in this case.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69a22f48b42d
Shrink down StyleInfo. r=morgan

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/69a22f48b42d

Comment 7

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

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox113 to wontfix.

For more information, please visit auto_nag documentation.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9327777 [details]
Bug 1826688 - Shrink down StyleInfo. r=jamie,morgan

Beta/Release Uplift Approval Request

• User impact if declined: crashes with a11y enabled
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: It seems there are some STR in comment 1
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Mostly adding null-checks in practice.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 9

[Pascal Chevrel:pascalc]

Comment on attachment 9327777 [details]
Bug 1826688 - Shrink down StyleInfo. r=jamie,morgan

Approved for 113 beta 3, thanks.

Comment 10

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/6a422426c256

Comment 11

[Anca Soncutean, Desktop QA]

I can constantly reproduce the crash with the steps mentioned in comment 1, with browser.migrate.content-modal.enabled set on true on Nightly 113.0a1 (2023-03-31), but no crash report is generated.
There is no crash with the latest Nightly 114.0a1 (2023-04-18) and Beta 113.0b5 on Windows 10, therefore this issue looks fixed on my end, but without having an actually crash report maybe a second verification should be considered. If time permits maybe Nathan can confirm as well if this crash signature is no longer reproducible indeed. Thank you!

Comment 12

[Nathan LaPré]

I can confirm Anca's report; the issue appears fixed for me, too. I can no longer reproduce the crash using the steps I originally posted. Thanks for fixing this!

Comment 13

[Anca Soncutean, Desktop QA]

Based on comments 11 and 12 I will mark this as verified fixed.