Auto merge of #27548 - jdm:canvas-text-panic, r=<try>

bors-servo · web-flow · commit 778dc0150dd5 · 2020-08-07T17:21:20.000-04:00
Canvas text and macOS font fixes These changes work around a source of panics by skipping glyphs that don't exist when drawing text to the canvas. Ideally we would have font fallback and/or tofu characters, but that's a larger project than I wanted to get into right now. Relatedly, while investigating the non-panic errors observed in #27476, I realized that the underlying source of #24622 could be dealt with because we shouldn't actually need to open the CTFont by a file path to get the bytes, since we already have bytes available that we use to create the CTFont. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24622 and fix #27476 and fix the panic observed in #20445. - [x] There are tests for these changes
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/components/canvas/Cargo.toml b/components/canvas/Cargo.toml
@@ -31,6 +31,7 @@ ipc-channel = "0.14"
 log = "0.4"
 lyon_geom = "0.14"
 num-traits = "0.2"
+pathfinder_geometry = "0.5"
 pixels = { path = "../pixels" }
 raqote = { version = "0.8", features = ["text"] }
 servo_arc = { path = "../servo_arc" }
diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs
@@ -523,16 +523,10 @@ impl<'a> CanvasData<'a> {
                         .first(font_context)
                         .expect("couldn't find font");
                     let font = font.borrow_mut();
-                    // Retrieving bytes from font template seems to panic for some core text fonts.
-                    // This check avoids having to obtain bytes from the font template data if they
-                    // are not already in the memory.
-                    if let Some(bytes) = font.handle.template().bytes_if_in_memory() {
-                        Font::from_bytes(Arc::new(bytes), 0)
-                            .ok()
-                            .or_else(|| load_system_font_from_style(Some(style)))
-                    } else {
-                        load_system_font_from_style(Some(style))
-                    }
+                    let template = font.handle.template();
+                    Font::from_bytes(Arc::new(template.bytes()), 0)
+                        .ok()
+                        .or_else(|| load_system_font_from_style(Some(style)))
                 })
             },
         );
diff --git a/components/canvas/raqote_backend.rs b/components/canvas/raqote_backend.rs
@@ -527,11 +527,33 @@ impl GenericDrawTarget for raqote::DrawTarget {
         pattern: &canvas_data::Pattern,
         options: &DrawOptions,
     ) {
-        self.draw_text(
+        let mut start = pathfinder_geometry::vector::vec2f(start.x, start.y);
+        let mut ids = Vec::new();
+        let mut positions = Vec::new();
+        for c in text.chars() {
+            let id = match font.glyph_for_char(c) {
+                Some(id) => id,
+                None => {
+                    warn!("Skipping non-existent glyph {}", c);
+                    continue;
+                },
+            };
+            ids.push(id);
+            positions.push(Point2D::new(start.x(), start.y()));
+            let advance = match font.advance(id) {
+                Ok(advance) => advance,
+                Err(e) => {
+                    warn!("Skipping glyph {} with missing advance: {:?}", c, e);
+                    continue;
+                },
+            };
+            start += advance * point_size / 24. / 96.;
+        }
+        self.draw_glyphs(
             font,
             point_size,
-            text,
-            start,
+            &ids,
+            &positions,
             &pattern.source(),
             options.as_raqote(),
         );
diff --git a/components/gfx/platform/macos/font_template.rs b/components/gfx/platform/macos/font_template.rs
@@ -23,7 +23,7 @@ use std::fs::{self, File};
 use std::io::{Error as IoError, Read};
 use std::ops::Deref;
 use std::path::Path;
-use std::sync::{Arc, Mutex};
+use std::sync::{Arc, Mutex, RwLock};
 use webrender_api::NativeFontHandle;
 
 /// Platform specific font representation for mac.
@@ -43,7 +43,7 @@ pub struct FontTemplateData {
     ctfont: CachedCTFont,
 
     pub identifier: Atom,
-    pub font_data: Option<Arc<Vec<u8>>>,
+    pub font_data: RwLock<Option<Arc<Vec<u8>>>>,
 }
 
 impl fmt::Debug for FontTemplateData {
@@ -55,6 +55,8 @@ impl fmt::Debug for FontTemplateData {
                 "font_data",
                 &self
                     .font_data
+                    .read()
+                    .unwrap()
                     .as_ref()
                     .map(|bytes| format!("[{} bytes]", bytes.len())),
             )
@@ -70,7 +72,7 @@ impl FontTemplateData {
         Ok(FontTemplateData {
             ctfont: CachedCTFont(Mutex::new(HashMap::new())),
             identifier: identifier.to_owned(),
-            font_data: font_data.map(Arc::new),
+            font_data: RwLock::new(font_data.map(Arc::new)),
         })
     }
 
@@ -82,7 +84,8 @@ impl FontTemplateData {
             // 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 ctfont = match self.font_data {
+            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);
@@ -114,8 +117,10 @@ impl FontTemplateData {
                         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 fontprov = CGDataProvider::from_buffer(Arc::new(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)
                             })
                         })
@@ -137,9 +142,17 @@ impl FontTemplateData {
             return font_data;
         }
 
+        // This is spooky action at a distance, but getting a CTFont from this template
+        // will (in the common case) bring the bytes into memory if they were not there
+        // already. This also helps work around intermittent panics like
+        // 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(
-            &*self
-                .ctfont(0.0)
+            &*ctfont
                 .expect("No Core Text font available!")
                 .url()
                 .expect("No URL for Core Text font!")
@@ -161,7 +174,11 @@ impl FontTemplateData {
     /// Returns a clone of 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<Vec<u8>> {
-        self.font_data.as_ref().map(|bytes| (**bytes).clone())
+        self.font_data
+            .read()
+            .unwrap()
+            .as_ref()
+            .map(|bytes| (**bytes).clone())
     }
 
     /// Returns the native font that underlies this font template, if applicable.
