fonts: Add FontIdentifier and LocalFontIdentifier (#31658)

Instead of using a simple `Atom` to identify a local font, use a data
structure. This allows us to carry more information necessary to
identify a local font (such as a path on MacOS). We need this for the
new version of WebRender, as fonts on MacOS now require a path.

This has a lot of benefits:
 1. We can avoid loading fonts without paths on MacOS, which should
    avoid a lot of problems with flakiness and ensure we always load the
    same font for a given identifier.
 2. This clarifies the difference between web fonts and local fonts,
    though there is more work to do here.
 3. This avoid a *lot* of font shenanigans, such as trying to work
    backwards from the name of the font to the path of the font we
    actually matched. In general, we can remove a lot of code trying to
    accomplish these shenanigans.
 4. Getting the font bytes always returns an `Arc` now avoiding an extra
    full font copy in the case of Canvas.

components/canvas/canvas_data.rs

@@ -501,8 +501,7 @@ impl<'a> CanvasData<'a> {
                         .first(font_context)
                         .expect("couldn't find font");
                     let font = font.borrow_mut();
-                    let template = font.handle.template();
+                    Font::from_bytes(font.handle.template().bytes(), 0)
-                    Font::from_bytes(Arc::new(template.bytes()), 0)
                         .ok()
                         .or_else(|| load_system_font_from_style(Some(style)))
                 })

components/gfx/font.rs

@@ -24,6 +24,7 @@ use style::values::computed::font::{GenericFontFamily, SingleFontFamily};
 use unicode_script::Script;
 use webrender_api::FontInstanceKey;
 
+use crate::font_cache_thread::FontIdentifier;
 use crate::font_context::{FontContext, FontSource};
 use crate::font_template::FontTemplateDescriptor;
 use crate::platform::font::{FontHandle, FontTable};
@@ -78,7 +79,7 @@ pub trait FontHandleMethods: Sized {
     fn table_for_tag(&self, _: FontTableTag) -> Option<FontTable>;
 
     /// A unique identifier for the font, allowing comparison.
-    fn identifier(&self) -> Atom;
+    fn identifier(&self) -> &FontIdentifier;
 }
 
 // Used to abstract over the shaper's choice of fixed int representation.
@@ -202,7 +203,7 @@ impl Font {
     }
 
     /// A unique identifier for the font, allowing comparison.
-    pub fn identifier(&self) -> Atom {
+    pub fn identifier(&self) -> &FontIdentifier {
         self.handle.identifier()
     }
 }

components/gfx/font_cache_thread.rs

@@ -28,7 +28,8 @@ use crate::font_context::FontSource;
 use crate::font_template::{FontTemplate, FontTemplateDescriptor};
 use crate::platform::font_context::FontContextHandle;
 use crate::platform::font_list::{
-    for_each_available_family, for_each_variation, system_default_family, SANS_SERIF_FONT_FAMILY,
+    for_each_available_family, for_each_variation, system_default_family, LocalFontIdentifier,
+    SANS_SERIF_FONT_FAMILY,
 };
 use crate::platform::font_template::FontTemplateData;
 
@@ -50,9 +51,15 @@ pub struct SerializedFontTemplateInfo {
     pub font_key: FontKey,
 }
 
+#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
+pub enum FontIdentifier {
+    Local(LocalFontIdentifier),
+    Web(ServoUrl),
+}
+
 #[derive(Debug, Deserialize, Serialize)]
 pub struct SerializedFontTemplate {
-    identifier: Atom,
+    identifier: FontIdentifier,
     bytes_receiver: ipc_channel::ipc::IpcBytesReceiver,
 }
 
@@ -110,7 +117,7 @@ impl FontTemplates {
         None
     }
 
-    pub fn add_template(&mut self, identifier: Atom, maybe_data: Option<Vec<u8>>) {
+    pub fn add_template(&mut self, identifier: FontIdentifier, maybe_data: Option<Vec<u8>>) {
         for template in &self.templates {
             if *template.identifier() == identifier {
                 return;
@@ -155,7 +162,7 @@ struct FontCache {
     font_context: FontContextHandle,
     core_resource_thread: CoreResourceThread,
     webrender_api: Box<dyn WebrenderApi>,
-    webrender_fonts: HashMap<Atom, FontKey>,
+    webrender_fonts: HashMap<FontIdentifier, FontKey>,
     font_instances: HashMap<(FontKey, Au), FontInstanceKey>,
 }
 
@@ -235,7 +242,7 @@ impl FontCache {
                 },
                 Command::AddDownloadedWebFont(family_name, url, bytes, result) => {
                     let templates = &mut self.web_families.get_mut(&family_name).unwrap();
-                    templates.add_template(Atom::from(url.to_string()), Some(bytes));
+                    templates.add_template(FontIdentifier::Web(url), Some(bytes));
                     drop(result.send(()));
                 },
                 Command::Ping => (),
@@ -346,9 +353,9 @@ impl FontCache {
                 let font_face_name = LowercaseString::new(&font.name);
                 let templates = &mut self.web_families.get_mut(&family_name).unwrap();
                 let mut found = false;
-                for_each_variation(&font_face_name, |path| {
+                for_each_variation(&font_face_name, |local_font_identifier| {
                     found = true;
-                    templates.add_template(Atom::from(&*path), None);
+                    templates.add_template(FontIdentifier::Local(local_font_identifier), None);
                 });
                 if found {
                     sender.send(()).unwrap();
@@ -389,8 +396,8 @@ impl FontCache {
             let s = self.local_families.get_mut(&family_name).unwrap();
 
             if s.templates.is_empty() {
-                for_each_variation(&family_name, |path| {
+                for_each_variation(&family_name, |local_font_identifier| {
-                    s.add_template(Atom::from(&*path), None);
+                    s.add_template(FontIdentifier::Local(local_font_identifier), None);
                 });
             }
 
@@ -430,9 +437,9 @@ impl FontCache {
             .entry(template.identifier.clone())
             .or_insert_with(|| {
                 let font = match (template.bytes_if_in_memory(), template.native_font()) {
-                    (Some(bytes), _) => FontData::Raw(bytes),
+                    (Some(bytes), _) => FontData::Raw((*bytes).clone()),
                     (None, Some(native_font)) => FontData::Native(native_font),
-                    (None, None) => FontData::Raw(template.bytes()),
+                    (None, None) => FontData::Raw((*template.bytes()).clone()),
                 };
                 webrender_api.add_font(font)
             });

components/gfx/font_template.rs

@@ -7,13 +7,13 @@ use std::io::Error as IoError;
 use std::sync::{Arc, Weak};
 
 use serde::{Deserialize, Serialize};
-use servo_atoms::Atom;
 use style::computed_values::font_stretch::T as FontStretch;
 use style::computed_values::font_style::T as FontStyle;
 use style::properties::style_structs::Font as FontStyleStruct;
 use style::values::computed::font::FontWeight;
 
 use crate::font::FontHandleMethods;
+use crate::font_cache_thread::FontIdentifier;
 use crate::platform::font::FontHandle;
 use crate::platform::font_context::FontContextHandle;
 use crate::platform::font_template::FontTemplateData;
@@ -85,7 +85,7 @@ impl<'a> From<&'a FontStyleStruct> for FontTemplateDescriptor {
 /// font instance handles. It contains a unique
 /// FontTemplateData structure that is platform specific.
 pub struct FontTemplate {
-    identifier: Atom,
+    identifier: FontIdentifier,
     descriptor: Option<FontTemplateDescriptor>,
     weak_ref: Option<Weak<FontTemplateData>>,
     // GWTODO: Add code path to unset the strong_ref for web fonts!
@@ -103,7 +103,10 @@ impl Debug for FontTemplate {
 /// is common, regardless of the number of instances of
 /// this font handle per thread.
 impl FontTemplate {
-    pub fn new(identifier: Atom, maybe_bytes: Option<Vec<u8>>) -> Result<FontTemplate, IoError> {
+    pub fn new(
+        identifier: FontIdentifier,
+        maybe_bytes: Option<Vec<u8>>,
+    ) -> Result<FontTemplate, IoError> {
         let maybe_data = match maybe_bytes {
             Some(_) => Some(FontTemplateData::new(identifier.clone(), maybe_bytes)?),
             None => None,
@@ -122,7 +125,7 @@ impl FontTemplate {
         })
     }
 
-    pub fn identifier(&self) -> &Atom {
+    pub fn identifier(&self) -> &FontIdentifier {
         &self.identifier
     }
 

components/gfx/platform/freetype/android/font_list.rs

@@ -5,6 +5,8 @@
 use std::path::Path;
 
 use log::warn;
+use serde::{Deserialize, Serialize};
+use style::Atom;
 use ucd::{Codepoint, UnicodeBlock};
 
 use super::xml::{Attribute, Node};
@@ -14,6 +16,13 @@ lazy_static::lazy_static! {
     static ref FONT_LIST: FontList = FontList::new();
 }
 
+/// An identifier for a local font on Android systems.
+#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
+pub struct LocalFontIdentifier {
+    /// The path to the font.
+    pub path: Atom,
+}
+
 // Android doesn't provide an API to query system fonts until Android O:
 // https://developer.android.com/reference/android/text/FontConfig.html
 // System font configuration files must be parsed until Android O version is set as the minimum target.
@@ -426,11 +435,17 @@ where
 
 pub fn for_each_variation<F>(family_name: &str, mut callback: F)
 where
-    F: FnMut(String),
+    F: FnMut(LocalFontIdentifier),
 {
+    let mut produce_font = |font: &Font| {
+        callback(LocalFontIdentifier {
+            path: Atom::from(FontList::font_absolute_path(&font.filename)),
+        })
+    };
+
     if let Some(family) = FONT_LIST.find_family(family_name) {
         for font in &family.fonts {
-            callback(FontList::font_absolute_path(&font.filename));
+            produce_font(font);
         }
         return;
     }
@@ -439,12 +454,8 @@ where
         if let Some(family) = FONT_LIST.find_family(&alias.to) {
             for font in &family.fonts {
                 match (alias.weight, font.weight) {
-                    (None, _) => callback(FontList::font_absolute_path(&font.filename)),
+                    (None, _) => produce_font(font),
-                    (Some(w1), Some(w2)) => {
+                    (Some(w1), Some(w2)) if w1 == w2 => produce_font(font),
-                        if w1 == w2 {
-                            callback(FontList::font_absolute_path(&font.filename))
-                        }
-                    },
                     _ => {},
                 }
             }

components/gfx/platform/freetype/font.rs

@@ -17,7 +17,6 @@ use freetype::freetype::{
 use freetype::succeeded;
 use freetype::tt_os2::TT_OS2;
 use log::debug;
-use servo_atoms::Atom;
 use style::computed_values::font_stretch::T as FontStretch;
 use style::computed_values::font_weight::T as FontWeight;
 use style::values::computed::font::FontStyle;
@@ -27,6 +26,7 @@ use crate::font::{
     FontHandleMethods, FontMetrics, FontTableMethods, FontTableTag, FractionalPixel, GPOS, GSUB,
     KERN,
 };
+use crate::font_cache_thread::FontIdentifier;
 use crate::platform::font_context::FontContextHandle;
 use crate::platform::font_template::FontTemplateData;
 use crate::text::glyph::GlyphId;
@@ -102,7 +102,9 @@ fn create_face(
         let mut face: FT_Face = ptr::null_mut();
         let face_index = 0 as FT_Long;
 
-        let result = if let Some(ref bytes) = template.bytes {
+        let result = match template.identifier {
+            FontIdentifier::Web(_) => {
+                let bytes = template.bytes();
                 FT_New_Memory_Face(
                     lib,
                     bytes.as_ptr(),
@@ -110,15 +112,16 @@ fn create_face(
                     face_index,
                     &mut face,
                 )
-        } else {
+            },
+            FontIdentifier::Local(ref local_identifier) => {
                 // This will trigger a synchronous file read during layout, which we may want to
                 // revisit at some point. See discussion here:
                 //
                 // https://github.com/servo/servo/pull/20506#issuecomment-378838800
-
                 let filename =
-                CString::new(&*template.identifier).expect("filename contains NUL byte!");
+                    CString::new(&*local_identifier.path).expect("filename contains NUL byte!");
                 FT_New_Face(lib, filename.as_ptr(), face_index, &mut face)
+            },
         };
 
         if !succeeded(result) || face.is_null() {
@@ -361,8 +364,8 @@ impl FontHandleMethods for FontHandle {
         }
     }
 
-    fn identifier(&self) -> Atom {
+    fn identifier(&self) -> &FontIdentifier {
-        self.font_data.identifier.clone()
+        &self.font_data.identifier
     }
 }
 

components/gfx/platform/freetype/font_list.rs

@@ -13,6 +13,8 @@ use fontconfig_sys::{
 };
 use libc::{c_char, c_int};
 use log::debug;
+use serde::{Deserialize, Serialize};
+use style::Atom;
 
 use super::c_str_to_string;
 use crate::text::util::is_cjk;
@@ -22,6 +24,15 @@ static FC_FILE: &[u8] = b"file\0";
 static FC_INDEX: &[u8] = b"index\0";
 static FC_FONTFORMAT: &[u8] = b"fontformat\0";
 
+/// An identifier for a local font on systems using Freetype.
+#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
+pub struct LocalFontIdentifier {
+    /// The path to the font.
+    pub path: Atom,
+    /// The variation index within the font.
+    pub variation_index: i32,
+}
+
 pub fn for_each_available_family<F>(mut callback: F)
 where
     F: FnMut(String),
@@ -59,7 +70,7 @@ where
 
 pub fn for_each_variation<F>(family_name: &str, mut callback: F)
 where
-    F: FnMut(String),
+    F: FnMut(LocalFontIdentifier),
 {
     debug!("getting variations for {}", family_name);
     unsafe {
@@ -84,31 +95,24 @@ where
         FcObjectSetAdd(object_set, FC_INDEX.as_ptr() as *mut c_char);
 
         let matches = FcFontSetList(config, font_set_array_ptr, 1, pattern, object_set);
-
         debug!("found {} variations", (*matches).nfont);
 
         for i in 0..((*matches).nfont as isize) {
             let font = (*matches).fonts.offset(i);
-            let mut file: *mut FcChar8 = ptr::null_mut();
+
-            let result = FcPatternGetString(*font, FC_FILE.as_ptr() as *mut c_char, 0, &mut file);
+            let mut path: *mut FcChar8 = ptr::null_mut();
-            let file = if result == FcResultMatch {
+            let result = FcPatternGetString(*font, FC_FILE.as_ptr() as *mut c_char, 0, &mut path);
-                c_str_to_string(file as *const c_char)
+            assert_eq!(result, FcResultMatch);
-            } else {
+
-                panic!();
-            };
             let mut index: libc::c_int = 0;
             let result =
                 FcPatternGetInteger(*font, FC_INDEX.as_ptr() as *mut c_char, 0, &mut index);
-            let index = if result == FcResultMatch {
+            assert_eq!(result, FcResultMatch);
-                index
-            } else {
-                panic!();
-            };
 
-            debug!("variation file: {}", file);
+            callback(LocalFontIdentifier {
-            debug!("variation index: {}", index);
+                path: Atom::from(c_str_to_string(path as *const c_char)),
-
+                variation_index: index as i32,
-            callback(file);
+            });
         }
 
         FcFontSetDestroy(matches);

components/gfx/platform/freetype/font_template.rs

@@ -5,67 +5,89 @@
 use std::fmt;
 use std::fs::File;
 use std::io::{Error, Read};
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
+use std::sync::{Arc, RwLock};
 
 use serde::{Deserialize, Serialize};
-use servo_atoms::Atom;
 use webrender_api::NativeFontHandle;
 
+use crate::font_cache_thread::FontIdentifier;
+
 /// Platform specific font representation for Linux.
-/// The identifier is an absolute path, and the bytes
-/// field is the loaded data that can be passed to
-/// freetype and Raqote directly.
 #[derive(Deserialize, Serialize)]
 pub struct FontTemplateData {
-    // If you add members here, review the Debug impl below
+    /// Lazily-loaded (for local fonts) byte data that can be passed
-    pub bytes: Option<Vec<u8>>,
+    /// to Freetype or Raqote directly.
-    pub identifier: Atom,
+    pub font_data: RwLock<Option<Arc<Vec<u8>>>>,
+    pub identifier: FontIdentifier,
 }
 
 impl fmt::Debug for FontTemplateData {
     fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
         fmt.debug_struct("FontTemplateData")
-            .field(
-                "bytes",
-                &self.bytes.as_ref().map(|b| format!("[{} bytes]", b.len())),
-            )
             .field("identifier", &self.identifier)
+            .field(
+                "font_data",
+                &self
+                    .font_data
+                    .read()
+                    .unwrap()
+                    .as_ref()
+                    .map(|bytes| format!("[{} bytes]", bytes.len())),
+            )
             .finish()
     }
 }
 
 impl FontTemplateData {
-    pub fn new(identifier: Atom, bytes: Option<Vec<u8>>) -> Result<FontTemplateData, Error> {
+    pub fn new(
-        Ok(FontTemplateData { bytes, identifier })
+        identifier: FontIdentifier,
-    }
+        font_data: Option<Vec<u8>>,
-
+    ) -> Result<FontTemplateData, Error> {
-    /// Returns a clone of the data in this font. This may be a hugely expensive
+        Ok(FontTemplateData {
-    /// operation (depending on the platform) which performs synchronous disk I/O
+            identifier,
-    /// and should never be done lightly.
+            font_data: RwLock::new(font_data.map(Arc::new)),
-    pub fn bytes(&self) -> Vec<u8> {
-        self.bytes_if_in_memory().unwrap_or_else(|| {
-            let mut file = File::open(&*self.identifier).expect("Couldn't open font file!");
-            let mut buffer = vec![];
-            file.read_to_end(&mut buffer).unwrap();
-            buffer
         })
     }
 
-    /// Returns a clone of the bytes in this font if they are in memory. This function never
+    /// Returns a reference to the data in this font. This may be a hugely expensive
-    /// performs disk I/O.
+    /// operation (depending on the platform) which performs synchronous disk I/O
-    pub fn bytes_if_in_memory(&self) -> Option<Vec<u8>> {
+    /// and should never be done lightly.
-        self.bytes.clone()
+    pub fn bytes(&self) -> Arc<Vec<u8>> {
+        self.font_data
+            .write()
+            .unwrap()
+            .get_or_insert_with(|| {
+                let path = match &self.identifier {
+                    FontIdentifier::Local(local) => local.path.clone(),
+                    FontIdentifier::Web(_) => unreachable!("Web fonts should always have data."),
+                };
+                let mut bytes = Vec::new();
+                File::open(Path::new(&*path))
+                    .expect("Couldn't open font file!")
+                    .read_to_end(&mut bytes)
+                    .unwrap();
+                Arc::new(bytes)
+            })
+            .clone()
+    }
+
+    /// Returns a reference to the bytes in this font if they are in memory. This function
+    /// never performs disk I/O.
+    pub fn bytes_if_in_memory(&self) -> Option<Arc<Vec<u8>>> {
+        self.font_data.read().unwrap().as_ref().cloned()
     }
 
     /// Returns the native font that underlies this font template, if applicable.
     pub fn native_font(&self) -> Option<NativeFontHandle> {
-        if self.bytes.is_none() {
+        let local_identifier = match &self.identifier {
+            FontIdentifier::Local(local_identifier) => local_identifier,
+            FontIdentifier::Web(_) => return None,
+        };
+
         Some(NativeFontHandle {
-                path: PathBuf::from(&*self.identifier),
+            path: PathBuf::from(&*local_identifier.path),
             index: 0,
         })
-        } else {
-            None
-        }
     }
 }

components/gfx/platform/macos/font.rs

@@ -19,13 +19,13 @@ use core_text::font_descriptor::{
     kCTFontDefaultOrientation, SymbolicTraitAccessors, TraitAccessors,
 };
 use log::debug;
-use servo_atoms::Atom;
 use style::values::computed::font::{FontStretch, FontStyle, FontWeight};
 
 use crate::font::{
     FontHandleMethods, FontMetrics, FontTableMethods, FontTableTag, FractionalPixel, GPOS, GSUB,
     KERN,
 };
+use crate::font_cache_thread::FontIdentifier;
 use crate::platform::font_template::FontTemplateData;
 use crate::platform::macos::font_context::FontContextHandle;
 use crate::text::glyph::GlyphId;
@@ -325,7 +325,7 @@ impl FontHandleMethods for FontHandle {
         result.and_then(|data| Some(FontTable::wrap(data)))
     }
 
-    fn identifier(&self) -> Atom {
+    fn identifier(&self) -> &FontIdentifier {
-        self.font_data.identifier.clone()
+        &self.font_data.identifier
     }
 }

components/gfx/platform/macos/font_list.rs

@@ -3,10 +3,21 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 use log::debug;
+use serde::{Deserialize, Serialize};
+use style::Atom;
 use ucd::{Codepoint, UnicodeBlock};
 
 use crate::text::util::unicode_plane;
 
+/// An identifier for a local font on a MacOS system. These values comes from the CoreText
+/// CTFontCollection. Note that `path` here is required. We do not load fonts that do not
+/// have paths.
+#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
+pub struct LocalFontIdentifier {
+    pub postscript_name: Atom,
+    pub path: Atom,
+}
+
 pub fn for_each_available_family<F>(mut callback: F)
 where
     F: FnMut(String),
@@ -19,7 +30,7 @@ where
 
 pub fn for_each_variation<F>(family_name: &str, mut callback: F)
 where
-    F: FnMut(String),
+    F: FnMut(LocalFontIdentifier),
 {
     debug!("Looking for faces of family: {}", family_name);
 
@@ -27,7 +38,15 @@ where
     if let Some(family_collection) = family_collection {
         if let Some(family_descriptors) = family_collection.get_descriptors() {
             for family_descriptor in family_descriptors.iter() {
-                callback(family_descriptor.font_name());
+                let path = family_descriptor.font_path();
+                let path = match path.as_ref().and_then(|path| path.to_str()) {
+                    Some(path) => path,
+                    None => continue,
+                };
+                callback(LocalFontIdentifier {
+                    postscript_name: Atom::from(family_descriptor.font_name()),
+                    path: Atom::from(path),
+                })
             }
         }
     }

components/gfx/platform/macos/font_template.rs

@@ -2,34 +2,27 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
-use std::borrow::ToOwned;
+use std::collections::hash_map::Entry;
 use std::collections::HashMap;
 use std::fmt;
-use std::fs::{self, File};
+use std::fs::File;
 use std::io::{Error as IoError, Read};
 use std::ops::Deref;
 use std::path::Path;
 use std::sync::{Arc, Mutex, RwLock};
 
 use app_units::Au;
-use core_foundation::array::CFArray;
-use core_foundation::base::{CFType, TCFType};
-use core_foundation::dictionary::CFDictionary;
-use core_foundation::string::CFString;
 use core_graphics::data_provider::CGDataProvider;
 use core_graphics::font::CGFont;
 use core_text::font::CTFont;
-use core_text::{font_collection, font_descriptor};
 use serde::de::{Error, Visitor};
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
-use servo_atoms::Atom;
-use servo_url::ServoUrl;
 use webrender_api::NativeFontHandle;
 
-/// Platform specific font representation for mac.
+use crate::font_cache_thread::FontIdentifier;
-/// The identifier is a PostScript font name. The
+
-/// CTFont object is cached here for use by the
+/// Platform specific font representation for MacOS. CTFont object is cached here for use
-/// paint functions that create CGFont references.
+/// by the paint functions that create CGFont references.
 #[derive(Deserialize, Serialize)]
 pub struct FontTemplateData {
     // If you add members here, review the Debug impl below
@@ -41,8 +34,7 @@ pub struct FontTemplateData {
     /// the other side, because `CTFont` instances cannot be sent across processes. This is
     /// harmless, however, because it can always be recreated.
     ctfont: CachedCTFont,
-
+    pub identifier: FontIdentifier,
-    pub identifier: Atom,
     pub font_data: RwLock<Option<Arc<Vec<u8>>>>,
 }
 
@@ -68,10 +60,13 @@ unsafe impl Send for FontTemplateData {}
 unsafe impl Sync for FontTemplateData {}
 
 impl FontTemplateData {
-    pub fn new(identifier: Atom, font_data: Option<Vec<u8>>) -> Result<FontTemplateData, IoError> {
+    pub fn new(
+        identifier: FontIdentifier,
+        font_data: Option<Vec<u8>>,
+    ) -> Result<FontTemplateData, IoError> {
         Ok(FontTemplateData {
             ctfont: CachedCTFont(Mutex::new(HashMap::new())),
-            identifier: identifier.to_owned(),
+            identifier,
             font_data: RwLock::new(font_data.map(Arc::new)),
         })
     }
@@ -79,105 +74,53 @@ impl FontTemplateData {
     /// Retrieves the Core Text font instance, instantiating it if necessary.
     pub fn ctfont(&self, pt_size: f64) -> Option<CTFont> {
         let mut ctfonts = self.ctfont.lock().unwrap();
-        let pt_size_key = Au::from_f64_px(pt_size);
+
-        if !ctfonts.contains_key(&pt_size_key) {
+        let entry = ctfonts.entry(Au::from_f64_px(pt_size));
+        match entry {
+            Entry::Occupied(entry) => return Some(entry.get().clone()),
+            Entry::Vacant(_) => {},
+        }
+
         // If you pass a zero font size to one of the Core Text APIs, it'll replace it with
         // 12.0. We don't want that! (Issue #10492.)
         let clamped_pt_size = pt_size.max(0.01);
-            let mut font_data = self.font_data.write().unwrap();
-            let ctfont = match *font_data {
-                Some(ref bytes) => {
-                    let fontprov = CGDataProvider::from_buffer(bytes.clone());
-                    let cgfont_result = CGFont::from_data_provider(fontprov);
-                    match cgfont_result {
-                        Ok(cgfont) => {
-                            Some(core_text::font::new_from_CGFont(&cgfont, clamped_pt_size))
-                        },
-                        Err(_) => None,
-                    }
-                },
-                None => {
-                    // We can't rely on Core Text to load a font for us by postscript
-                    // name here, due to https://github.com/servo/servo/issues/23290.
-                    // The APIs will randomly load the wrong font, forcing us to use
-                    // the roundabout route of creating a Core Graphics font from a
-                    // a set of descriptors and then creating a Core Text font from
-                    // that one.
 
-                    let attributes: CFDictionary<CFString, CFType> =
+        let provider = CGDataProvider::from_buffer(self.bytes());
-                        CFDictionary::from_CFType_pairs(&[(
+        let cgfont = CGFont::from_data_provider(provider).ok()?;
-                            CFString::new("NSFontNameAttribute"),
+        let ctfont = core_text::font::new_from_CGFont(&cgfont, clamped_pt_size);
-                            CFString::new(&*self.identifier).as_CFType(),
-                        )]);
 
-                    let descriptor = font_descriptor::new_from_attributes(&attributes);
+        // Cache the newly created CTFont font.
-                    let descriptors = CFArray::from_CFTypes(&[descriptor]);
+        entry.or_insert(ctfont.clone());
-                    let collection = font_collection::new_from_descriptors(&descriptors);
+
-                    collection.get_descriptors().and_then(|descriptors| {
+        Some(ctfont)
-                        let descriptor = descriptors.get(0).unwrap();
-                        let font_path = Path::new(&descriptor.font_path().unwrap()).to_owned();
-                        fs::read(&font_path).ok().and_then(|bytes| {
-                            let font_bytes = Arc::new(bytes);
-                            let fontprov = CGDataProvider::from_buffer(font_bytes.clone());
-                            CGFont::from_data_provider(fontprov).ok().map(|cgfont| {
-                                *font_data = Some(font_bytes);
-                                core_text::font::new_from_CGFont(&cgfont, clamped_pt_size)
-                            })
-                        })
-                    })
-                },
-            };
-            if let Some(ctfont) = ctfont {
-                ctfonts.insert(pt_size_key, ctfont);
-            }
-        }
-        ctfonts.get(&pt_size_key).map(|ctfont| (*ctfont).clone())
     }
 
-    /// Returns a clone of the data in this font. This may be a hugely expensive
+    /// Returns a reference to the data in this font. This may be a hugely expensive
     /// operation (depending on the platform) which performs synchronous disk I/O
     /// and should never be done lightly.
-    pub fn bytes(&self) -> Vec<u8> {
+    pub fn bytes(&self) -> Arc<Vec<u8>> {
-        if let Some(font_data) = self.bytes_if_in_memory() {
+        self.font_data
-            return font_data;
+            .write()
-        }
+            .unwrap()
-
+            .get_or_insert_with(|| {
-        // This is spooky action at a distance, but getting a CTFont from this template
+                let path = match &self.identifier {
-        // will (in the common case) bring the bytes into memory if they were not there
+                    FontIdentifier::Local(local) => local.path.clone(),
-        // already. This also helps work around intermittent panics like
+                    FontIdentifier::Web(_) => unreachable!("Web fonts should always have data."),
-        // https://github.com/servo/servo/issues/24622 that occur for unclear reasons.
+                };
-        let ctfont = self.ctfont(0.0);
-        if let Some(font_data) = self.bytes_if_in_memory() {
-            return font_data;
-        }
-
-        let path = ServoUrl::parse(
-            &*ctfont
-                .expect("No Core Text font available!")
-                .url()
-                .expect("No URL for Core Text font!")
-                .get_string()
-                .to_string(),
-        )
-        .expect("Couldn't parse Core Text font URL!")
-        .to_file_path()
-        .expect("Core Text font didn't name a path!");
                 let mut bytes = Vec::new();
-        File::open(path)
+                File::open(Path::new(&*path))
                     .expect("Couldn't open font file!")
                     .read_to_end(&mut bytes)
                     .unwrap();
-        bytes
+                Arc::new(bytes)
+            })
+            .clone()
     }
 
-    /// Returns a clone of the bytes in this font if they are in memory. This function never
+    /// Returns a reference to the bytes in this font if they are in memory.
-    /// performs disk I/O.
+    /// This function never performs disk I/O.
-    pub fn bytes_if_in_memory(&self) -> Option<Vec<u8>> {
+    pub fn bytes_if_in_memory(&self) -> Option<Arc<Vec<u8>>> {
-        self.font_data
+        self.font_data.read().unwrap().as_ref().cloned()
-            .read()
-            .unwrap()
-            .as_ref()
-            .map(|bytes| (**bytes).clone())
     }
 
     /// Returns the native font that underlies this font template, if applicable.

components/gfx/platform/windows/font.rs

@@ -13,7 +13,6 @@ use std::sync::Arc;
 use app_units::Au;
 use dwrote::{Font, FontFace, FontFile, FontStretch, FontStyle};
 use log::debug;
-use servo_atoms::Atom;
 use style::computed_values::font_stretch::T as StyleFontStretch;
 use style::computed_values::font_weight::T as StyleFontWeight;
 use style::values::computed::font::FontStyle as StyleFontStyle;
@@ -22,9 +21,9 @@ use style::values::specified::font::FontStretchKeyword;
 use crate::font::{
     FontHandleMethods, FontMetrics, FontTableMethods, FontTableTag, FractionalPixel,
 };
+use crate::font_cache_thread::FontIdentifier;
 use crate::platform::font_template::FontTemplateData;
 use crate::platform::windows::font_context::FontContextHandle;
-use crate::platform::windows::font_list::font_from_atom;
 use crate::text::glyph::GlyphId;
 
 // 1em = 12pt = 16px, assuming 72 points per inch and 96 px per inch
@@ -208,7 +207,6 @@ pub struct FontHandle {
     face: Nondebug<FontFace>,
     info: FontInfo,
     em_size: f32,
-    du_per_em: f32,
     du_to_px: f32,
     scaled_du_to_px: f32,
 }
@@ -236,24 +234,17 @@ impl FontHandleMethods for FontHandle {
         template: Arc<FontTemplateData>,
         pt_size: Option<Au>,
     ) -> Result<Self, ()> {
-        let (info, face) = if let Some(ref raw_font) = template.bytes {
+        let (face, info) = match template.get_font() {
-            let font_file = FontFile::new_from_data(Arc::new(raw_font.clone()));
+            Some(font) => (font.create_font_face(), FontInfo::new_from_font(&font)?),
-            if font_file.is_none() {
+            None => {
-                // failed to load raw font
+                let bytes = template.bytes();
-                return Err(());
+                let font_file = FontFile::new_from_data(bytes).ok_or(())?;
-            }
-
                 let face = font_file
-                .unwrap()
                     .create_face(0, dwrote::DWRITE_FONT_SIMULATIONS_NONE)
                     .map_err(|_| ())?;
                 let info = FontInfo::new_from_face(&face)?;
-            (info, face)
+                (face, info)
-        } else {
+            },
-            let font = font_from_atom(&template.identifier);
-            let face = font.create_font_face();
-            let info = FontInfo::new_from_font(&font)?;
-            (info, face)
         };
 
         let pt_size = pt_size.unwrap_or(au_from_pt(12.));
@@ -268,9 +259,8 @@ impl FontHandleMethods for FontHandle {
         Ok(FontHandle {
             font_data: template.clone(),
             face: Nondebug(face),
-            info: info,
+            info,
-            em_size: em_size,
+            em_size,
-            du_per_em: du_per_em,
             du_to_px: design_units_to_pixels,
             scaled_du_to_px: scaled_design_units_to_pixels,
         })
@@ -366,7 +356,7 @@ impl FontHandleMethods for FontHandle {
             .map(|bytes| FontTable { data: bytes })
     }
 
-    fn identifier(&self) -> Atom {
+    fn identifier(&self) -> &FontIdentifier {
-        self.font_data.identifier.clone()
+        &self.font_data.identifier
     }
 }

components/gfx/platform/windows/font_list.rs

@@ -3,22 +3,18 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 use std::collections::HashMap;
-use std::sync::atomic::{AtomicUsize, Ordering};
+use std::hash::Hash;
-use std::sync::Mutex;
+use std::sync::{Arc, Mutex};
 
-use dwrote::{Font, FontCollection, FontDescriptor};
+use dwrote::{FontCollection, FontDescriptor};
 use lazy_static::lazy_static;
+use serde::{Deserialize, Serialize};
 use servo_atoms::Atom;
 use ucd::{Codepoint, UnicodeBlock};
 
 use crate::text::util::unicode_plane;
 
-lazy_static! {
+pub static SANS_SERIF_FONT_FAMILY: &str = "Arial";
-    static ref FONT_ATOM_COUNTER: AtomicUsize = AtomicUsize::new(1);
-    static ref FONT_ATOM_MAP: Mutex<HashMap<Atom, FontDescriptor>> = Mutex::new(HashMap::new());
-}
-
-pub static SANS_SERIF_FONT_FAMILY: &'static str = "Arial";
 
 pub fn system_default_family(_: &str) -> Option<String> {
     Some("Verdana".to_owned())
@@ -34,6 +30,24 @@ where
     }
 }
 
+/// An identifier for a local font on a Windows system.
+#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
+pub struct LocalFontIdentifier {
+    /// The FontDescriptor of this font.
+    pub font_descriptor: Arc<FontDescriptor>,
+}
+
+impl Eq for LocalFontIdentifier {}
+
+impl Hash for LocalFontIdentifier {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        self.font_descriptor.family_name.hash(state);
+        self.font_descriptor.weight.to_u32().hash(state);
+        self.font_descriptor.stretch.to_u32().hash(state);
+        self.font_descriptor.style.to_u32().hash(state);
+    }
+}
+
 // for_each_variation is supposed to return a string that can be
 // atomized and then uniquely used to return back to this font.
 // Some platforms use the full postscript name (MacOS X), or
@@ -45,35 +59,20 @@ where
 
 pub fn for_each_variation<F>(family_name: &str, mut callback: F)
 where
-    F: FnMut(String),
+    F: FnMut(LocalFontIdentifier),
 {
     let system_fc = FontCollection::system();
     if let Some(family) = system_fc.get_font_family_by_name(family_name) {
         let count = family.get_font_count();
         for i in 0..count {
             let font = family.get_font(i);
-            let index = FONT_ATOM_COUNTER.fetch_add(1, Ordering::Relaxed);
+            callback(LocalFontIdentifier {
-            let index_str = format!("{}", index);
+                font_descriptor: Arc::new(font.to_descriptor()),
-            let atom = Atom::from(index_str.clone());
+            });
-
-            {
-                let descriptor = font.to_descriptor();
-                let mut fonts = FONT_ATOM_MAP.lock().unwrap();
-                fonts.insert(atom, descriptor);
-            }
-
-            callback(index_str);
         }
     }
 }
 
-pub fn font_from_atom(ident: &Atom) -> Font {
-    let fonts = FONT_ATOM_MAP.lock().unwrap();
-    FontCollection::system()
-        .get_font_from_descriptor(fonts.get(ident).unwrap())
-        .unwrap()
-}
-
 // Based on gfxWindowsPlatform::GetCommonFallbackFonts() in Gecko
 pub fn fallback_font_families(codepoint: Option<char>) -> Vec<&'static str> {
     let mut families = vec!["Arial"];

components/gfx/platform/windows/font_template.rs

@@ -2,28 +2,32 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
+use std::sync::{Arc, RwLock};
 use std::{fmt, io};
 
+use dwrote::{Font, FontCollection};
 use serde::{Deserialize, Serialize};
-use servo_atoms::Atom;
 use webrender_api::NativeFontHandle;
 
-use crate::platform::windows::font_list::font_from_atom;
+use crate::font_cache_thread::FontIdentifier;
 
 #[derive(Deserialize, Serialize)]
 pub struct FontTemplateData {
-    // If you add members here, review the Debug impl below
+    /// The identifier for this font.
-    pub bytes: Option<Vec<u8>>,
+    pub identifier: FontIdentifier,
-    pub identifier: Atom,
+    /// The bytes of this font, lazily loaded if this is a local font.
+    pub font_data: RwLock<Option<Arc<Vec<u8>>>>,
 }
 
 impl fmt::Debug for FontTemplateData {
     fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
         fmt.debug_struct("FontTemplateData")
             .field(
-                "bytes",
+                "font_data",
                 &self
-                    .bytes
+                    .font_data
+                    .read()
+                    .unwrap()
                     .as_ref()
                     .map(|bytes| format!("[{} bytes]", bytes.len())),
             )
@@ -34,40 +38,61 @@ impl fmt::Debug for FontTemplateData {
 
 impl FontTemplateData {
     pub fn new(
-        identifier: Atom,
+        identifier: FontIdentifier,
         font_data: Option<Vec<u8>>,
     ) -> Result<FontTemplateData, io::Error> {
         Ok(FontTemplateData {
-            bytes: font_data,
+            identifier,
-            identifier: identifier,
+            font_data: RwLock::new(font_data.map(Arc::new)),
         })
     }
 
-    pub fn bytes(&self) -> Vec<u8> {
+    /// Returns a reference to the data in this font. This may be a hugely expensive
-        if self.bytes.is_some() {
+    /// operation (depending on the platform) which performs synchronous disk I/O
-            self.bytes.as_ref().unwrap().clone()
+    /// and should never be done lightly.
-        } else {
+    pub fn bytes(&self) -> Arc<Vec<u8>> {
-            let font = font_from_atom(&self.identifier);
+        self.font_data
+            .write()
+            .unwrap()
+            .get_or_insert_with(|| {
+                let font_descriptor = match &self.identifier {
+                    FontIdentifier::Local(local_identifier) => {
+                        local_identifier.font_descriptor.clone()
+                    },
+                    FontIdentifier::Web(_) => unreachable!("Created a web font without data."),
+                };
+
+                let font = FontCollection::system()
+                    .get_font_from_descriptor(&font_descriptor)
+                    .unwrap();
                 let face = font.create_font_face();
                 let files = face.get_files();
-            assert!(files.len() > 0);
+                assert!(!files.is_empty());
-
+                Arc::new(files[0].get_font_file_bytes())
-            files[0].get_font_file_bytes()
+            })
-        }
+            .clone()
     }
 
-    pub fn bytes_if_in_memory(&self) -> Option<Vec<u8>> {
+    /// Returns a reference to the bytes in this font if they are in memory.
-        self.bytes.clone()
+    /// This function never performs disk I/O.
+    pub fn bytes_if_in_memory(&self) -> Option<Arc<Vec<u8>>> {
+        self.font_data.read().unwrap().as_ref().cloned()
+    }
+
+    /// Get a [`FontFace`] for this font if it is a local font or return `None` if it's a
+    /// web font.
+    pub fn get_font(&self) -> Option<Font> {
+        let font_descriptor = match &self.identifier {
+            FontIdentifier::Local(local_identifier) => local_identifier.font_descriptor.clone(),
+            FontIdentifier::Web(_) => return None,
+        };
+
+        FontCollection::system().get_font_from_descriptor(&font_descriptor)
     }
 
     pub fn native_font(&self) -> Option<NativeFontHandle> {
-        if self.bytes.is_some() {
+        let face = self.get_font()?.create_font_face();
-            return None;
+        let path = face.get_files().first()?.get_font_file_path()?;
-        }
-        let font = font_from_atom(&self.identifier);
-        let face = font.create_font_face();
-        let files = face.get_files();
-        let path = files.iter().next()?.get_font_file_path()?;
         Some(NativeFontHandle {
             path,
             index: face.get_index(),

components/gfx/tests/font_context.rs

@@ -13,11 +13,12 @@ use app_units::Au;
 use gfx::font::{
     fallback_font_families, FontDescriptor, FontFamilyDescriptor, FontFamilyName, FontSearchScope,
 };
-use gfx::font_cache_thread::{FontTemplateInfo, FontTemplates};
+use gfx::font_cache_thread::{FontIdentifier, FontTemplateInfo, FontTemplates};
 use gfx::font_context::{FontContext, FontContextHandle, FontSource};
 use gfx::font_template::FontTemplateDescriptor;
 use servo_arc::Arc;
 use servo_atoms::Atom;
+use servo_url::ServoUrl;
 use style::properties::longhands::font_variant_caps::computed_value::T as FontVariantCaps;
 use style::properties::style_structs::Font as FontStyleStruct;
 use style::values::computed::font::{
@@ -36,13 +37,13 @@ struct TestFontSource {
 impl TestFontSource {
     fn new() -> TestFontSource {
         let mut csstest_ascii = FontTemplates::default();
-        Self::add_face(&mut csstest_ascii, "csstest-ascii", None);
+        Self::add_face(&mut csstest_ascii, "csstest-ascii");
 
         let mut csstest_basic = FontTemplates::default();
-        Self::add_face(&mut csstest_basic, "csstest-basic-regular", None);
+        Self::add_face(&mut csstest_basic, "csstest-basic-regular");
 
         let mut fallback = FontTemplates::default();
-        Self::add_face(&mut fallback, "csstest-basic-regular", Some("fallback"));
+        Self::add_face(&mut fallback, "csstest-basic-regular");
 
         let mut families = HashMap::new();
         families.insert("CSSTest ASCII".to_owned(), csstest_ascii);
@@ -56,16 +57,25 @@ impl TestFontSource {
         }
     }
 
-    fn add_face(family: &mut FontTemplates, name: &str, identifier: Option<&str>) {
+    fn identifier_for_font_name(name: &str) -> FontIdentifier {
+        let mut path: PathBuf = [env!("CARGO_MANIFEST_DIR"), "tests", "support", "CSSTest"]
+            .iter()
+            .collect();
+        path.push(format!("{}.ttf", name));
+        FontIdentifier::Web(ServoUrl::from_file_path(path).unwrap())
+    }
+
+    fn add_face(family: &mut FontTemplates, name: &str) {
         let mut path: PathBuf = [env!("CARGO_MANIFEST_DIR"), "tests", "support", "CSSTest"]
             .iter()
             .collect();
         path.push(format!("{}.ttf", name));
 
         let file = File::open(path).unwrap();
-        let identifier = Atom::from(identifier.unwrap_or(name));
+        family.add_template(
-
+            Self::identifier_for_font_name(name),
-        family.add_template(identifier, Some(file.bytes().map(|b| b.unwrap()).collect()))
+            Some(file.bytes().map(|b| b.unwrap()).collect()),
+        )
     }
 }
 
@@ -165,7 +175,10 @@ fn test_font_group_find_by_codepoint() {
         .borrow_mut()
         .find_by_codepoint(&mut context, 'a')
         .unwrap();
-    assert_eq!(&*font.borrow().identifier(), "csstest-ascii");
+    assert_eq!(
+        *font.borrow().identifier(),
+        TestFontSource::identifier_for_font_name("csstest-ascii")
+    );
     assert_eq!(
         count.get(),
         1,
@@ -176,7 +189,10 @@ fn test_font_group_find_by_codepoint() {
         .borrow_mut()
         .find_by_codepoint(&mut context, 'a')
         .unwrap();
-    assert_eq!(&*font.borrow().identifier(), "csstest-ascii");
+    assert_eq!(
+        *font.borrow().identifier(),
+        TestFontSource::identifier_for_font_name("csstest-ascii")
+    );
     assert_eq!(
         count.get(),
         1,
@@ -187,7 +203,10 @@ fn test_font_group_find_by_codepoint() {
         .borrow_mut()
         .find_by_codepoint(&mut context, 'á')
         .unwrap();
-    assert_eq!(&*font.borrow().identifier(), "csstest-basic-regular");
+    assert_eq!(
+        *font.borrow().identifier(),
+        TestFontSource::identifier_for_font_name("csstest-basic-regular")
+    );
     assert_eq!(count.get(), 2, "both fonts should now have been loaded");
 }
 
@@ -206,8 +225,8 @@ fn test_font_fallback() {
         .find_by_codepoint(&mut context, 'a')
         .unwrap();
     assert_eq!(
-        &*font.borrow().identifier(),
+        *font.borrow().identifier(),
-        "csstest-ascii",
+        TestFontSource::identifier_for_font_name("csstest-ascii"),
         "a family in the group should be used if there is a matching glyph"
     );
 
@@ -216,8 +235,8 @@ fn test_font_fallback() {
         .find_by_codepoint(&mut context, 'á')
         .unwrap();
     assert_eq!(
-        &*font.borrow().identifier(),
+        *font.borrow().identifier(),
-        "fallback",
+        TestFontSource::identifier_for_font_name("csstest-basic-regular"),
         "a fallback font should be used if there is no matching glyph in the group"
     );
 }

components/gfx/tests/font_template.rs

@@ -10,9 +10,10 @@ fn test_font_template_descriptor() {
     use std::io::prelude::*;
     use std::path::PathBuf;
 
+    use gfx::font_cache_thread::FontIdentifier;
     use gfx::font_context::FontContextHandle;
     use gfx::font_template::{FontTemplate, FontTemplateDescriptor};
-    use servo_atoms::Atom;
+    use servo_url::ServoUrl;
     use style::values::computed::font::{FontStretch, FontStyle, FontWeight};
 
     fn descriptor(filename: &str) -> FontTemplateDescriptor {
@@ -27,10 +28,9 @@ fn test_font_template_descriptor() {
         .collect();
         path.push(format!("{}.ttf", filename));
 
-        let file = File::open(path).unwrap();
+        let file = File::open(path.clone()).unwrap();
-
         let mut template = FontTemplate::new(
-            Atom::from(filename),
+            FontIdentifier::Web(ServoUrl::from_file_path(path).unwrap()),
             Some(file.bytes().map(|b| b.unwrap()).collect()),
         )
         .unwrap();

components/layout/text.rs

@@ -11,12 +11,12 @@ use std::sync::Arc;
 
 use app_units::Au;
 use gfx::font::{self, FontMetrics, FontRef, RunMetrics, ShapingFlags, ShapingOptions};
+use gfx::font_cache_thread::FontIdentifier;
 use gfx::text::glyph::ByteIndex;
 use gfx::text::text_run::TextRun;
 use gfx::text::util::{self, CompressionMode};
 use log::{debug, warn};
 use range::Range;
-use servo_atoms::Atom;
 use style::computed_values::text_rendering::T as TextRendering;
 use style::computed_values::white_space::T as WhiteSpace;
 use style::computed_values::word_break::T as WordBreak;
@@ -665,10 +665,10 @@ impl RunInfo {
     }
 
     fn has_font(&self, font: &Option<FontRef>) -> bool {
-        fn identifier_and_pt_size(font: &Option<FontRef>) -> Option<(Atom, Au)> {
+        fn identifier_and_pt_size(font: &Option<FontRef>) -> Option<(FontIdentifier, Au)> {
             font.as_ref().map(|font| {
                 let font = font.borrow();
-                (font.identifier(), font.descriptor.pt_size)
+                (font.identifier().clone(), font.descriptor.pt_size)
             })
         }