Closed Bug 1904139 Opened 1 month ago Closed 27 days ago

Crash in [@ atomic_refcell::AtomicRefCell<T>::borrow_mut] with reentrancy into restyling via `gfxPlatformFontList::InitFontList()`

Categories

(Core :: CSS Parsing and Computation, defect)

defect

Tracking

()

RESOLVED FIXED
129 Branch
Tracking Status
firefox-esr115 --- wontfix
firefox-esr128 --- affected
firefox127 --- wontfix
firefox128 --- wontfix
firefox129 --- fixed

People

(Reporter: dholbert, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

[spinning this off from bug 1903930 comment 1]

Crash report: https://crash-stats.mozilla.org/report/index/1280ec23-9dd4-47af-aa26-ec0ff0240620

MOZ_CRASH Reason: already mutably borrowed

Top 10 frames:

0  libxul.so  <name omitted>  /usr/src/debug/firefox-127.0-1.fc40.x86_64/objdir/dist/include/mozilla/Assertions.h:317
0  libxul.so  RustMozCrash  /usr/src/debug/firefox-127.0-1.fc40.x86_64/mozglue/static/rust/wrappers.cpp:18
1  libxul.so  mozglue_static::panic_hook  mozglue/static/rust/lib.rs:98
1  libxul.so  core::ops::function::Fn::call  /builddir/build/BUILD/rustc-1.78.0-src/library/core/src/ops/function.rs:79
2  libxul.so  <name omitted>  library/alloc/src/boxed.rs:2034
2  libxul.so  std::panicking::rust_panic_with_hook  library/std/src/panicking.rs:783
3  libxul.so  std::panicking::begin_panic_handler::{{closure}}  library/std/src/panicking.rs:657
4  libxul.so  std::sys_common::backtrace::__rust_end_short_backtrace  library/std/src/sys_common/backtrace.rs:171
5  libxul.so  rust_begin_unwind  library/std/src/panicking.rs:645
6  libxul.so  core::panicking::panic_fmt  library/core/src/panicking.rs:72

Note: those top 10 frames were auto-populated but they're all just from rust panicking. The really interesting part is further up in the backtrace:

7 	libxul.so 	core::panicking::panic_display
8 	libxul.so 	atomic_refcell::AtomicRefCell<T>::borrow_mut
8 	libxul.so 	<style::gecko::wrapper::GeckoElement as style::dom::TElement>::mutate_data::{{closure}}
9 	libxul.so 	mozilla::RestyleManager::RebuildAllStyleData(nsChangeHint, mozilla::StyleRestyleHint)
[...]
13 	gfxPlatform::ForceGlobalReflow(gfxPlatform::NeedsReframe, gfxPlatform::BroadcastToChildren)
[...]
15 	libxul.so 	gfxPlatformFontList::InitFontList()
[...]
20 	libxul.so 	mozilla::ReflowInput::CalcLineHeight(mozilla::StyleGenericLineHeight<float, mozilla::StyleCSSPixelLength> const&, nsStyleFont const&, nsPresContext*, bool, nsIContent const*, int, float)
[...]
20 	libxul.so 	Gecko_CalcLineHeight
21 	libxul.so 	style::gecko::media_queries::Device::calc_line_height
21 	libxul.so 	style::matching::MatchMethods::finish_restyle
21 	libxul.so 	style::traversal::compute_style
[...]
25 	libxul.so 	Servo_TraverseSubtree
32 	libxul.so 	mozilla::css::SheetLoadData::SheetFinishedParsingAsync()

It looks like when we're restyling, we hit a media query which needs to know the line-height, which in turn needs to know the font, which triggers gfxPlatformFontList::InitFontList(), which (when it completes) invalidates styles everywhere and calls RebuildAllStyleData which puts us back into restyling code; and we end up trying to borrow some style-related struct that we've already got a reference to further up on the stack.

See Also: → 1903930

(note, bug 1800306 tracks this crash signature in general. Marking this as blocking that bug. There are probably different ways of hitting this crash, so let's have this bug here focused on the reentrancy described in comment 0 for now.)

Blocks: 1800306

[transferring ni=emilio from bug 1903930]

Flags: needinfo?(emilio)

Thanks for doing this, I was planning to do this but ran out of time... But, yeah, basically, gfxFontGroup::GetDefaultFont() triggering font list initialization (and thus restyling) during styling itself is wrong...

There are multiple solutions here... First of all, if it's the first font-list initialization, do we even need to invalidate? If it's a font-list update, then we should either:

  • Defer the invalidation (not amazing).
  • Do it a bit eagerly (maybe during here, which is where we update the user font-set).

The later is probably trivial-ish.

Flags: needinfo?(emilio)
Severity: -- → S3

Even on the main thread we can be at a bad time to post global font
updates.

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc0f7d3e6a3d
Don't re-initialize platform font list from GetDefaultFont. r=jfkthame
Status: ASSIGNED → RESOLVED
Closed: 27 days ago
Resolution: --- → FIXED
Target Milestone: --- → 129 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: