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

An installed family was added to the font database one file at a time, so a
lookup on another thread could find it with only some of its styles: text
falling back to Arial took Arial Bold Italic or Bold (its files sort first)
for upright Regular text, and the same text laid out a moment later took
Arial Regular. One Arabic word could even mix the two. Two threads loading
the same family could also add a face twice.

A family's files are now read and parsed first and their faces added under
one lock, skipping faces another thread added meanwhile. Loading a family
reports it loaded when another thread loaded it meanwhile, so the fallback
search doesn't go on to other families.

This made tests_bidi::a_change_of_paint_in_a_mixed_direction_line_moves_no_glyph
fail intermittently when another test loaded Arial at the same time.

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);
-        let mut added = 0;
-        for (i, spec) in enumerate_faces(&data) {
-            // 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 version = skrifa::FontRef::from_index(&data, i).ok().and_then(|f| name(&f, &[StringId::VERSION_STRING])).unwrap_or_default();
-            if self
-                .read_faces()
-                .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)) {
-                self.faces.write().unwrap_or_else(|e| e.into_inner()).push(Arc::new(f));
-                added += 1;
+        self.add_fonts([(bytes, path.map(Path::to_path_buf))])
+    }
+
+    /// 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();
+                let present = |f: &FontFace| same(f, &spec.family, &spec.style, &version);
+                if self.read_faces().iter().any(|f| present(f)) || made.iter().any(present) {
+                    continue;
+                }
+                if let Some(f) = make_face(FontBytes::Owned(data.clone()), i, spec, path.clone()) {
+                    made.push(f);
+                }
             }
         }
+        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;
-        for p in paths {
-            for data in font_files(&p) {
-                any |= self.add_font_from(data, Some(&p)) > 0;
-            }
-        }
-        any
+        let files = paths.into_iter().flat_map(|p| font_files(&p).into_iter().map(move |data| (data, Some(p.clone()))));
+        self.add_fonts(files) > 0 || self.is_loaded(family)
     }
 
     /// 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");