From 074e090f050ed98c63e7fd36dcaa889ea4f66865 Mon Sep 17 00:00:00 2001 From: lloyd Date: Sat, 10 Oct 2026 17:50:21 +0200 Subject: [PATCH] Fonts: a lookup finds an installed family whichever thread loads it first Two threads resolving a family nobody had loaded yet raced. The one that lost the race saw add_font add nothing and fell back to another face, and a lookup made while a family was being added saw only part of it (Arial Bold Italic loads first, so "Regular" came back bold italic). The same text was then laid out in different fonts depending on timing, which made clicks_in_flipped_shapes_hit_the_text_where_it_is_drawn flaky when the theme font isn't bundled. Loading installed fonts now happens under one lock and rechecks once it holds it, and a family's faces are added in one step. Co-Authored-By: Claude Opus 5.5 --- crates/fonts/src/fontdb.rs | 75 ++++++++++++++++++++++++++++++-------- crates/fonts/src/lib.rs | 28 ++++++++++++++ 2 files changed, 87 insertions(+), 16 deletions(-) diff --git a/crates/fonts/src/fontdb.rs b/crates/fonts/src/fontdb.rs index f0a06efe..426b95c9 100644 --- a/crates/fonts/src/fontdb.rs +++ b/crates/fonts/src/fontdb.rs @@ -245,6 +245,11 @@ pub struct FontDb { /// System fallback state: enabled, and characters no system font covers. #[cfg(not(target_arch = "wasm32"))] sys: Mutex, + /// Held while installed fonts are loaded, so threads that need the same family wait for one + /// load instead of racing it: what a lookup finds doesn't depend on which thread got there + /// first. + #[cfg(not(target_arch = "wasm32"))] + loading: Mutex<()>, } #[cfg(not(target_arch = "wasm32"))] @@ -535,6 +540,8 @@ impl FontDb { cataloged: std::sync::OnceLock::new(), #[cfg(not(target_arch = "wasm32"))] sys: Mutex::new(SysFallback { enabled: true, ..Default::default() }), + #[cfg(not(target_arch = "wasm32"))] + loading: Mutex::new(()), } } @@ -600,14 +607,33 @@ impl FontDb { /// Add a user font (TTF/OTF/TTC bytes). Returns the number of faces added (0 if unparseable or /// every face was already present). pub fn add_font(&self, bytes: Vec) -> usize { - let data = Arc::new(bytes); - let mut added = 0; - for (i, family, style, coords) in enumerate_faces(&data) { - if self.read_faces().iter().any(|f| f.family.eq_ignore_ascii_case(&family) && f.style.eq_ignore_ascii_case(&style)) { - continue; + self.add_fonts(vec![bytes]) + } + + /// Add font files together: lookups see all of their faces or none, never part of a family. + /// Returns the number of faces added. + fn add_fonts(&self, files: Vec>) -> usize { + let present = |faces: &[Arc], family: &str, style: &str| { + faces.iter().any(|f| f.family.eq_ignore_ascii_case(family) && f.style.eq_ignore_ascii_case(style)) + }; + let mut new = Vec::new(); + for bytes in files { + let data = Arc::new(bytes); + for (i, family, style, coords) in enumerate_faces(&data) { + if present(&self.read_faces(), &family, &style) { + continue; + } + if let Some(f) = make_face(FontBytes::Owned(data.clone()), i, family, style, coords) { + new.push(Arc::new(f)); + } } - if let Some(f) = make_face(FontBytes::Owned(data.clone()), i, family, style, coords) { - self.faces.write().unwrap_or_else(|e| e.into_inner()).push(Arc::new(f)); + } + let mut faces = self.faces.write().unwrap_or_else(|e| e.into_inner()); + let mut added = 0; + for f in new { + // Checked again under the lock: another thread may have added the same face. + if !present(&faces, &f.family, &f.style) { + faces.push(f); added += 1; } } @@ -677,9 +703,21 @@ impl FontDb { n } - /// Load the files of the installed `family`. Returns whether any face was added. + /// Load the files of the installed `family` unless it is loaded. Returns whether it is loaded + /// (by this call or any other). #[cfg(not(target_arch = "wasm32"))] fn load_cataloged(&self, family: &str) -> bool { + let loading = self.loading.lock().unwrap_or_else(|e| e.into_inner()); + self.load_family(family, &loading) + } + + /// [`FontDb::load_cataloged`] for a caller already holding `loading`. + #[cfg(not(target_arch = "wasm32"))] + fn load_family(&self, family: &str, _loading: &std::sync::MutexGuard<'_, ()>) -> bool { + // Another thread may have loaded it while this one waited for the lock. + if self.is_loaded(family) { + return true; + } let paths: Vec = { let cat = self.read_catalog(); let mut p: Vec<_> = cat.iter().filter(|c| c.family.eq_ignore_ascii_case(family)).map(|c| c.path.clone()).collect(); @@ -687,13 +725,8 @@ impl FontDb { p.dedup(); p }; - let mut any = false; - for p in paths { - if let Ok(data) = std::fs::read(&p) { - any |= self.add_font(data) > 0; - } - } - any + self.add_fonts(paths.iter().filter_map(|p| std::fs::read(p).ok()).collect()); + self.is_loaded(family) } /// Resolve a family + style to a face, falling back to the closest style of the family, then to @@ -846,6 +879,7 @@ impl FontDb { if c.is_control() || c.is_whitespace() { return false; } + let seen = self.read_faces().len(); { let sys = self.sys.lock().unwrap_or_else(|e| e.into_inner()); if !sys.enabled || sys.misses.contains(&c) { @@ -854,9 +888,18 @@ impl FontDb { } let covered = |db: &FontDb| db.read_faces().iter().any(|f| f.covers(c)); let cataloged: Vec = self.read_catalog().iter().map(|e| e.family.clone()).collect(); + let loading = self.loading.lock().unwrap_or_else(|e| e.into_inner()); + // Another thread may have loaded a face for `c` (faces are only ever added), or found + // there is none, while this one waited. + if self.read_faces().len() != seen && covered(self) { + return true; + } + if self.sys.lock().unwrap_or_else(|e| e.into_inner()).misses.contains(&c) { + return false; + } for fam in SYSTEM_FALLBACKS { if !self.is_loaded(fam) && cataloged.iter().any(|f| f.eq_ignore_ascii_case(fam)) { - self.load_cataloged(fam); + self.load_family(fam, &loading); if covered(self) { return true; } diff --git a/crates/fonts/src/lib.rs b/crates/fonts/src/lib.rs index 94e8081f..2587e7b0 100644 --- a/crates/fonts/src/lib.rs +++ b/crates/fonts/src/lib.rs @@ -152,6 +152,34 @@ mod tests { assert!(!shape(&f, "Hello", &[], |c| c).is_empty()); } + #[test] + fn concurrent_lookups_all_find_an_installed_family() { + // Threads resolving an installed family nobody had loaded yet used to fall back to + // another face when a different thread finished loading it first, so the same text was + // laid out in different fonts depending on timing. + let dir = std::env::temp_dir().join(format!("deckcraft-fonts-concurrent-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("Hack-Regular.ttf"), epaint_default_fonts::HACK_REGULAR).unwrap(); + for _ in 0..20 { + let db = FontDb::with_font_dirs(vec![dir.clone()]); + db.set_system_fallback(false); + let start = std::sync::Barrier::new(8); + let got: Vec = std::thread::scope(|s| { + let threads: Vec<_> = (0..8) + .map(|_| { + s.spawn(|| { + start.wait(); + db.face("Hack", "Regular").family.clone() + }) + }) + .collect(); + threads.into_iter().map(|t| t.join().unwrap()).collect() + }); + assert!(got.iter().all(|f| f == "Hack"), "{got:?}"); + } + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn office_fonts_have_substitutes() { assert!(substitutes("Calibri").contains(&"Carlito"));