diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index 3edcd350a0..a48de69111 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -213,6 +213,11 @@ impl PdfCraftApp { ocr_web_waiting: None, ocr_verify: ocr_verify::OcrVerify::default(), run_inline: false, + panicking_frames: 0, + frame_panicked: false, + logic_without_ui: false, + #[cfg(test)] + panic_next: None, custom_actions: Vec::new(), wizard: Default::default(), action_run: None, @@ -465,7 +470,28 @@ impl eframe::App for PdfCraftApp { /// Each frame's work before drawing: opens, operating-system and control-channel requests, /// the quit guard, shortcuts, and the background jobs' results. + /// + /// Last-resort guard (AGENTS.md §4, #3914): a panic in a frame is reported and the app, with + /// every open tab, keeps running; see `frame_guard`. fn logic(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) { + self.begin_logic(); + self.guarded_frame(|app| app.logic_unguarded(ctx)); + if self.frame_panicked { + self.close_after_panic(ctx); + } + } + + fn ui(&mut self, ui: &mut egui::Ui, _frame: &mut eframe::Frame) { + self.begin_ui(); + self.guarded_frame(|app| app.ui_unguarded(ui)); + self.end_ui(); + } +} + +impl PdfCraftApp { + fn logic_unguarded(&mut self, ctx: &egui::Context) { + #[cfg(test)] + self.injected_panic(crate::frame_guard::FramePart::Logic); self.ctx = Some(ctx.clone()); #[cfg(not(target_arch = "wasm32"))] self.window_state.repair(ctx); @@ -670,7 +696,9 @@ impl eframe::App for PdfCraftApp { self.sync_titles(ctx); } - fn ui(&mut self, ui: &mut egui::Ui, _frame: &mut eframe::Frame) { + fn ui_unguarded(&mut self, ui: &mut egui::Ui) { + #[cfg(test)] + self.injected_panic(crate::frame_guard::FramePart::Ui); let ctx = ui.ctx().clone(); i18n::set_current(i18n::Lang::from_pref(&self.language)); // A language change switches stock chrome between LTR and RTL before anything is drawn. diff --git a/crates/ui-egui/src/commands.rs b/crates/ui-egui/src/commands.rs index 73b097c706..d455d0a74b 100644 --- a/crates/ui-egui/src/commands.rs +++ b/crates/ui-egui/src/commands.rs @@ -116,6 +116,9 @@ impl PdfCraftApp { match pdfcraft_engine::guard(|| self.execute_unguarded(id)) { Ok(done) => done, Err(m) => { + // A command stopped part way through closing or replacing a tab must not leave + // the next frame pointing past the last one (#3914). + self.keep_active_valid(); self.notify_fmt("That didn't work: an internal error stopped it ({m}). Your documents are unchanged.", &[("m", m.as_str())]); true } diff --git a/crates/ui-egui/src/editing/close.rs b/crates/ui-egui/src/editing/close.rs index 6713a91df6..a6803e9dd4 100644 --- a/crates/ui-egui/src/editing/close.rs +++ b/crates/ui-egui/src/editing/close.rs @@ -228,6 +228,12 @@ impl PdfCraftApp { if shortcut { self.request_quit(ctx); } + self.hold_close_for_unsaved(ctx); + } + + /// The window's close: held back with the "save changes?" question while something is + /// unsaved. Also run after a frame whose logic panicked before `guard_quit` (#3914). + pub(crate) fn hold_close_for_unsaved(&mut self, ctx: &egui::Context) { self.sync_unsaved_flag(); if !ctx.input(|i| i.viewport().close_requested()) { return; diff --git a/crates/ui-egui/src/frame_guard.rs b/crates/ui-egui/src/frame_guard.rs new file mode 100644 index 0000000000..1996515258 --- /dev/null +++ b/crates/ui-egui/src/frame_guard.rs @@ -0,0 +1,254 @@ +//! The last-resort guard around each frame (AGENTS.md §4, #3914). Commands, opens and file use +//! already catch panics; a frame's logic and drawing (dialog buttons, panel actions, canvas +//! gestures, reloads) did not, so a panic anywhere in them ended the app with every open tab and +//! up to a minute of unsaved edits. Now it is reported, the app is put back in a state the next +//! frame can draw, and unsaved documents are written to the recovery folder at once. + +use crate::PdfCraftApp; + +/// Frames in a row that may panic before the guard lets the panic end the app, as it did before. +/// By then every unsaved document has been snapshotted, so the next start offers to recover it. +const MAX_PANICKING_FRAMES: u32 = 3; + +impl PdfCraftApp { + /// Before `logic`. While the window is hidden eframe runs `logic` without `ui`, so a + /// `logic` that ran cleanly with no `ui` after it ends the run of panicking frames here. + pub(crate) fn begin_logic(&mut self) { + if self.logic_without_ui && !self.frame_panicked { + self.panicking_frames = 0; + } + self.frame_panicked = false; + self.logic_without_ui = true; + } + + /// Before `ui`: this frame's `logic` is followed by drawing. + pub(crate) fn begin_ui(&mut self) { + self.logic_without_ui = false; + } + + /// After `ui`: a frame whose logic and drawing both ran cleanly ends the run. + pub(crate) fn end_ui(&mut self) { + if !self.frame_panicked { + self.panicking_frames = 0; + } + } + + /// Run one part of the frame (`logic` or `ui`) under the guard. + pub(crate) fn guarded_frame(&mut self, part: impl FnOnce(&mut PdfCraftApp)) { + if let Err(message) = pdfcraft_engine::guard(|| part(self)) { + self.after_frame_panic(&message); + } + } + + /// After a frame panicked: keep the work, drop what the frame was showing on top (a dialog, + /// the command palette), keep `active` pointing at a tab, and say so. + fn after_frame_panic(&mut self, message: &str) { + log::error!("a frame panicked and was caught: {message}"); + if !self.frame_panicked { + self.frame_panicked = true; + self.panicking_frames = self.panicking_frames.saturating_add(1); + } + // Now, not at the next autosave tick. Guarded too: the session may be what panicked. + if let Err(e) = pdfcraft_engine::guard(|| self.autosave_now()) { + log::error!("snapshotting unsaved documents after a frame panic failed: {e}"); + } + if self.panicking_frames >= MAX_PANICKING_FRAMES { + // Every frame panics: the window can't be used or even closed from the app. End it as + // before this guard; the snapshots above are offered for recovery on the next start. + std::panic::resume_unwind(Box::new(message.to_string())); + } + self.dialog = None; + self.palette_open = false; + self.keep_active_valid(); + self.notify_fmt("That didn't work: an internal error stopped it ({m}).", &[("m", message)]); + if let Some(ctx) = &self.ctx { + ctx.request_repaint(); + } + } + + /// After `logic` panicked, perhaps before its quit guard ran: a close of the window asked for + /// in this frame still asks about unsaved changes, or eframe closes it and what the recovery + /// snapshot doesn't hold (a text being typed) is lost. If even that panics, the close is + /// cancelled; the user can close again. + pub(crate) fn close_after_panic(&mut self, ctx: &egui::Context) { + if ctx.input(|i| i.viewport().close_requested()) && pdfcraft_engine::guard(|| self.hold_close_for_unsaved(ctx)).is_err() { + ctx.send_viewport_cmd(egui::ViewportCommand::CancelClose); + } + } + + /// `active` names an open tab, or nothing: a panic part way through closing or replacing a + /// tab can leave it past the end, and the next frame would index out of bounds unguarded. + pub(crate) fn keep_active_valid(&mut self) { + if self.active.is_some_and(|i| i >= self.views.len()) { + self.active = self.views.len().checked_sub(1); + } + } +} + +/// Tests: the part of a frame that panics on purpose while `panic_next` names it. +#[cfg(test)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum FramePart { + Logic, + Ui, +} + +#[cfg(test)] +impl PdfCraftApp { + pub(crate) fn injected_panic(&mut self, part: FramePart) { + if self.panic_next == Some(part) { + panic!("injected {part:?} panic"); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{Dialog, RecoveryStore}; + use egui_kittest::Harness; + use pdfcraft_engine::{Edit, Session}; + + /// A recovery folder of its own, removed on drop. + struct Store(RecoveryStore); + + impl Store { + fn new(tag: &str) -> Store { + let dir = std::env::temp_dir().join(format!("pdfcraft-frame-guard-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + Store(RecoveryStore::new(dir)) + } + } + + impl Drop for Store { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(self.0.dir()); + } + } + + /// Snapshots written to the store. `RecoveryStore::list` leaves out a running instance's own. + fn snapshots(store: &Store) -> usize { + std::fs::read_dir(store.0.dir()).map(|d| d.flatten().filter(|e| e.path().extension().is_some_and(|x| x == "pdf")).count()).unwrap_or(0) + } + + fn pdf(text: &str) -> Vec { + Session::new().create_from_text("t", text).unwrap().to_vec() + } + + /// Two tabs, the second with an unsaved edit, and the About dialog open. + fn harness(store: &Store) -> Harness<'static, PdfCraftApp> { + let recovery = store.0.clone(); + let mut h = Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_eframe(move |_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.enable_recovery(recovery); + app.open_bytes("a.pdf", None, pdf("First")).unwrap(); + app.open_bytes("b.pdf", None, pdf("Second")).unwrap(); + app + }); + h.run_steps(4); + assert!(h.state_mut().apply_edit(Edit::RotatePages { pages: vec![0], degrees: 90 })); + h.state_mut().dialog = Some(Dialog::About); + h.run_steps(1); + h + } + + fn toast(h: &Harness<'static, PdfCraftApp>) -> String { + h.state().toast.as_ref().map(|t| t.0.clone()).unwrap_or_default() + } + + #[test] + fn a_panic_while_drawing_keeps_the_app_its_tabs_and_their_edits() { + let store = Store::new("ui"); + let mut h = harness(&store); + h.state_mut().panic_next = Some(FramePart::Ui); + h.run_steps(1); + h.state_mut().panic_next = None; + h.run_steps(2); + let app = h.state(); + assert_eq!(app.views.len(), 2, "every tab is still open"); + assert_eq!(app.active, Some(1)); + assert!(app.dialog.is_none(), "the dialog that was showing is dropped"); + assert_eq!(app.panicking_frames, 0, "a frame that draws resets the count"); + assert!(toast(&h).starts_with("That didn't work: an internal error stopped it (injected Ui panic)"), "{}", toast(&h)); + assert_eq!(snapshots(&store), 1, "the edited tab was snapshotted for recovery at once"); + } + + #[test] + fn a_panic_in_a_frames_logic_is_caught_too() { + let store = Store::new("logic"); + let mut h = harness(&store); + h.state_mut().panic_next = Some(FramePart::Logic); + h.run_steps(1); + h.state_mut().panic_next = None; + h.run_steps(2); + assert_eq!(h.state().views.len(), 2); + assert!(toast(&h).contains("injected Logic panic"), "{}", toast(&h)); + assert_eq!(snapshots(&store), 1); + } + + #[test] + fn closing_the_window_in_a_frame_whose_logic_panics_still_asks_about_unsaved_changes() { + // The panic comes before the quit guard runs: without the question eframe closes the + // window, and what isn't in the recovery snapshot (a text being typed) is lost. + let store = Store::new("quit"); + let mut h = harness(&store); + h.state_mut().dialog = None; + h.state_mut().panic_next = Some(FramePart::Logic); + h.input_mut().viewports.entry(egui::ViewportId::ROOT).or_default().events.push(egui::ViewportEvent::Close); + h.run_steps(1); + h.state_mut().panic_next = None; + h.run_steps(1); + assert_eq!(h.state().close_request, Some(crate::CloseRequest::Quit), "the save question is asked"); + } + + #[test] + fn frames_that_keep_panicking_end_the_app_after_snapshotting_its_work() { + let store = Store::new("again"); + let mut h = harness(&store); + h.state_mut().panic_next = Some(FramePart::Ui); + // The guard lets the third panicking frame through, as the app ended before it. + let ended = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| h.run_steps(MAX_PANICKING_FRAMES as usize))); + assert!(ended.is_err(), "an app that can't draw a single frame isn't kept running"); + assert_eq!(snapshots(&store), 1, "its unsaved tab is offered for recovery on the next start"); + } + + #[test] + fn hidden_frames_that_run_cleanly_reset_the_count() { + // eframe runs `logic` without `ui` while the window is hidden: panics separated by + // clean hidden frames are not "in a row" and must not end the app. + let mut app = PdfCraftApp::new(); + for _ in 0..MAX_PANICKING_FRAMES * 2 { + app.begin_logic(); + app.guarded_frame(|_| panic!("hidden logic panic")); + app.begin_logic(); + app.guarded_frame(|_| {}); + } + app.begin_logic(); + assert_eq!(app.panicking_frames, 0, "a clean hidden frame resets the count"); + } + + #[test] + fn hidden_frames_that_keep_panicking_still_end_the_app() { + let mut app = PdfCraftApp::new(); + let ended = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + for _ in 0..MAX_PANICKING_FRAMES { + app.begin_logic(); + app.guarded_frame(|_| panic!("hidden logic panic")); + } + })); + assert!(ended.is_err(), "panics in every hidden frame still reach the limit"); + } + + #[test] + fn active_stays_on_an_open_tab() { + let mut app = PdfCraftApp::new(); + app.active = Some(3); + app.keep_active_valid(); + assert_eq!(app.active, None, "no tabs: nothing is active"); + app.open_bytes("a.pdf", None, pdf("Only")).unwrap(); + app.active = Some(5); + app.keep_active_valid(); + assert_eq!(app.active, Some(0)); + } +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 23625cb97b..d1440afda6 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -80,6 +80,7 @@ use theme::{ThemeKind, ThemePreference}; pub use workflows::ocr_verify::{OcrVerify, Phase}; mod app; +mod frame_guard; mod hooks; mod persist; mod platform; @@ -557,6 +558,16 @@ pub struct PdfCraftApp { pub ocr_verify: ocr_verify::OcrVerify, /// Background jobs (OCR, actions, listing pinned folders) run inline instead (tests). pub run_inline: bool, + /// Frames in a row whose logic or drawing panicked, and whether this frame's did + /// (`frame_guard`, #3914). + panicking_frames: u32, + frame_panicked: bool, + /// The last `logic` ran without a `ui` after it yet (eframe skips `ui` while the window is + /// hidden). + logic_without_ui: bool, + /// Tests: the part of the next frame that panics on purpose. + #[cfg(test)] + panic_next: Option, /// Action Wizard: the user's actions, the dialog state, the running action and (tests) the /// files to use instead of a picker. pub custom_actions: Vec,