Restore terminal state again in panic handler

Our panic handler attempts a blocking read from stdin and only exits
after the user presses Enter.

This is unconventional behavior and might cause surprise but there is a
significant upside: crashes become more visible for terminals that don't
already detect crashes (see ecdc9ce1d (Install a panic handler to avoid
dropping crash stacktraces, 2024-03-24)).

As reported in 4d0aa2b5d (Fix panic handler, 2024-08-28), the panic handler
failed to exit fish if the panic happens on background threads.  It would
only exit the background thread (like autosuggestion/highlight/history-pager
performer) itself. The fix was to abort the whole process.
Aborting has the additional upside of generating a coredump.

However since abort() skips stack unwinding, 4d0aa2b5d makes us no longer
restore the terminal on panic. In particular, if the terminal supports kitty
progressive enhancements, keys like ctrl-p will no longer work in say,
a Bash parent shell.  So it broke 121680147 (Use RAII for restoring term
modes, 2024-03-24).

Fix this while still aborting to create coredumps.  This means we can't use
RAII (for better or worse).  The bad part is that we have to deal with added
complexity; we need to make sure that we set the AT_EXIT handler only after
all its inputs (like TERMINAL_MODE_ON_STARTUP) are initialized to a safe
value, but also before any damage has been done to the terminal. I guess we
can add a bunch of assertions.

Unfortunately, if a background thread panics, I haven't yet figured out how
to tell the main thread to do the blocking read.  So the trick of "Press
Enter to exit", which allows users to attach a debugger doesn't yet work for
panics in background threads.  We can probably figure that out later. Maybe
use pthread_kill(3)?  Of course we still create coredumps, so that's fine.
As a temporary workaround, let's sleep for a bit so the user can at least
see that there is a crash & stacktrace.

One ugly bit here is that unit tests run AT_EXIT twice but it should be
idempotent.

src/bin/fish.rs

@@ -27,9 +27,8 @@ use fish::{
         BUILTIN_ERR_MISSING, BUILTIN_ERR_UNKNOWN, STATUS_CMD_OK, STATUS_CMD_UNKNOWN,
     },
     common::{
-        escape, get_executable_path, restore_term_foreground_process_group_for_exit,
-        save_term_foreground_process_group, scoped_push_replacer, str2wcstring, wcs2string,
-        ScopeGuard, PACKAGE_NAME, PROFILING_ACTIVE, PROGRAM_NAME,
+        escape, get_executable_path, save_term_foreground_process_group, scoped_push_replacer,
+        str2wcstring, wcs2string, PACKAGE_NAME, PROFILING_ACTIVE, PROGRAM_NAME,
     },
     env::{
         environment::{env_init, EnvStack, Environment},
@@ -573,9 +572,7 @@ fn throwing_main() -> i32 {
     features::set_from_string(opts.features.as_utfstr());
     proc_init();
     fish::env::misc_init();
-    let _restore_term_foreground_process_group =
-        ScopeGuard::new((), |()| restore_term_foreground_process_group_for_exit());
-    let _restore_term = reader_init();
+    reader_init(true);
 
     // Construct the root parser!
     let env = Rc::new(EnvStack::globals().create_child(true /* dispatches_var_changes */));

src/bin/fish_key_reader.rs

@@ -132,7 +132,7 @@ fn setup_and_process_keys(continuous_mode: bool, verbose: bool) -> i32 {
     topic_monitor_init();
     threads::init();
     env_init(None, true, false);
-    let _restore_term = reader_init();
+    reader_init(false);
 
     signal_set_handlers(true);
     // We need to set the shell-modes for ICRNL,

src/panic.rs

@@ -1,34 +1,61 @@
-use std::panic::{set_hook, take_hook, UnwindSafe};
+use std::{
+    panic::{set_hook, take_hook, UnwindSafe},
+    sync::atomic::{AtomicBool, Ordering},
+    time::Duration,
+};
 
 use libc::STDIN_FILENO;
+use once_cell::sync::OnceCell;
 
 use crate::{
     common::{read_blocked, PROGRAM_NAME},
     nix::isatty,
-    threads::asan_maybe_exit,
+    threads::{asan_maybe_exit, is_main_thread},
 };
 
+pub static AT_EXIT: OnceCell<Box<dyn Fn() + Send + Sync>> = OnceCell::new();
+
 pub fn panic_handler(main: impl FnOnce() -> i32 + UnwindSafe) -> ! {
     if isatty(STDIN_FILENO) {
         let standard_hook = take_hook();
         set_hook(Box::new(move |panic_info| {
             standard_hook(panic_info);
+            static PANICKING: AtomicBool = AtomicBool::new(false);
+            if PANICKING
+                .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire)
+                .is_err()
+            {
+                return;
+            }
+            if let Some(at_exit) = AT_EXIT.get() {
+                (at_exit)();
+            }
             eprintf!(
-                "%s crashed, please report a bug. Debug PID %d or press Enter to exit\n",
+                "%s crashed, please report a bug.",
                 PROGRAM_NAME.get().unwrap(),
-                unsafe { libc::getpid() }
             );
-            let mut buf = [0_u8; 1];
-            loop {
-                let n = read_blocked(STDIN_FILENO, &mut buf).unwrap_or(0);
-                if n == 0 || matches!(buf[0], b'q' | b'\n' | b'\r') {
-                    eprintf!("\n");
-                    std::process::abort();
+            if is_main_thread() {
+                eprintf!("\n");
+                std::thread::sleep(Duration::from_secs(1));
+            } else {
+                eprintf!(" Debug PID %d or press Enter to exit\n", unsafe {
+                    libc::getpid()
+                });
+                let mut buf = [0_u8; 1];
+                while let Ok(n) = read_blocked(STDIN_FILENO, &mut buf) {
+                    if n == 0 || matches!(buf[0], b'q' | b'\n' | b'\r') {
+                        eprintf!("\n");
+                        break;
+                    }
                 }
             }
+            std::process::abort();
         }));
     }
     let exit_status = main();
+    if let Some(at_exit) = AT_EXIT.get() {
+        (at_exit)();
+    }
     asan_maybe_exit(exit_status);
     std::process::exit(exit_status)
 }

src/reader.rs

@@ -45,11 +45,12 @@ use crate::abbrs::abbrs_match;
 use crate::ast::{self, Ast, Category, Traversal};
 use crate::builtins::shared::STATUS_CMD_OK;
 use crate::color::RgbColor;
+use crate::common::restore_term_foreground_process_group_for_exit;
 use crate::common::{
     escape, escape_string, exit_without_destructors, get_ellipsis_char, get_obfuscation_read_char,
     redirect_tty_output, scoped_push_replacer, scoped_push_replacer_ctx, shell_modes, str2wcstring,
-    wcs2string, write_loop, EscapeFlags, EscapeStringStyle, ScopeGuard, ScopeGuarding,
-    PROGRAM_NAME, UTF8_BOM_WCHAR,
+    wcs2string, write_loop, EscapeFlags, EscapeStringStyle, ScopeGuard, PROGRAM_NAME,
+    UTF8_BOM_WCHAR,
 };
 use crate::complete::{
     complete, complete_load, sort_and_prioritize, CompleteFlags, Completion, CompletionList,
@@ -85,6 +86,7 @@ use crate::nix::isatty;
 use crate::operation_context::{get_bg_context, OperationContext};
 use crate::output::Outputter;
 use crate::pager::{PageRendering, Pager, SelectionMotion};
+use crate::panic::AT_EXIT;
 use crate::parse_constants::SourceRange;
 use crate::parse_constants::{ParseTreeFlags, ParserTestErrorBits};
 use crate::parse_tree::ParsedSource;
@@ -784,11 +786,15 @@ fn read_ni(parser: &Parser, fd: RawFd, io: &IoChain) -> i32 {
 }
 
 /// Initialize the reader.
-pub fn reader_init() -> impl ScopeGuarding<Target = ()> {
+pub fn reader_init(will_restore_foreground_pgroup: bool) {
     // Save the initial terminal mode.
     let mut terminal_mode_on_startup = TERMINAL_MODE_ON_STARTUP.lock().unwrap();
     unsafe { libc::tcgetattr(STDIN_FILENO, &mut *terminal_mode_on_startup) };
 
+    #[cfg(not(test))]
+    assert!(AT_EXIT.get().is_none());
+    AT_EXIT.get_or_init(|| Box::new(move || reader_deinit(will_restore_foreground_pgroup)));
+
     // Set the mode used for program execution, initialized to the current mode.
     let mut tty_modes_for_external_cmds = TTY_MODES_FOR_EXTERNAL_CMDS.lock().unwrap();
     *tty_modes_for_external_cmds = *terminal_mode_on_startup;
@@ -813,10 +819,14 @@ pub fn reader_init() -> impl ScopeGuarding<Target = ()> {
             term_donate(/*quiet=*/ true);
         }
     }
-    ScopeGuard::new((), move |()| {
-        restore_term_mode();
-        crate::input_common::terminal_protocols_disable_ifn();
-    })
+}
+
+pub fn reader_deinit(restore_foreground_pgroup: bool) {
+    restore_term_mode();
+    crate::input_common::terminal_protocols_disable_ifn();
+    if restore_foreground_pgroup {
+        restore_term_foreground_process_group_for_exit();
+    }
 }
 
 /// Restore the term mode if we own the terminal and are interactive (#8705).

src/signal.rs

@@ -1,9 +1,9 @@
 use std::num::NonZeroI32;
 
-use crate::common::{exit_without_destructors, restore_term_foreground_process_group_for_exit};
+use crate::common::exit_without_destructors;
 use crate::event::{enqueue_signal, is_signal_observed};
-use crate::input_common::terminal_protocols_disable_ifn;
 use crate::nix::getpid;
+use crate::panic::AT_EXIT;
 use crate::reader::{reader_handle_sigint, reader_sighup};
 use crate::termsize::TermsizeContainer;
 use crate::topic_monitor::{topic_monitor_principal, Generation, GenerationsList, Topic};
@@ -88,8 +88,9 @@ extern "C" fn fish_signal_handler(
         libc::SIGTERM => {
             // Handle sigterm. The only thing we do is restore the front process ID, then die.
             if !observed {
-                restore_term_foreground_process_group_for_exit();
-                terminal_protocols_disable_ifn();
+                if let Some(at_exit) = AT_EXIT.get() {
+                    (at_exit)();
+                }
                 // Safety: signal() and raise() are async-signal-safe.
                 unsafe {
                     libc::signal(libc::SIGTERM, libc::SIG_DFL);

src/tests/mod.rs

@@ -28,10 +28,10 @@ mod topic_monitor;
 mod wgetopt;
 
 pub mod prelude {
-    use crate::common::ScopeGuarding;
+    use crate::common::{ScopeGuard, ScopeGuarding};
     use crate::env::{env_init, misc_init};
     use crate::parser::{CancelBehavior, Parser};
-    use crate::reader::reader_init;
+    use crate::reader::{reader_deinit, reader_init};
     use crate::signal::signal_reset_handlers;
     pub use crate::tests::env::{PwdEnvironment, TestEnvironment};
     use crate::topic_monitor::topic_monitor_init;
@@ -109,7 +109,10 @@ pub mod prelude {
             // Set PWD from getcwd - fixes #5599
             EnvStack::globals().set_pwd_from_getcwd();
         });
-        reader_init()
+        reader_init(false);
+        ScopeGuard::new((), |()| {
+            reader_deinit(false);
+        })
     }
 
     pub use serial_test::serial;