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()`


(Core :: CSS Parsing and Computation, defect)




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


(Reporter: dholbert, Assigned: emilio)


(Blocks 1 open bug)


(Keywords: crash)

Crash Data


(1 file)

[spinning this off from bug 1903930 comment 1]

Crash report:

MOZ_CRASH Reason: already mutably borrowed

Top 10 frames:

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

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

Assignee: nobody → emilio
Pushed by
Don't re-initialize platform font list from GetDefaultFont. r=jfkthame
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.