Sandbox node graph execution on native targets and attempt recovery from panics on Wasm (#1846)

* Test out wasm unwinding

* Implement panic catching for native targets

* Hack in support for recovering panics in wasm

* Keep debug info in release builds

* Check for DynAnyNode in Backtrace because that can't be inlined as well

* Improve error dialog

* Use a mutex for storing the frontend state instead of a RefCell

* Code review

* Update crash text

---------

Co-authored-by: Keavon Chambers <keavon@keavon.com>

Cargo.lock

@@ -2449,6 +2449,7 @@ dependencies = [
  "serde",
  "serde_json",
  "specta",
+ "spin",
  "thiserror",
  "tokio",
  "usvg",
@@ -5775,6 +5776,9 @@ name = "spin"
 version = "0.9.8"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67"
+dependencies = [
+ "lock_api",
+]
 
 [[package]]
 name = "spirv"

editor/Cargo.toml

@@ -61,6 +61,10 @@ web-sys = { workspace = true, features = [
 	"TextMetrics",
 ] }
 
+# Required dependencies
+async-mutex = "1.4.0"
+spin = "0.9.8"
+
 # Optional local dependencies
 wgpu-executor = { path = "../node-graph/wgpu-executor", optional = true }
 gpu-executor = { path = "../node-graph/gpu-executor", optional = true }
@@ -68,7 +72,6 @@ gpu-executor = { path = "../node-graph/gpu-executor", optional = true }
 # Optional workspace dependencies
 wasm-bindgen = { workspace = true, optional = true }
 wasm-bindgen-futures = { workspace = true, optional = true }
-async-mutex = "1.4.0"
 
 [dev-dependencies]
 # Workspace dependencies

editor/src/node_graph_executor.rs

@@ -4,7 +4,6 @@ use crate::messages::portfolio::document::node_graph::document_node_types::wrap_
 use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier;
 use crate::messages::prelude::*;
 
-use futures::lock::Mutex;
 use graph_craft::concrete;
 use graph_craft::document::value::TaggedValue;
 use graph_craft::document::{generate_uuid, DocumentNodeImplementation, NodeId, NodeNetwork};
@@ -26,6 +25,7 @@ use interpreted_executor::dynamic_executor::{DynamicExecutor, ResolvedDocumentNo
 
 use glam::{DAffine2, DVec2, UVec2};
 use once_cell::sync::Lazy;
+use spin::Mutex;
 use std::sync::mpsc::{Receiver, Sender};
 use std::sync::Arc;
 
@@ -120,7 +120,7 @@ impl NodeGraphUpdateSender for InternalNodeGraphUpdateSender {
 	}
 }
 
-pub(crate) static NODE_RUNTIME: Lazy<Mutex<Option<NodeRuntime>>> = Lazy::new(|| Mutex::new(None));
+pub static NODE_RUNTIME: Lazy<Mutex<Option<NodeRuntime>>> = Lazy::new(|| Mutex::new(None));
 
 impl NodeRuntime {
 	pub fn new(receiver: Receiver<NodeRuntimeMessage>, sender: Sender<NodeGraphUpdate>) -> Self {
@@ -377,7 +377,7 @@ impl NodeRuntime {
 }
 
 pub async fn introspect_node(path: &[NodeId]) -> Option<Arc<dyn std::any::Any>> {
-	let runtime = NODE_RUNTIME.lock().await;
+	let runtime = NODE_RUNTIME.lock();
 	if let Some(ref mut runtime) = runtime.as_ref() {
 		return runtime.executor.introspect(path).flatten();
 	}
@@ -385,14 +385,14 @@ pub async fn introspect_node(path: &[NodeId]) -> Option<Arc<dyn std::any::Any>>
 }
 
 pub async fn run_node_graph() {
-	let mut runtime = NODE_RUNTIME.lock().await;
+	let Some(mut runtime) = NODE_RUNTIME.try_lock() else { return };
 	if let Some(ref mut runtime) = runtime.as_mut() {
 		runtime.run().await;
 	}
 }
 
 pub async fn replace_node_runtime(runtime: NodeRuntime) -> Option<NodeRuntime> {
-	let mut node_runtime = NODE_RUNTIME.lock().await;
+	let mut node_runtime = NODE_RUNTIME.lock();
 	node_runtime.replace(runtime)
 }
 

frontend/wasm/.cargo/config.toml

@@ -12,4 +12,4 @@ rustflags = [
 ]
 
 [unstable]
-build-std = ["panic_abort", "std"]
+build-std = ["std"]

frontend/wasm/Cargo.toml

@@ -52,6 +52,9 @@ web-sys = { workspace = true, features = [
 # Optional workspace dependencies
 ron = { workspace = true, optional = true }
 
+[profile.release]
+debug = true
+
 [package.metadata.wasm-pack.profile.dev]
 wasm-opt = false
 
@@ -61,7 +64,7 @@ demangle-name-section = true
 dwarf-debug-info = true
 
 [package.metadata.wasm-pack.profile.release]
-wasm-opt = ["-Os"]
+wasm-opt = ["-Os", "-g"]
 
 [package.metadata.wasm-pack.profile.release.wasm-bindgen]
 debug-js-glue = false

frontend/wasm/src/editor_api.rs

@@ -71,10 +71,10 @@ impl EditorHandle {
 	pub fn new(frontend_message_handler_callback: js_sys::Function) -> Self {
 		let editor = Editor::new();
 		let editor_handle = EditorHandle { frontend_message_handler_callback };
-		if EDITOR.with(|editor_cell| editor_cell.set(RefCell::new(editor))).is_err() {
+		if EDITOR.with(|handle| handle.lock().ok().map(|mut guard| *guard = Some(editor))).is_none() {
 			log::error!("Attempted to initialize the editor more than once");
 		}
-		if EDITOR_HANDLE.with(|handle_cell| handle_cell.set(RefCell::new(editor_handle.clone()))).is_err() {
+		if EDITOR_HANDLE.with(|handle| handle.lock().ok().map(|mut guard| *guard = Some(editor_handle.clone()))).is_none() {
 			log::error!("Attempted to initialize the editor handle more than once");
 		}
 		editor_handle
@@ -143,18 +143,20 @@ impl EditorHandle {
 			*g.borrow_mut() = Some(Closure::new(move |timestamp| {
 				wasm_bindgen_futures::spawn_local(poll_node_graph_evaluation());
 
-				editor_and_handle(|editor, handle| {
-					let micros: f64 = timestamp * 1000.;
-					let timestamp = Duration::from_micros(micros.round() as u64);
+				if !EDITOR_HAS_CRASHED.load(Ordering::SeqCst) {
+					editor_and_handle(|editor, handle| {
+						let micros: f64 = timestamp * 1000.;
+						let timestamp = Duration::from_micros(micros.round() as u64);
 
-					for message in editor.handle_message(InputPreprocessorMessage::FrameTimeAdvance { timestamp }) {
-						handle.send_frontend_message_to_js(message);
-					}
+						for message in editor.handle_message(InputPreprocessorMessage::FrameTimeAdvance { timestamp }) {
+							handle.send_frontend_message_to_js(message);
+						}
 
-					for message in editor.handle_message(BroadcastMessage::TriggerEvent(BroadcastEvent::AnimationFrame)) {
-						handle.send_frontend_message_to_js(message);
-					}
-				});
+						for message in editor.handle_message(BroadcastMessage::TriggerEvent(BroadcastEvent::AnimationFrame)) {
+							handle.send_frontend_message_to_js(message);
+						}
+					});
+				}
 
 				// Schedule ourself for another requestAnimationFrame callback
 				request_animation_frame(f.borrow().as_ref().unwrap());
@@ -893,29 +895,27 @@ fn set_timeout(f: &Closure<dyn FnMut()>, delay: Duration) {
 }
 
 /// Provides access to the `Editor` by calling the given closure with it as an argument.
-fn editor<T: Default>(callback: impl FnOnce(&mut editor::application::Editor) -> T) -> T {
+fn editor<T>(callback: impl FnOnce(&mut editor::application::Editor) -> T) -> T {
 	EDITOR.with(|editor| {
-		let Some(Ok(mut editor)) = editor.get().map(RefCell::try_borrow_mut) else {
-			// TODO: Investigate if this should just panic instead, and if not doing so right now may be the cause of silent crashes that don't inform the user that the app has panicked
-			log::error!("Failed to borrow the editor");
-			return T::default();
-		};
+		let mut guard = editor.lock();
+		let Ok(Some(ref mut editor)) = guard.as_deref_mut() else { panic!("Failed to borrow the editor") };
 
-		callback(&mut editor)
+		callback(editor)
 	})
 }
 
 /// Provides access to the `Editor` and its `EditorHandle` by calling the given closure with them as arguments.
-fn editor_and_handle(mut callback: impl FnMut(&mut Editor, &mut EditorHandle)) {
-	editor(|editor| {
-		EDITOR_HANDLE.with(|editor_handle| {
-			let Some(Ok(mut handle)) = editor_handle.get().map(RefCell::try_borrow_mut) else {
+pub(crate) fn editor_and_handle(mut callback: impl FnMut(&mut Editor, &mut EditorHandle)) {
+	EDITOR_HANDLE.with(|editor_handle| {
+		editor(|editor| {
+			let mut guard = editor_handle.lock();
+			let Ok(Some(ref mut editor_handle)) = guard.as_deref_mut() else {
 				log::error!("Failed to borrow editor handle");
 				return;
 			};
 
 			// Call the closure with the editor and its handle
-			callback(editor, &mut handle);
+			callback(editor, editor_handle);
 		})
 	});
 }
@@ -937,13 +937,18 @@ async fn poll_node_graph_evaluation() {
 			}
 		}
 
+		// Clear the error display if there are no more errors
+		if !messages.is_empty() {
+			crate::NODE_GRAPH_ERROR_DISPLAYED.store(false, Ordering::SeqCst);
+		}
+
 		// Send each `FrontendMessage` to the JavaScript frontend
 		for response in messages.into_iter().flat_map(|message| editor.handle_message(message)) {
 			handle.send_frontend_message_to_js(response);
 		}
 
 		// If the editor cannot be borrowed then it has encountered a panic - we should just ignore new dispatches
-	})
+	});
 }
 
 fn auto_save_all_documents() {

frontend/wasm/src/lib.rs

@@ -9,17 +9,19 @@ pub mod helpers;
 
 use editor::messages::prelude::*;
 
-use std::cell::{OnceCell, RefCell};
 use std::panic;
 use std::sync::atomic::{AtomicBool, Ordering};
+use std::sync::Mutex;
 use wasm_bindgen::prelude::*;
 
 // Set up the persistent editor backend state
 pub static EDITOR_HAS_CRASHED: AtomicBool = AtomicBool::new(false);
+pub static NODE_GRAPH_ERROR_DISPLAYED: AtomicBool = AtomicBool::new(false);
 pub static LOGGER: WasmLog = WasmLog;
+
 thread_local! {
-	pub static EDITOR: OnceCell<RefCell<editor::application::Editor>> = const { OnceCell::new() };
-	pub static EDITOR_HANDLE: OnceCell<RefCell<editor_api::EditorHandle>> = const { OnceCell::new() };
+	pub static EDITOR: Mutex<Option<editor::application::Editor>> = const { Mutex::new(None) };
+	pub static EDITOR_HANDLE: Mutex<Option<editor_api::EditorHandle>> = const { Mutex::new(None) };
 }
 
 /// Initialize the backend
@@ -35,16 +37,44 @@ pub fn init_graphite() {
 
 /// When a panic occurs, notify the user and log the error to the JS console before the backend dies
 pub fn panic_hook(info: &panic::PanicInfo) {
-	EDITOR_HAS_CRASHED.store(true, Ordering::SeqCst);
+	let info = info.to_string();
+	let backtrace = Error::new("stack").stack().to_string();
+	if backtrace.contains("DynAnyNode") {
+		log::error!("Node graph evaluation panicked {info}");
 
-	error!("{info}");
+		// When the graph panics, the node runtime lock may not be released properly
+		if editor::node_graph_executor::NODE_RUNTIME.try_lock().is_none() {
+			unsafe { editor::node_graph_executor::NODE_RUNTIME.force_unlock() };
+		}
+
+		if !NODE_GRAPH_ERROR_DISPLAYED.load(Ordering::SeqCst) {
+			NODE_GRAPH_ERROR_DISPLAYED.store(true, Ordering::SeqCst);
+			editor_api::editor_and_handle(|_, handle| {
+				let error = r#"
+				<rect x="50%" y="50%" width="600" height="100" transform="translate(-300 -50)" rx="4" fill="var(--color-error-red)" />
+				<text x="50%" y="50%" dominant-baseline="middle" text-anchor="middle" font-size="18" fill="var(--color-2-mildblack)">
+					<tspan x="50%" dy="-24" font-weight="bold">The document crashed while being rendered in its current state.</tspan>
+					<tspan x="50%" dy="24">The editor is now unstable! Undo your last action to restore the artwork,</tspan>
+					<tspan x="50%" dy="24">then save your document and restart the editor before continuing work.</tspan>
+				/text>"#
+				// It's a mystery why the `/text>` tag above needs to be missing its `<`, but when it exists it prints the `<` character in the text. However this works with it removed.
+				.to_string();
+				handle.send_frontend_message_to_js_rust_proxy(FrontendMessage::UpdateDocumentArtwork { svg: error });
+			});
+		}
+
+		return;
+	} else {
+		EDITOR_HAS_CRASHED.store(true, Ordering::SeqCst);
+	}
+
+	log::error!("{info}");
 
 	EDITOR_HANDLE.with(|editor_handle| {
-		editor_handle.get().map(|handle| {
-			handle
-				.borrow_mut()
-				.send_frontend_message_to_js_rust_proxy(FrontendMessage::DisplayDialogPanic { panic_info: info.to_string() })
-		})
+		let mut guard = editor_handle.lock();
+		if let Ok(Some(ref mut handle)) = guard.as_deref_mut() {
+			handle.send_frontend_message_to_js_rust_proxy(FrontendMessage::DisplayDialogPanic { panic_info: info.to_string() });
+		}
 	});
 }
 
@@ -56,6 +86,9 @@ extern "C" {
 
 	#[wasm_bindgen(constructor)]
 	pub fn new(msg: &str) -> Error;
+
+	#[wasm_bindgen(structural, method, getter)]
+	fn stack(error: &Error) -> String;
 }
 
 /// Logging to the JS console
@@ -69,6 +102,8 @@ extern "C" {
 	fn warn(msg: &str, format: &str);
 	#[wasm_bindgen(js_namespace = console)]
 	fn error(msg: &str, format: &str);
+	#[wasm_bindgen(js_namespace = console)]
+	fn trace(msg: &str, format: &str);
 }
 
 #[derive(Default)]

node-graph/interpreted-executor/src/dynamic_executor.rs

@@ -10,6 +10,7 @@ use graph_craft::Type;
 
 use std::collections::{HashMap, HashSet};
 use std::error::Error;
+use std::panic::UnwindSafe;
 use std::sync::Arc;
 
 /// An executor of a node graph that does not require an online compilation server, and instead uses `Box<dyn ...>`.
@@ -102,9 +103,22 @@ impl DynamicExecutor {
 	}
 }
 
-impl<'a, I: StaticType + 'static + Send + Sync> Executor<I, TaggedValue> for &'a DynamicExecutor {
+impl<'a, I: StaticType + 'static + Send + Sync + std::panic::UnwindSafe> Executor<I, TaggedValue> for &'a DynamicExecutor {
 	fn execute(&self, input: I) -> LocalFuture<Result<TaggedValue, Box<dyn Error>>> {
-		Box::pin(async move { self.tree.eval_tagged_value(self.output, input).await.map_err(|e| e.into()) })
+		Box::pin(async move {
+			use futures::FutureExt;
+
+			let result = self.tree.eval_tagged_value(self.output, input);
+			let wrapped_result = std::panic::AssertUnwindSafe(result).catch_unwind().await;
+
+			match wrapped_result {
+				Ok(result) => result.map_err(|e| e.into()),
+				Err(e) => {
+					Box::leak(e);
+					Err("Node graph execution paniced".into())
+				}
+			}
+		})
 	}
 }
 
@@ -176,7 +190,7 @@ impl BorrowTree {
 	}
 	/// Evaluate the output node of the [`BorrowTree`] and cast it to a tagged value.
 	/// This ensures that no borrowed data can escape the node graph.
-	pub async fn eval_tagged_value<I: StaticType + 'static + Send + Sync>(&self, id: NodeId, input: I) -> Result<TaggedValue, String> {
+	pub async fn eval_tagged_value<I: StaticType + 'static + Send + Sync + UnwindSafe>(&self, id: NodeId, input: I) -> Result<TaggedValue, String> {
 		let node = self.nodes.get(&id).cloned().ok_or("Output node not found in executor")?;
 		let output = node.eval(Box::new(input));
 		TaggedValue::try_from_any(output.await)