Merge pull request #3921 from Taha-Mahmoodi/fix/font-family-load-race

Fix #3920: a family another thread is loading is found whole, so fallback text never takes the style of its first file

crates/text/src/fontdb.rs

@@ -929,6 +929,30 @@ pub(crate) fn alias_lists() -> u64 {
     ALIAS_LISTS.with(std::cell::Cell::get)
 }
 
+#[cfg(test)]
+thread_local! {
+    /// Tests: run on this thread each time it publishes faces to a database, to look the database
+    /// up at exactly that point (as another thread could).
+    static ON_FACES_PUBLISHED: std::cell::RefCell<Option<Box<dyn FnMut()>>> = const { std::cell::RefCell::new(None) };
+}
+
+/// Tests: run `hook` each time this thread publishes faces to a database.
+#[cfg(test)]
+pub(crate) fn on_faces_published(hook: Option<Box<dyn FnMut()>>) {
+    ON_FACES_PUBLISHED.with(|h| *h.borrow_mut() = hook);
+}
+
+#[cfg(test)]
+fn faces_published() {
+    // Taken out while it runs, so a hook that adds faces itself doesn't run again.
+    if let Some(mut hook) = ON_FACES_PUBLISHED.with(|h| h.borrow_mut().take()) {
+        hook();
+        ON_FACES_PUBLISHED.with(|h| {
+            h.borrow_mut().get_or_insert(hook);
+        });
+    }
+}
+
 /// How a requested family and style resolved ([`FontDb::resolve`]).
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum FontMatch {
@@ -1890,24 +1914,45 @@ impl FontDb {
 
     /// [`FontDb::add_font`] for the bytes of the file at `path` (when known).
     fn add_font_from(&self, bytes: Vec<u8>, path: Option<&Path>) -> usize {
-        let data = Arc::new(bytes);
+        self.add_fonts([(bytes, path.map(Path::to_path_buf))])
-        let mut added = 0;
+    }
-        for (i, spec) in enumerate_faces(&data) {
+
+    /// Add the faces of `files` (each file's bytes, and the file when known) all at once: a lookup
+    /// on another thread finds all of them or none, never a family with only some of its styles
+    /// loaded, whose nearest style it would take instead (Arial Bold Italic, the first of Arial's
+    /// files, for upright Regular text). Returns the number of faces added.
+    fn add_fonts(&self, files: impl IntoIterator<Item = (Vec<u8>, Option<PathBuf>)>) -> usize {
         // Another version of a family and style is kept: a document may name it (the first one
         // loaded stays the one the family and style alone resolve to).
+        let same = |f: &FontFace, family: &str, style: &str, version: &str| {
+            f.family.eq_ignore_ascii_case(family) && f.style.eq_ignore_ascii_case(style) && f.version == version
+        };
+        let mut made: Vec<FontFace> = Vec::new();
+        for (bytes, path) in files {
+            let data = Arc::new(bytes);
+            for (i, spec) in enumerate_faces(&data) {
                 let version = skrifa::FontRef::from_index(&data, i).ok().and_then(|f| name(&f, &[StringId::VERSION_STRING])).unwrap_or_default();
-            if self
+                let present = |f: &FontFace| same(f, &spec.family, &spec.style, &version);
-                .read_faces()
+                if self.read_faces().iter().any(|f| present(f)) || made.iter().any(present) {
-                .iter()
-                .any(|f| f.family.eq_ignore_ascii_case(&spec.family) && f.style.eq_ignore_ascii_case(&spec.style) && f.version == version)
-            {
                     continue;
                 }
-            if let Some(f) = make_face(FontBytes::Owned(data.clone()), i, spec, path.map(Path::to_path_buf)) {
+                if let Some(f) = make_face(FontBytes::Owned(data.clone()), i, spec, path.clone()) {
-                self.faces.write().unwrap_or_else(|e| e.into_inner()).push(Arc::new(f));
+                    made.push(f);
-                added += 1;
                 }
             }
+        }
+        let mut faces = self.faces.write().unwrap_or_else(|e| e.into_inner());
+        let before = faces.len();
+        for f in made {
+            // Another thread may have added the same face meanwhile.
+            if !faces.iter().any(|g| same(g, &f.family, &f.style, &f.version)) {
+                faces.push(Arc::new(f));
+            }
+        }
+        let added = faces.len() - before;
+        drop(faces);
+        #[cfg(test)]
+        faces_published();
         if added > 0 {
             self.changed();
         }
@@ -2183,20 +2228,17 @@ impl FontDb {
         self.postscript.read().unwrap_or_else(|e| e.into_inner()).get(&norm(name)).cloned()
     }
 
-    /// Load the files of the installed `family`. Returns whether any face was added.
+    /// Load the files of the installed `family`, adding all their faces at once
+    /// ([`Self::add_fonts`]). Returns whether the family is loaded now: by this call, or by another
+    /// thread meanwhile.
     #[cfg(not(target_arch = "wasm32"))]
     fn load_cataloged(&self, family: &str) -> bool {
         let mut paths: Vec<PathBuf> =
             self.read_catalog().get(&family.to_ascii_lowercase()).map(|c| c.faces.iter().map(|cf| cf.path.clone()).collect()).unwrap_or_default();
         paths.sort();
         paths.dedup();
-        let mut any = false;
+        let files = paths.into_iter().flat_map(|p| font_files(&p).into_iter().map(move |data| (data, Some(p.clone()))));
-        for p in paths {
+        self.add_fonts(files) > 0 || self.is_loaded(family)
-            for data in font_files(&p) {
-                any |= self.add_font_from(data, Some(&p)) > 0;
-            }
-        }
-        any
     }
 
     /// Resolve a family + style to a face, falling back to the closest style of the family, then to

crates/text/src/tests_sysfonts.rs

@@ -194,6 +194,50 @@ fn rescanning_retries_characters_no_font_covered() {
     assert_eq!(db.face_covering(C).unwrap().family, "Retry Sans");
 }
 
+/// A lookup on another thread while an installed family loads finds the whole family or none of
+/// it: never only the face of its first file, which falling back takes for text in the others' style
+/// (Arial Bold Italic for upright Regular Arabic, the same text laid out again moments later in
+/// Arial Regular).
+#[test]
+fn a_family_loading_on_another_thread_is_not_found_in_part() {
+    // A private-use character none of the bundled fonts has.
+    const C: char = '\u{F0000}';
+    let dir = std::env::temp_dir().join(format!("vc-sysfonts-{}-whole-family", std::process::id()));
+    let _ = std::fs::remove_dir_all(&dir);
+    std::fs::create_dir_all(&dir).unwrap();
+    // [`FAMILY`] Bold and Regular, their character maps replaced by one holding only `C`. The Bold's
+    // file sorts first, so it is read first.
+    for (file, source) in [("Sysfont-Bold.ttf", "SourceSans3-Bold.ttf"), ("Sysfont-Regular.ttf", "SourceSans3-Regular.ttf")] {
+        let base = renamed(source);
+        let font = skrifa::FontRef::new(&base).unwrap();
+        let mut builder = write_fonts::FontBuilder::new();
+        builder.add_raw(write_fonts::types::Tag::new(b"cmap"), crate::embed::cmap(&[(C as u32, 1)]).unwrap());
+        for r in font.table_directory.table_records() {
+            let tag = write_fonts::types::Tag::new(&r.tag().to_be_bytes());
+            if !builder.contains(tag) {
+                builder.add_raw(tag, font.table_data(r.tag()).unwrap().as_bytes());
+            }
+        }
+        std::fs::write(dir.join(file), builder.build()).unwrap();
+    }
+    let db = std::sync::Arc::new(FontDb::with_font_dirs(vec![dir.clone()]));
+    // Each time this thread publishes faces, another thread falls back for `C` right then.
+    let seen = std::sync::Arc::new(std::sync::Mutex::new(Vec::new()));
+    let (hook_db, hook_seen) = (db.clone(), seen.clone());
+    crate::fontdb::on_faces_published(Some(Box::new(move || {
+        let db = hook_db.clone();
+        let style = std::thread::spawn(move || db.fallback_for(C, 0).map(|f| f.style.clone())).join().unwrap();
+        hook_seen.lock().unwrap().push(style);
+    })));
+    let face = db.face(FAMILY, "Regular").map(|f| f.style.clone());
+    crate::fontdb::on_faces_published(None);
+    assert_eq!(face.as_deref(), Some("Regular"));
+    let seen = seen.lock().unwrap().clone();
+    assert!(!seen.is_empty());
+    assert!(seen.iter().all(|s| s.as_deref() == Some("Regular")), "the lookups took the family's {seen:?} faces");
+    std::fs::remove_dir_all(dir).unwrap();
+}
+
 #[test]
 fn rescanning_finds_fonts_installed_since() {
     let dir = font_dir("rescan");