markdown_preview: Fix panic in mermaid diagram renderer (#50176)

Smit Barmase created 1 month ago

The mermaid renderer can fail to render certain diagrams, and we already
have a fallback for that. It's not worth crashing Zed over it.

Wrapping `catch_unwind` alone doesn't work because our panic hooks
terminate the process before unwinding begins (we intentionally
terminate so the crash handler subprocess can generate a minidump), so
there was no way to gracefully handle panics from third-party code until
now (in rare cases where we need it, like in this case).

To handle this gracefully, we added `crashes::recoverable_panic` which
tells the panic hook to stand down on the current thread so the unwind
can proceed and be caught. This should be used sparingly since caught
panics bypass crash reporting.

Release Notes:

- Fixed a crash when rendering mermaid diagrams in markdown preview.

Change summary

Cargo.lock                                       |  2 +
crates/crashes/Cargo.toml                        |  1 
crates/crashes/src/crashes.rs                    | 36 +++++++++++++++++
crates/markdown_preview/Cargo.toml               |  1 
crates/markdown_preview/src/markdown_renderer.rs |  4 +
5 files changed, 42 insertions(+), 2 deletions(-)

Detailed changes

Cargo.lock

@@ -4081,6 +4081,7 @@ dependencies = [
 name = "crashes"
 version = "0.1.0"
 dependencies = [
+ "anyhow",
  "bincode",
  "cfg-if",
  "crash-handler",
@@ -9994,6 +9995,7 @@ dependencies = [
  "anyhow",
  "async-recursion",
  "collections",
+ "crashes",
  "editor",
  "fs",
  "gpui",

crates/crashes/Cargo.toml

@@ -6,6 +6,7 @@ edition.workspace = true
 license = "GPL-3.0-or-later"
 
 [dependencies]
+anyhow.workspace = true
 bincode.workspace = true
 cfg-if.workspace = true
 crash-handler.workspace = true

crates/crashes/src/crashes.rs

@@ -4,6 +4,7 @@ use log::info;
 use minidumper::{Client, LoopAction, MinidumpBinary};
 use release_channel::{RELEASE_CHANNEL, ReleaseChannel};
 use serde::{Deserialize, Serialize};
+use std::cell::Cell;
 use std::mem;
 
 #[cfg(not(target_os = "windows"))]
@@ -15,7 +16,7 @@ use std::{
     env,
     fs::{self, File},
     io,
-    panic::{self, PanicHookInfo},
+    panic::{self, AssertUnwindSafe, PanicHookInfo},
     path::{Path, PathBuf},
     process::{self},
     sync::{
@@ -26,6 +27,31 @@ use std::{
     time::Duration,
 };
 
+thread_local! {
+    static ALLOW_UNWIND: Cell<bool> = const { Cell::new(false) };
+}
+
+/// Catch a panic as an error instead of aborting the process. Unlike plain
+/// `catch_unwind`, this bypasses the crash-reporting panic hook which would
+/// normally abort before unwinding can occur.
+///
+/// **Use sparingly.** Prefer this only for isolating third-party code
+/// that is known to panic, where you want to handle the failure gracefully
+/// instead of crashing.
+pub fn recoverable_panic<T>(closure: impl FnOnce() -> T) -> anyhow::Result<T> {
+    ALLOW_UNWIND.with(|flag| flag.set(true));
+    let result = panic::catch_unwind(AssertUnwindSafe(closure));
+    ALLOW_UNWIND.with(|flag| flag.set(false));
+    result.map_err(|payload| {
+        let message = payload
+            .downcast_ref::<&str>()
+            .map(|s| s.to_string())
+            .or_else(|| payload.downcast_ref::<String>().cloned())
+            .unwrap_or_else(|| "unknown panic".to_string());
+        anyhow::anyhow!("panic: {message}")
+    })
+}
+
 // set once the crash handler has initialized and the client has connected to it
 pub static CRASH_HANDLER: OnceLock<Arc<Client>> = OnceLock::new();
 // set when the first minidump request is made to avoid generating duplicate crash reports
@@ -57,6 +83,9 @@ pub fn init(crash_init: InitCrashHandler, spawn: impl FnOnce(BoxFuture<'static,
     if !should_install_crash_handler() {
         let old_hook = panic::take_hook();
         panic::set_hook(Box::new(move |info| {
+            if ALLOW_UNWIND.with(|flag| flag.get()) {
+                return;
+            }
             unsafe { env::set_var("RUST_BACKTRACE", "1") };
             old_hook(info);
             // prevent the macOS crash dialog from popping up
@@ -322,6 +351,11 @@ pub fn panic_hook(info: &PanicHookInfo) {
     let current_thread = std::thread::current();
     let thread_name = current_thread.name().unwrap_or("<unnamed>");
 
+    if ALLOW_UNWIND.with(|flag| flag.get()) {
+        log::error!("thread '{thread_name}' panicked at {span} (allowing unwind):\n{message}");
+        return;
+    }
+
     // wait 500ms for the crash handler process to start up
     // if it's still not there just write panic info and no minidump
     let retry_frequency = Duration::from_millis(100);

crates/markdown_preview/Cargo.toml

@@ -18,6 +18,7 @@ test-support = []
 anyhow.workspace = true
 async-recursion.workspace = true
 collections.workspace = true
+crashes.workspace = true
 editor.workspace = true
 fs.workspace = true
 gpui.workspace = true

crates/markdown_preview/src/markdown_renderer.rs

@@ -133,7 +133,9 @@ impl CachedMermaidDiagram {
         let _task = cx.spawn(async move |this, cx| {
             let value = cx
                 .background_spawn(async move {
-                    let svg_string = mermaid_rs_renderer::render(&contents.contents)?;
+                    let svg_string = crashes::recoverable_panic(|| {
+                        mermaid_rs_renderer::render(&contents.contents)
+                    })??;
                     let scale = contents.scale as f32 / 100.0;
                     svg_renderer
                         .render_single_frame(svg_string.as_bytes(), scale, true)