From 558a4c123036c3a4b25259929082b14cff098e53 Mon Sep 17 00:00:00 2001 From: skylar taylor-barrick Date: Sat, 17 Oct 2026 10:08:17 -0600 Subject: [PATCH] Read the system clipboard once per right-click, and one thread at a time The canvas asked can_paste_image() on every frame, which opens the OS clipboard and decodes any image on it. With UI tests running in parallel, many threads opened the Windows clipboard at once and the editing test binary died with STATUS_HEAP_CORRUPTION. - Ask whether there's an image to paste when a right-click opens a menu, not on every frame. Pasting still reads the clipboard at paste time. - Route every arboard call through os_clipboard::with, which holds a process-wide lock across open, use and close. - stale_clipboard_plain_text_does_not_paste_image no longer writes the real clipboard; it gets its text from the paste event. - Run CI tests in parallel again (revert --test-threads=1). --- .github/workflows/ci.yml | 4 ++-- crates/ui-egui/src/canvas.rs | 11 ++++++++++- crates/ui-egui/src/content_ui.rs | 16 ++++++---------- crates/ui-egui/src/create_ui.rs | 13 ++++++++----- crates/ui-egui/src/image_pick.rs | 2 +- crates/ui-egui/src/lib.rs | 9 ++++----- crates/ui-egui/src/os_clipboard.rs | 18 ++++++++++++++++++ crates/ui-egui/src/zoom_snap.rs | 2 +- crates/ui-egui/tests/editing.rs | 14 +++++--------- 9 files changed, 55 insertions(+), 34 deletions(-) create mode 100644 crates/ui-egui/src/os_clipboard.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7eca1256b0..388b44f032 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,11 +85,11 @@ jobs: - name: Test if: runner.os != 'Linux' - run: cargo test --workspace -- --test-threads=1 + run: cargo test --workspace - name: Test (with craft-fonts) if: runner.os == 'Linux' - run: cargo test --workspace -- --test-threads=1 + run: cargo test --workspace - name: Layering if: runner.os == 'Linux' diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index cc91fef9e9..2b3366e9c2 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2840,7 +2840,16 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { let editing_content = (app.left_open && app.left == crate::LeftPanel::Tool("edit")) || matches!(app.quick_tool, QuickTool::AddText | QuickTool::EditText); let text_style = app.text_style.clone(); - let can_paste_image = app.can_paste_image(); + // Asking means opening the system clipboard (and decoding any image on it), so it's asked + // once when a right-click opens a menu, not on every frame. + let can_paste_key = egui::Id::new("can-paste-image"); + let can_paste_image = if ui.input(|i| i.pointer.secondary_pressed()) { + let can = app.can_paste_image(); + ui.data_mut(|d| d.insert_temp(can_paste_key, can)); + can + } else { + ui.data(|d| d.get_temp(can_paste_key)).unwrap_or(false) + }; let view = &mut app.views[index]; // Opened without the owner password and something is restricted. let secured = doc.security_summary().is_some_and(|s| !(s.owner || (s.permissions.modify() && s.permissions.assemble()))); diff --git a/crates/ui-egui/src/content_ui.rs b/crates/ui-egui/src/content_ui.rs index 61be108256..d0c3721eb2 100644 --- a/crates/ui-egui/src/content_ui.rs +++ b/crates/ui-egui/src/content_ui.rs @@ -824,7 +824,7 @@ impl crate::PdfCraftApp { #[cfg(not(target_arch = "wasm32"))] if self.system_clipboard { let img = arboard::ImageData { width: w as usize, height: h as usize, bytes: std::borrow::Cow::Borrowed(&rgba) }; - arboard::Clipboard::new().and_then(|mut c| c.set_image(img)).map_err(|e| e.to_string())?; + crate::os_clipboard::with(|c| c.set_image(img)).map_err(|e| e.to_string())?; } #[cfg(target_arch = "wasm32")] if self.system_clipboard { @@ -897,10 +897,8 @@ impl crate::PdfCraftApp { pub fn can_paste_image(&self) -> bool { #[cfg(not(target_arch = "wasm32"))] { - if self.system_clipboard - && let Ok(mut cb) = arboard::Clipboard::new() - { - return cb.get_image().is_ok(); + if self.system_clipboard { + return crate::os_clipboard::with(|cb| cb.get_image().map(|_| ())).is_ok(); } } self.last_copied_image.is_some() || self.last_copied_file.is_some() @@ -920,11 +918,9 @@ impl crate::PdfCraftApp { #[cfg(not(target_arch = "wasm32"))] let (has_system_cb, os_image) = if self.system_clipboard { - if let Ok(mut cb) = arboard::Clipboard::new() { - let img_opt = cb.get_image().ok().map(|img| (img.width, img.height, img.bytes.to_vec())); - (true, img_opt) - } else { - (false, None) + match crate::os_clipboard::with(|cb| Ok(cb.get_image().ok().map(|img| (img.width, img.height, img.bytes.into_owned())))) { + Ok(img_opt) => (true, img_opt), + Err(_) => (false, None), } } else { (false, None) diff --git a/crates/ui-egui/src/create_ui.rs b/crates/ui-egui/src/create_ui.rs index fb8f1b2e27..bf12378206 100644 --- a/crates/ui-egui/src/create_ui.rs +++ b/crates/ui-egui/src/create_ui.rs @@ -59,11 +59,14 @@ pub enum Clip { /// An image on the system clipboard, or else its text. #[cfg(not(target_arch = "wasm32"))] fn read_clipboard() -> Option { - let mut cb = arboard::Clipboard::new().ok()?; - if let Ok(img) = cb.get_image() { - return Some(Clip::Image { width: img.width, height: img.height, rgba: img.bytes.into_owned() }); - } - cb.get_text().ok().filter(|t| !t.trim().is_empty()).map(Clip::Text) + crate::os_clipboard::with(|cb| { + if let Ok(img) = cb.get_image() { + return Ok(Some(Clip::Image { width: img.width, height: img.height, rgba: img.bytes.into_owned() })); + } + Ok(cb.get_text().ok().filter(|t| !t.trim().is_empty()).map(Clip::Text)) + }) + .ok() + .flatten() } pub(crate) fn png_from_rgba(width: usize, height: usize, rgba: &[u8]) -> Result, String> { diff --git a/crates/ui-egui/src/image_pick.rs b/crates/ui-egui/src/image_pick.rs index 9e117395cd..0dc879d09d 100644 --- a/crates/ui-egui/src/image_pick.rs +++ b/crates/ui-egui/src/image_pick.rs @@ -208,7 +208,7 @@ impl PdfCraftApp { } let dir = clipboard_dir().ok_or("there is no folder for the copied file")?; let path = write_clipboard_file(&dir, name, bytes)?; - arboard::Clipboard::new().and_then(|mut c| c.set().file_list(&[path])).map_err(|e| e.to_string()) + crate::os_clipboard::with(|c| c.set().file_list(&[path])).map_err(|e| e.to_string()) } #[cfg(target_arch = "wasm32")] diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index ba8fd6390d..083f65769d 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -96,6 +96,8 @@ mod icon_data; pub mod icons; pub mod last_session; pub mod motion; +#[cfg(not(target_arch = "wasm32"))] +mod os_clipboard; mod page_selection_ui; mod pageboxes; mod palette; @@ -2951,11 +2953,8 @@ impl PdfCraftApp { } if paste { #[cfg(not(target_arch = "wasm32"))] - let system_has_text = if self.system_clipboard { - arboard::Clipboard::new().and_then(|mut cb| cb.get_text()).map(|t| !t.is_empty()).unwrap_or(false) - } else { - false - }; + let system_has_text = + if self.system_clipboard { crate::os_clipboard::with(|cb| cb.get_text()).map(|t| !t.is_empty()).unwrap_or(false) } else { false }; #[cfg(target_arch = "wasm32")] let system_has_text = false; diff --git a/crates/ui-egui/src/os_clipboard.rs b/crates/ui-egui/src/os_clipboard.rs new file mode 100644 index 0000000000..0160dc1fcc --- /dev/null +++ b/crates/ui-egui/src/os_clipboard.rs @@ -0,0 +1,18 @@ +//! The one way to the operating system's clipboard. +//! +//! The system clipboard is a single process-wide resource: on Windows, two threads that +//! open it at once can corrupt the heap. Every access goes through [`with`], which holds a +//! lock across the whole open, use and close, so parallel callers (test threads, workers) +//! take turns instead of racing. + +use std::sync::{Mutex, PoisonError}; + +static LOCK: Mutex<()> = Mutex::new(()); + +/// Opens the system clipboard, runs `f` on it and closes it, with no other thread inside. +pub(crate) fn with(f: impl FnOnce(&mut arboard::Clipboard) -> Result) -> Result { + let _turn = LOCK.lock().unwrap_or_else(PoisonError::into_inner); + // Declared after the guard, so it's closed before the lock is released. + let mut clipboard = arboard::Clipboard::new()?; + f(&mut clipboard) +} diff --git a/crates/ui-egui/src/zoom_snap.rs b/crates/ui-egui/src/zoom_snap.rs index eb2cc86727..caf21d8775 100644 --- a/crates/ui-egui/src/zoom_snap.rs +++ b/crates/ui-egui/src/zoom_snap.rs @@ -141,7 +141,7 @@ impl PdfCraftApp { #[cfg(not(target_arch = "wasm32"))] if self.system_clipboard { let img = arboard::ImageData { width: w as usize, height: h as usize, bytes: std::borrow::Cow::Borrowed(&out.rgba) }; - arboard::Clipboard::new().and_then(|mut c| c.set_image(img)).map_err(|e| e.to_string())?; + crate::os_clipboard::with(|c| c.set_image(img)).map_err(|e| e.to_string())?; } self.last_snapshot = Some((w, h, out.rgba)); Ok((w, h)) diff --git a/crates/ui-egui/tests/editing.rs b/crates/ui-egui/tests/editing.rs index a9c3dca38a..6efab8114a 100644 --- a/crates/ui-egui/tests/editing.rs +++ b/crates/ui-egui/tests/editing.rs @@ -4977,20 +4977,16 @@ fn stale_clipboard_plain_text_does_not_paste_image() { // Put an image into internal cache h.state_mut().last_copied_image = Some((10, 10, vec![0; 400])); h.state_mut().last_copied_file = Some(("cached.png".to_string(), vec![1, 2, 3])); - h.state_mut().system_clipboard = true; - - // Set actual system clipboard to plain text - #[cfg(not(target_arch = "wasm32"))] - if let Ok(mut cb) = arboard::Clipboard::new() { - cb.set_text("just plain text").ok(); - } + // The text arrives in the paste event, as it does from the OS. The real system clipboard is + // shared by every test thread, so this test doesn't write it. + h.state_mut().system_clipboard = false; // A paste event containing plain text clears internal image cache and does not paste image h.event(egui::Event::Paste("just plain text".into())); h.run_steps(2); - // can_paste_image should now be false because the system clipboard has text, not an image! - assert!(!h.state().can_paste_image(), "cannot paste image when system clipboard has text"); + // can_paste_image should now be false because the clipboard has text, not an image! + assert!(!h.state().can_paste_image(), "cannot paste image when the clipboard has text"); // paste_image should fail and return false without pasting assert!(!h.state_mut().paste_image(0, None), "must not paste stale cached image");