diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c1ab2bec30422a..557dd4bc1b149d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -236,7 +236,7 @@ jobs: ~/.cargo/registry/index ~/.cargo/registry/cache ~/.cargo/git/db - key: 8-cargo-home-${{ matrix.os }}-${{ hashFiles('Cargo.lock') }} + key: 9-cargo-home-${{ matrix.os }}-${{ hashFiles('Cargo.lock') }} # In main branch, always creates fresh cache - name: Cache build output (main) @@ -252,7 +252,7 @@ jobs: !./target/*/*.zip !./target/*/*.tar.gz key: | - 8-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ github.sha }} + 9-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ github.sha }} # Restore cache from the latest 'main' branch build. - name: Cache build output (PR) @@ -268,7 +268,7 @@ jobs: !./target/*/*.tar.gz key: never_saved restore-keys: | - 8-cargo-target-${{ matrix.os }}-${{ matrix.profile }}- + 9-cargo-target-${{ matrix.os }}-${{ matrix.profile }}- # Don't save cache after building PRs or branches other than 'main'. - name: Skip save cache (PR) diff --git a/cli/fmt_errors.rs b/cli/fmt_errors.rs index 3016e0ff4a263d..b29b97ea2cd911 100644 --- a/cli/fmt_errors.rs +++ b/cli/fmt_errors.rs @@ -4,11 +4,8 @@ use crate::colors::cyan; use crate::colors::italic_bold; use crate::colors::red; use crate::colors::yellow; -use deno_core::error::{AnyError, JsError, JsStackFrame}; +use deno_core::error::{JsError, JsStackFrame}; use deno_core::url::Url; -use std::error::Error; -use std::fmt; -use std::ops::Deref; const SOURCE_ABBREV_THRESHOLD: usize = 150; const DATA_URL_ABBREV_THRESHOLD: usize = 150; @@ -181,12 +178,12 @@ fn format_maybe_source_line( format!("\n{}{}\n{}{}", indent, source_line, indent, color_underline) } -fn format_js_error(js_error: &JsError, is_child: bool) -> String { +fn format_js_error_inner(js_error: &JsError, is_child: bool) -> String { let mut s = String::new(); s.push_str(&js_error.exception_message); if let Some(aggregated) = &js_error.aggregated { for aggregated_error in aggregated { - let error_string = format_js_error(aggregated_error, true); + let error_string = format_js_error_inner(aggregated_error, true); for line in error_string.trim_start_matches("Uncaught ").lines() { s.push_str(&format!("\n {}", line)); } @@ -209,7 +206,7 @@ fn format_js_error(js_error: &JsError, is_child: bool) -> String { s.push_str(&format!("\n at {}", format_frame(frame))); } if let Some(cause) = &js_error.cause { - let error_string = format_js_error(cause, true); + let error_string = format_js_error_inner(cause, true); s.push_str(&format!( "\nCaused by: {}", error_string.trim_start_matches("Uncaught ") @@ -218,33 +215,10 @@ fn format_js_error(js_error: &JsError, is_child: bool) -> String { s } -/// Wrapper around deno_core::JsError which provides colorful -/// string representation. -#[derive(Debug)] -pub struct PrettyJsError(JsError); - -impl PrettyJsError { - pub fn create(js_error: JsError) -> AnyError { - let pretty_js_error = Self(js_error); - pretty_js_error.into() - } -} - -impl Deref for PrettyJsError { - type Target = JsError; - fn deref(&self) -> &Self::Target { - &self.0 - } +pub fn format_js_error(js_error: &JsError) -> String { + format_js_error_inner(js_error, false) } -impl fmt::Display for PrettyJsError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", &format_js_error(&self.0, false)) - } -} - -impl Error for PrettyJsError {} - #[cfg(test)] mod tests { use super::*; diff --git a/cli/main.rs b/cli/main.rs index 218bc70f558ca9..6e60e34e9985e9 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -63,7 +63,7 @@ use crate::flags::TypeCheckMode; use crate::flags::UninstallFlags; use crate::flags::UpgradeFlags; use crate::flags::VendorFlags; -use crate::fmt_errors::PrettyJsError; +use crate::fmt_errors::format_js_error; use crate::graph_util::graph_lock_or_exit; use crate::graph_util::graph_valid; use crate::module_loader::CliModuleLoader; @@ -73,6 +73,7 @@ use crate::resolver::JsxResolver; use deno_ast::MediaType; use deno_core::error::generic_error; use deno_core::error::AnyError; +use deno_core::error::JsError; use deno_core::futures::future::FutureExt; use deno_core::futures::future::LocalFutureObj; use deno_core::futures::Future; @@ -102,7 +103,6 @@ use std::io::Write; use std::iter::once; use std::path::PathBuf; use std::pin::Pin; -use std::rc::Rc; use std::sync::Arc; fn create_web_worker_preload_module_callback( @@ -166,8 +166,8 @@ fn create_web_worker_callback(ps: ProcState) -> Arc { module_loader, create_web_worker_cb, preload_module_cb, + format_js_error_fn: Some(Arc::new(format_js_error)), source_map_getter: Some(Box::new(ps.clone())), - js_error_create_fn: Some(Rc::new(PrettyJsError::create)), use_deno_namespace: args.use_deno_namespace, worker_type: args.worker_type, maybe_inspector_server, @@ -257,7 +257,7 @@ pub fn create_main_worker( user_agent: version::get_user_agent(), seed: ps.flags.seed, source_map_getter: Some(Box::new(ps.clone())), - js_error_create_fn: Some(Rc::new(PrettyJsError::create)), + format_js_error_fn: Some(Arc::new(format_js_error)), create_web_worker_cb, web_worker_preload_module_cb, maybe_inspector_server, @@ -1442,10 +1442,14 @@ fn unwrap_or_exit(result: Result) -> T { match result { Ok(value) => value, Err(error) => { + let error_string = match error.downcast_ref::() { + Some(e) => format_js_error(e), + None => format!("{:?}", error), + }; eprintln!( "{}: {}", colors::red_bold("error"), - format!("{:?}", error).trim_start_matches("error: ") + error_string.trim_start_matches("error: ") ); std::process::exit(1); } diff --git a/cli/standalone.rs b/cli/standalone.rs index 9f2aba9bd1a1ed..07aef1a26fbaf6 100644 --- a/cli/standalone.rs +++ b/cli/standalone.rs @@ -3,6 +3,7 @@ use crate::colors; use crate::file_fetcher::get_source_from_data_url; use crate::flags::Flags; +use crate::fmt_errors::format_js_error; use crate::ops; use crate::proc_state::ProcState; use crate::version; @@ -293,7 +294,7 @@ pub async fn run( root_cert_store: Some(root_cert_store), seed: metadata.seed, source_map_getter: None, - js_error_create_fn: None, + format_js_error_fn: Some(Arc::new(format_js_error)), create_web_worker_cb, web_worker_preload_module_cb, maybe_inspector_server: None, diff --git a/cli/tests/integration/compile_tests.rs b/cli/tests/integration/compile_tests.rs index 3d2a17ecca31a1..4e45adc07d41a2 100644 --- a/cli/tests/integration/compile_tests.rs +++ b/cli/tests/integration/compile_tests.rs @@ -105,9 +105,12 @@ fn standalone_error() { assert!(!output.status.success()); assert_eq!(output.stdout, b""); let stderr = String::from_utf8(output.stderr).unwrap(); + let stderr = util::strip_ansi_codes(&stderr).to_string(); // On Windows, we cannot assert the file path (because '\'). // Instead we just check for relevant output. - assert!(stderr.contains("error: Error: boom!\n at boom (file://")); + assert!(stderr.contains("error: Uncaught Error: boom!")); + assert!(stderr.contains("throw new Error(\"boom!\");")); + assert!(stderr.contains("\n at boom (file://")); assert!(stderr.contains("standalone_error.ts:2:11")); assert!(stderr.contains("at foo (file://")); assert!(stderr.contains("standalone_error.ts:5:5")); @@ -148,9 +151,12 @@ fn standalone_error_module_with_imports() { println!("{:#?}", &output); assert_eq!(output.stdout, b"hello\n"); let stderr = String::from_utf8(output.stderr).unwrap(); + let stderr = util::strip_ansi_codes(&stderr).to_string(); // On Windows, we cannot assert the file path (because '\'). // Instead we just check for relevant output. - assert!(stderr.contains("error: Error: boom!\n at file://")); + assert!(stderr.contains("error: Uncaught Error: boom!")); + assert!(stderr.contains("throw new Error(\"boom!\");")); + assert!(stderr.contains("\n at file://")); assert!(stderr.contains("standalone_error_module_with_imports_2.ts:2:7")); } diff --git a/cli/tests/integration/worker_tests.rs b/cli/tests/integration/worker_tests.rs index 57faaa6d384109..63d5ccbd3c81db 100644 --- a/cli/tests/integration/worker_tests.rs +++ b/cli/tests/integration/worker_tests.rs @@ -104,3 +104,9 @@ itest!(worker_terminate_tla_crash { args: "run --quiet --reload workers/terminate_tla_crash.js", output: "workers/terminate_tla_crash.js.out", }); + +itest!(worker_error_event { + args: "run --quiet -A workers/error_event.ts", + output: "workers/error_event.ts.out", + exit_code: 1, +}); diff --git a/cli/tests/testdata/workers/error_event.ts b/cli/tests/testdata/workers/error_event.ts new file mode 100644 index 00000000000000..f3046178ab0982 --- /dev/null +++ b/cli/tests/testdata/workers/error_event.ts @@ -0,0 +1,11 @@ +const worker = new Worker(new URL("error.ts", import.meta.url).href, { + type: "module", +}); +worker.addEventListener("error", (e) => { + console.log({ + "message": e.message, + "filename": e.filename?.slice?.(-100), + "lineno": e.lineno, + "colno": e.colno, + }); +}); diff --git a/cli/tests/testdata/workers/error_event.ts.out b/cli/tests/testdata/workers/error_event.ts.out new file mode 100644 index 00000000000000..9c075e23e44e2c --- /dev/null +++ b/cli/tests/testdata/workers/error_event.ts.out @@ -0,0 +1,13 @@ +error: Uncaught (in worker "") Error: foo + throw new Error("foo"); + ^ + at foo ([WILDCARD]/error.ts:2:9) + at [WILDCARD]/error.ts:5:1 +{ + message: "Uncaught Error: foo", + filename: "[WILDCARD]/error.ts", + lineno: 2, + colno: 9 +} +error: Uncaught (in promise) Error: Unhandled error in child worker. + at [WILDCARD] diff --git a/cli/tools/test.rs b/cli/tools/test.rs index d7817eb1ad815f..49a4b4f0545a23 100644 --- a/cli/tools/test.rs +++ b/cli/tools/test.rs @@ -13,7 +13,7 @@ use crate::file_watcher::ResolutionResult; use crate::flags::Flags; use crate::flags::TestFlags; use crate::flags::TypeCheckMode; -use crate::fmt_errors::PrettyJsError; +use crate::fmt_errors::format_js_error; use crate::fs_util::collect_specifiers; use crate::fs_util::is_supported_test_ext; use crate::fs_util::is_supported_test_path; @@ -555,8 +555,8 @@ fn abbreviate_test_error(js_error: &JsError) -> JsError { js_error } -// This function maps JsError to PrettyJsError and applies some changes -// specifically for test runner purposes: +// This function prettifies `JsError` and applies some changes specifically for +// test runner purposes: // // - filter out stack frames: // - if stack trace consists of mixed user and internal code, the frames @@ -568,7 +568,7 @@ pub fn format_test_error(js_error: &JsError) -> String { .exception_message .trim_start_matches("Uncaught ") .to_string(); - PrettyJsError::create(js_error).to_string() + format_js_error(&js_error) } fn create_reporter( diff --git a/core/error.rs b/core/error.rs index 587581e827c96f..8e6c4348212cea 100644 --- a/core/error.rs +++ b/core/error.rs @@ -172,10 +172,6 @@ pub(crate) struct NativeJsError { } impl JsError { - pub(crate) fn create(js_error: Self) -> Error { - js_error.into() - } - pub fn from_v8_exception( scope: &mut v8::HandleScope, exception: v8::Local, diff --git a/core/runtime.rs b/core/runtime.rs index adc0bf3ee4b7e9..865995f749056c 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -162,7 +162,6 @@ pub(crate) struct JsRuntimeState { /// of the event loop. dyn_module_evaluate_idle_counter: u32, pub(crate) source_map_getter: Option>, - pub(crate) js_error_create_fn: Rc, pub(crate) pending_ops: FuturesUnordered, pub(crate) unrefed_ops: HashSet, pub(crate) have_unpolled_ops: bool, @@ -231,11 +230,6 @@ pub struct RuntimeOptions { /// Source map reference for errors. pub source_map_getter: Option>, - /// Allows a callback to be set whenever a V8 exception is made. This allows - /// the caller to wrap the JsError into an error. By default this callback - /// is set to `JsError::create()`. - pub js_error_create_fn: Option>, - /// Allows to map error type to a string "class" used to represent /// error in JavaScript. pub get_error_class_fn: Option, @@ -294,10 +288,6 @@ impl JsRuntime { let has_startup_snapshot = options.startup_snapshot.is_some(); - let js_error_create_fn = options - .js_error_create_fn - .unwrap_or_else(|| Rc::new(JsError::create)); - // Add builtins extension options .extensions @@ -387,7 +377,6 @@ impl JsRuntime { has_tick_scheduled: false, js_wasm_streaming_cb: None, source_map_getter: options.source_map_getter, - js_error_create_fn, pending_ops: FuturesUnordered::new(), unrefed_ops: HashSet::new(), shared_array_buffer_store: options.shared_array_buffer_store, @@ -1062,14 +1051,13 @@ pub(crate) fn exception_to_err_result<'s, T>( js_error.exception_message.trim_start_matches("Uncaught ") ); } - let js_error = (state_rc.borrow().js_error_create_fn)(js_error); if is_terminating_exception { // Re-enable exception termination. scope.terminate_execution(); } - Err(js_error) + Err(js_error.into()) } // Related to module loading diff --git a/runtime/examples/hello_runtime.rs b/runtime/examples/hello_runtime.rs index b4716076e88010..ace6b9e1945bfa 100644 --- a/runtime/examples/hello_runtime.rs +++ b/runtime/examples/hello_runtime.rs @@ -45,7 +45,7 @@ async fn main() -> Result<(), AnyError> { user_agent: "hello_runtime".to_string(), seed: None, source_map_getter: None, - js_error_create_fn: None, + format_js_error_fn: None, web_worker_preload_module_cb, create_web_worker_cb, maybe_inspector_server: None, diff --git a/runtime/js/11_workers.js b/runtime/js/11_workers.js index 5b8d03e7140e86..87949c1c0edecf 100644 --- a/runtime/js/11_workers.js +++ b/runtime/js/11_workers.js @@ -134,17 +134,16 @@ const event = new ErrorEvent("error", { cancelable: true, message: e.message, - lineno: e.lineNumber ? e.lineNumber + 1 : undefined, - colno: e.columnNumber ? e.columnNumber + 1 : undefined, + lineno: e.lineNumber ? e.lineNumber : undefined, + colno: e.columnNumber ? e.columnNumber : undefined, filename: e.fileName, error: null, }); this.dispatchEvent(event); // Don't bubble error event to window for loader errors (`!e.fileName`). - // TODO(nayeemrmn): Currently these are never bubbled because worker - // error event fields aren't populated correctly and `e.fileName` is - // always empty. + // TODO(nayeemrmn): It's not correct to use `e.fileName` to detect user + // errors. It won't be there for non-awaited async ops for example. if (e.fileName && !event.defaultPrevented) { window.dispatchEvent(event); } diff --git a/runtime/ops/worker_host.rs b/runtime/ops/worker_host.rs index 6c691c5a06d590..23ffcd8b1b0426 100644 --- a/runtime/ops/worker_host.rs +++ b/runtime/ops/worker_host.rs @@ -11,6 +11,7 @@ use crate::web_worker::WebWorkerHandle; use crate::web_worker::WebWorkerType; use crate::web_worker::WorkerControlEvent; use crate::web_worker::WorkerId; +use crate::worker::FormatJsErrorFn; use deno_core::error::AnyError; use deno_core::futures::future::LocalFutureObj; use deno_core::op; @@ -54,6 +55,9 @@ pub type PreloadModuleCb = dyn Fn(WebWorker) -> LocalFutureObj<'static, Result); +#[derive(Clone)] +pub struct FormatJsErrorFnHolder(Option>); + /// A holder for callback that can used to preload some modules into a WebWorker /// before actual worker code is executed. It's a struct instead of a type /// because `GothamState` used in `OpState` overrides @@ -106,6 +110,7 @@ pub type WorkersTable = HashMap; pub fn init( create_web_worker_cb: Arc, preload_module_cb: Arc, + format_js_error_fn: Option>, ) -> Extension { Extension::builder() .state(move |state| { @@ -118,6 +123,9 @@ pub fn init( let preload_module_cb_holder = PreloadModuleCbHolder(preload_module_cb.clone()); state.put::(preload_module_cb_holder); + let format_js_error_fn_holder = + FormatJsErrorFnHolder(format_js_error_fn.clone()); + state.put::(format_js_error_fn_holder); Ok(()) }) @@ -193,6 +201,8 @@ fn op_create_worker( state.put::(create_web_worker_cb.clone()); let preload_module_cb = state.take::(); state.put::(preload_module_cb.clone()); + let format_js_error_fn = state.take::(); + state.put::(format_js_error_fn.clone()); state.put::(worker_id.next().unwrap()); let module_specifier = deno_core::resolve_url(&specifier)?; @@ -238,6 +248,7 @@ fn op_create_worker( module_specifier, maybe_source_code, preload_module_cb.0, + format_js_error_fn.0, ) })?; diff --git a/runtime/web_worker.rs b/runtime/web_worker.rs index ac103addae422a..5aa6cfe1a4f31f 100644 --- a/runtime/web_worker.rs +++ b/runtime/web_worker.rs @@ -5,6 +5,7 @@ use crate::js; use crate::ops; use crate::permissions::Permissions; use crate::tokio_util::run_basic; +use crate::worker::FormatJsErrorFn; use crate::BootstrapOptions; use deno_broadcast_channel::InMemoryBroadcastChannel; use deno_core::error::AnyError; @@ -22,7 +23,6 @@ use deno_core::CancelHandle; use deno_core::CompiledWasmModuleStore; use deno_core::Extension; use deno_core::GetErrorClassFn; -use deno_core::JsErrorCreateFn; use deno_core::JsRuntime; use deno_core::ModuleId; use deno_core::ModuleLoader; @@ -93,7 +93,10 @@ impl Serialize for WorkerControlEvent { | WorkerControlEvent::Error(error) => { let value = match error.downcast_ref::() { Some(js_error) => { - let frame = js_error.frames.first(); + let frame = js_error.frames.iter().find(|f| match &f.file_name { + Some(s) => !s.trim_start_matches('[').starts_with("deno:"), + None => false, + }); json!({ "message": js_error.exception_message, "fileName": frame.map(|f| f.file_name.as_ref()), @@ -324,8 +327,8 @@ pub struct WebWorkerOptions { pub module_loader: Rc, pub create_web_worker_cb: Arc, pub preload_module_cb: Arc, + pub format_js_error_fn: Option>, pub source_map_getter: Option>, - pub js_error_create_fn: Option>, pub use_deno_namespace: bool, pub worker_type: WebWorkerType, pub maybe_inspector_server: Option>, @@ -406,6 +409,7 @@ impl WebWorker { ops::worker_host::init( options.create_web_worker_cb.clone(), options.preload_module_cb.clone(), + options.format_js_error_fn.clone(), ), // Extensions providing Deno.* features ops::fs_events::init().enabled(options.use_deno_namespace), @@ -443,7 +447,6 @@ impl WebWorker { module_loader: Some(options.module_loader.clone()), startup_snapshot: Some(js::deno_isolate_init()), source_map_getter: options.source_map_getter, - js_error_create_fn: options.js_error_create_fn.clone(), get_error_class_fn: options.get_error_class_fn, shared_array_buffer_store: options.shared_array_buffer_store.clone(), compiled_wasm_module_store: options.compiled_wasm_module_store.clone(), @@ -644,7 +647,18 @@ impl WebWorker { } } -fn print_worker_error(error_str: String, name: &str) { +fn print_worker_error( + error: &AnyError, + name: &str, + format_js_error_fn: Option<&FormatJsErrorFn>, +) { + let error_str = match format_js_error_fn { + Some(format_js_error_fn) => match error.downcast_ref::() { + Some(js_error) => format_js_error_fn(js_error), + None => error.to_string(), + }, + None => error.to_string(), + }; eprintln!( "{}: Uncaught (in worker \"{}\") {}", colors::red_bold("error"), @@ -660,6 +674,7 @@ pub fn run_web_worker( specifier: ModuleSpecifier, maybe_source_code: Option, preload_module_cb: Arc, + format_js_error_fn: Option>, ) -> Result<(), AnyError> { let name = worker.name.to_string(); @@ -673,7 +688,7 @@ pub fn run_web_worker( let mut worker = match result { Ok(worker) => worker, Err(e) => { - print_worker_error(e.to_string(), &name); + print_worker_error(&e, &name, format_js_error_fn.as_deref()); internal_handle .post_event(WorkerControlEvent::TerminalError(e)) .expect("Failed to post message to host"); @@ -713,7 +728,7 @@ pub fn run_web_worker( }; if let Err(e) = result { - print_worker_error(e.to_string(), &name); + print_worker_error(&e, &name, format_js_error_fn.as_deref()); internal_handle .post_event(WorkerControlEvent::TerminalError(e)) .expect("Failed to post message to host"); diff --git a/runtime/worker.rs b/runtime/worker.rs index 37047570362f95..732924c0c48215 100644 --- a/runtime/worker.rs +++ b/runtime/worker.rs @@ -7,12 +7,12 @@ use crate::permissions::Permissions; use crate::BootstrapOptions; use deno_broadcast_channel::InMemoryBroadcastChannel; use deno_core::error::AnyError; +use deno_core::error::JsError; use deno_core::futures::Future; use deno_core::located_script_name; use deno_core::CompiledWasmModuleStore; use deno_core::Extension; use deno_core::GetErrorClassFn; -use deno_core::JsErrorCreateFn; use deno_core::JsRuntime; use deno_core::LocalInspectorSession; use deno_core::ModuleId; @@ -32,6 +32,8 @@ use std::sync::Arc; use std::task::Context; use std::task::Poll; +pub type FormatJsErrorFn = dyn Fn(&JsError) -> String + Sync + Send; + /// This worker is created and used by almost all /// subcommands in Deno executable. /// @@ -55,8 +57,8 @@ pub struct WorkerOptions { // Callbacks invoked when creating new instance of WebWorker pub create_web_worker_cb: Arc, pub web_worker_preload_module_cb: Arc, + pub format_js_error_fn: Option>, pub source_map_getter: Option>, - pub js_error_create_fn: Option>, pub maybe_inspector_server: Option>, pub should_break_on_first_statement: bool, pub get_error_class_fn: Option, @@ -131,6 +133,7 @@ impl MainWorker { ops::worker_host::init( options.create_web_worker_cb.clone(), options.web_worker_preload_module_cb.clone(), + options.format_js_error_fn.clone(), ), ops::spawn::init(), ops::fs_events::init(), @@ -159,7 +162,6 @@ impl MainWorker { module_loader: Some(options.module_loader.clone()), startup_snapshot: Some(js::deno_isolate_init()), source_map_getter: options.source_map_getter, - js_error_create_fn: options.js_error_create_fn.clone(), get_error_class_fn: options.get_error_class_fn, shared_array_buffer_store: options.shared_array_buffer_store.clone(), compiled_wasm_module_store: options.compiled_wasm_module_store.clone(), @@ -377,8 +379,8 @@ mod tests { unsafely_ignore_certificate_errors: None, root_cert_store: None, seed: None, + format_js_error_fn: None, source_map_getter: None, - js_error_create_fn: None, web_worker_preload_module_cb: Arc::new(|_| unreachable!()), create_web_worker_cb: Arc::new(|_| unreachable!()), maybe_inspector_server: None,