gpui: Fix `TextLayout::layout` producing invalid text runs (#41224)

Lukas Wirth created

The issues is that the closure supplied to `request_measured_layout`
could be run multiple times with differing `known_dimensions`. This in
turn will mean we truncate the font runs once, inserting a multibyte
char at the end but then in the next iteration use those truncated runs
for possible the original untruncated string which has multibyte
characters overlapping the now truncated run end resulting in a faulty
utf8 boundary index.

Solution to this is simple, truncate a clone of the runs when needed
instead of modifying the original.

Fixes https://github.com/zed-industries/zed/issues/36925
Fixed ZED-2FF
Fixes ZED-2KM
Fixes ZED-2KK
Fixes ZED-1FF
Fixes ZED-255
Fixes ZED-2JD
Fixes ZED-2FX
Fixes ZED-2K2
Fixes ZED-2JX
Fixes ZED-2GE
Fixes ZED-2FC
Fixes ZED-2GD
Fixes ZED-2HY
Fixes ZED-2HR
Fixes ZED-2FN
Fixes ZED-2GT
Fixes ZED-2FK
Fixes ZED-2EY
Fixes ZED-27E
Fixes ZED-272
Fixes ZED-2EM
Fixes ZED-2CC
Fixes ZED-29V
Fixes ZED-25B
Fixes ZED-257
Fixes ZED-24R
Fixes ZED-24Q
Fixes ZED-23Z
Fixes ZED-227

Release Notes:

- Fixed a crash in text shaping when truncating rendered text

Change summary

crates/gpui/src/elements/text.rs            | 10 ++--
crates/gpui/src/styled.rs                   |  1 
crates/gpui/src/text_system.rs              | 50 +++++++++++++---------
crates/gpui/src/text_system/line_wrapper.rs | 37 ++++++++--------
crates/gpui/src/window.rs                   | 11 +---
5 files changed, 57 insertions(+), 52 deletions(-)

Detailed changes

crates/gpui/src/elements/text.rs 🔗

@@ -8,6 +8,7 @@ use crate::{
 use anyhow::Context as _;
 use smallvec::SmallVec;
 use std::{
+    borrow::Cow,
     cell::{Cell, RefCell},
     mem,
     ops::Range,
@@ -334,12 +335,11 @@ impl TextLayout {
             .line_height
             .to_pixels(font_size.into(), window.rem_size());
 
-        let mut runs = if let Some(runs) = runs {
+        let runs = if let Some(runs) = runs {
             runs
         } else {
             vec![text_style.to_run(text.len())]
         };
-
         window.request_measured_layout(Default::default(), {
             let element_state = self.clone();
 
@@ -378,15 +378,15 @@ impl TextLayout {
                 }
 
                 let mut line_wrapper = cx.text_system().line_wrapper(text_style.font(), font_size);
-                let text = if let Some(truncate_width) = truncate_width {
+                let (text, runs) = if let Some(truncate_width) = truncate_width {
                     line_wrapper.truncate_line(
                         text.clone(),
                         truncate_width,
                         &truncation_suffix,
-                        &mut runs,
+                        &runs,
                     )
                 } else {
-                    text.clone()
+                    (text.clone(), Cow::Borrowed(&*runs))
                 };
                 let len = text.len();
 

crates/gpui/src/styled.rs 🔗

@@ -9,7 +9,6 @@ pub use gpui_macros::{
     overflow_style_methods, padding_style_methods, position_style_methods,
     visibility_style_methods,
 };
-
 const ELLIPSIS: SharedString = SharedString::new_static("…");
 
 /// A trait for elements that can be styled.

crates/gpui/src/text_system.rs 🔗

@@ -418,22 +418,21 @@ impl WindowTextSystem {
         let mut font_runs = self.font_runs_pool.lock().pop().unwrap_or_default();
 
         let mut lines = SmallVec::new();
-        let mut line_start = 0;
-        let mut max_wrap_lines = line_clamp.unwrap_or(usize::MAX);
+        let mut max_wrap_lines = line_clamp;
         let mut wrapped_lines = 0;
 
-        let mut process_line = |line_text: SharedString| {
+        let mut process_line = |line_text: SharedString, line_start, line_end| {
             font_runs.clear();
-            let line_end = line_start + line_text.len();
 
             let mut decoration_runs = SmallVec::<[DecorationRun; 32]>::new();
             let mut run_start = line_start;
             while run_start < line_end {
                 let Some(run) = runs.peek_mut() else {
+                    log::warn!("`TextRun`s do not cover the entire to be shaped text");
                     break;
                 };
 
-                let run_len_within_line = cmp::min(line_end, run_start + run.len) - run_start;
+                let run_len_within_line = cmp::min(line_end - run_start, run.len);
 
                 let decoration_changed = if let Some(last_run) = decoration_runs.last_mut()
                     && last_run.color == run.color
@@ -467,11 +466,10 @@ impl WindowTextSystem {
                     });
                 }
 
-                if run_len_within_line == run.len {
+                // Preserve the remainder of the run for the next line
+                run.len -= run_len_within_line;
+                if run.len == 0 {
                     runs.next();
-                } else {
-                    // Preserve the remainder of the run for the next line
-                    run.len -= run_len_within_line;
                 }
                 run_start += run_len_within_line;
             }
@@ -481,7 +479,7 @@ impl WindowTextSystem {
                 font_size,
                 &font_runs,
                 wrap_width,
-                Some(max_wrap_lines - wrapped_lines),
+                max_wrap_lines.map(|max| max.saturating_sub(wrapped_lines)),
             );
             wrapped_lines += layout.wrap_boundaries.len();
 
@@ -492,7 +490,6 @@ impl WindowTextSystem {
             });
 
             // Skip `\n` character.
-            line_start = line_end + 1;
             if let Some(run) = runs.peek_mut() {
                 run.len -= 1;
                 if run.len == 0 {
@@ -502,21 +499,34 @@ impl WindowTextSystem {
         };
 
         let mut split_lines = text.split('\n');
-        let mut processed = false;
 
+        // Special case single lines to prevent allocating a sharedstring
         if let Some(first_line) = split_lines.next()
             && let Some(second_line) = split_lines.next()
         {
-            processed = true;
-            process_line(first_line.to_string().into());
-            process_line(second_line.to_string().into());
+            let mut line_start = 0;
+            process_line(
+                SharedString::new(first_line),
+                line_start,
+                line_start + first_line.len(),
+            );
+            line_start += first_line.len() + '\n'.len_utf8();
+            process_line(
+                SharedString::new(second_line),
+                line_start,
+                line_start + second_line.len(),
+            );
             for line_text in split_lines {
-                process_line(line_text.to_string().into());
+                line_start += line_text.len() + '\n'.len_utf8();
+                process_line(
+                    SharedString::new(line_text),
+                    line_start,
+                    line_start + line_text.len(),
+                );
             }
-        }
-
-        if !processed {
-            process_line(text);
+        } else {
+            let end = text.len();
+            process_line(text, 0, end);
         }
 
         self.font_runs_pool.lock().push(font_runs);

crates/gpui/src/text_system/line_wrapper.rs 🔗

@@ -1,6 +1,6 @@
 use crate::{FontId, FontRun, Pixels, PlatformTextSystem, SharedString, TextRun, px};
 use collections::HashMap;
-use std::{iter, sync::Arc};
+use std::{borrow::Cow, iter, sync::Arc};
 
 /// The GPUI line wrapper, used to wrap lines of text to a given width.
 pub struct LineWrapper {
@@ -129,13 +129,13 @@ impl LineWrapper {
     }
 
     /// Truncate a line of text to the given width with this wrapper's font and font size.
-    pub fn truncate_line(
+    pub fn truncate_line<'a>(
         &mut self,
         line: SharedString,
         truncate_width: Pixels,
         truncation_suffix: &str,
-        runs: &mut Vec<TextRun>,
-    ) -> SharedString {
+        runs: &'a [TextRun],
+    ) -> (SharedString, Cow<'a, [TextRun]>) {
         let mut width = px(0.);
         let mut suffix_width = truncation_suffix
             .chars()
@@ -154,13 +154,14 @@ impl LineWrapper {
             if width.floor() > truncate_width {
                 let result =
                     SharedString::from(format!("{}{}", &line[..truncate_ix], truncation_suffix));
-                update_runs_after_truncation(&result, truncation_suffix, runs);
+                let mut runs = runs.to_vec();
+                update_runs_after_truncation(&result, truncation_suffix, &mut runs);
 
-                return result;
+                return (result, Cow::Owned(runs));
             }
         }
 
-        line
+        (line, Cow::Borrowed(runs))
     }
 
     /// Any character in this list should be treated as a word character,
@@ -493,15 +494,14 @@ mod tests {
         fn perform_test(
             wrapper: &mut LineWrapper,
             text: &'static str,
-            result: &'static str,
+            expected: &'static str,
             ellipsis: &str,
         ) {
             let dummy_run_lens = vec![text.len()];
-            let mut dummy_runs = generate_test_runs(&dummy_run_lens);
-            assert_eq!(
-                wrapper.truncate_line(text.into(), px(220.), ellipsis, &mut dummy_runs),
-                result
-            );
+            let dummy_runs = generate_test_runs(&dummy_run_lens);
+            let (result, dummy_runs) =
+                wrapper.truncate_line(text.into(), px(220.), ellipsis, &dummy_runs);
+            assert_eq!(result, expected);
             assert_eq!(dummy_runs.first().unwrap().len, result.len());
         }
 
@@ -532,16 +532,15 @@ mod tests {
         fn perform_test(
             wrapper: &mut LineWrapper,
             text: &'static str,
-            result: &str,
+            expected: &str,
             run_lens: &[usize],
             result_run_len: &[usize],
             line_width: Pixels,
         ) {
-            let mut dummy_runs = generate_test_runs(run_lens);
-            assert_eq!(
-                wrapper.truncate_line(text.into(), line_width, "…", &mut dummy_runs),
-                result
-            );
+            let dummy_runs = generate_test_runs(run_lens);
+            let (result, dummy_runs) =
+                wrapper.truncate_line(text.into(), line_width, "…", &dummy_runs);
+            assert_eq!(result, expected);
             for (run, result_len) in dummy_runs.iter().zip(result_run_len) {
                 assert_eq!(run.len, *result_len);
             }

crates/gpui/src/window.rs 🔗

@@ -3255,14 +3255,11 @@ impl Window {
     /// returns a `Size`.
     ///
     /// This method should only be called as part of the request_layout or prepaint phase of element drawing.
-    pub fn request_measured_layout<
-        F: FnMut(Size<Option<Pixels>>, Size<AvailableSpace>, &mut Window, &mut App) -> Size<Pixels>
+    pub fn request_measured_layout<F>(&mut self, style: Style, measure: F) -> LayoutId
+    where
+        F: Fn(Size<Option<Pixels>>, Size<AvailableSpace>, &mut Window, &mut App) -> Size<Pixels>
             + 'static,
-    >(
-        &mut self,
-        style: Style,
-        measure: F,
-    ) -> LayoutId {
+    {
         self.invalidator.debug_assert_prepaint();
 
         let rem_size = self.rem_size();