diff options
| -rw-r--r-- | cli/fmt_errors.rs | 5 | ||||
| -rw-r--r-- | cli/proc_state.rs | 29 | ||||
| -rw-r--r-- | core/error.rs | 140 | ||||
| -rw-r--r-- | core/ops_builtin_v8.rs | 6 | ||||
| -rw-r--r-- | core/runtime.rs | 3 | ||||
| -rw-r--r-- | core/source_map.rs | 116 |
6 files changed, 147 insertions, 152 deletions
diff --git a/cli/fmt_errors.rs b/cli/fmt_errors.rs index d30f661a9..37a58364a 100644 --- a/cli/fmt_errors.rs +++ b/cli/fmt_errors.rs @@ -8,8 +8,6 @@ use deno_core::error::format_file_name; use deno_core::error::JsError; use deno_core::error::JsStackFrame; -const SOURCE_ABBREV_THRESHOLD: usize = 150; - // Keep in sync with `/core/error.js`. pub fn format_location(frame: &JsStackFrame) -> String { let _internal = frame @@ -115,8 +113,7 @@ fn format_maybe_source_line( let source_line = source_line.unwrap(); // sometimes source_line gets set with an empty string, which then outputs // an empty source line when displayed, so need just short circuit here. - // Also short-circuit on error line too long. - if source_line.is_empty() || source_line.len() > SOURCE_ABBREV_THRESHOLD { + if source_line.is_empty() { return "".to_string(); } if source_line.contains("Couldn't format source line: ") { diff --git a/cli/proc_state.rs b/cli/proc_state.rs index 2c454c0ee..9e948643d 100644 --- a/cli/proc_state.rs +++ b/cli/proc_state.rs @@ -694,11 +694,9 @@ impl SourceMapGetter for ProcState { _ => return None, } if let Some((code, maybe_map)) = self.get_emit(&specifier) { - let code = String::from_utf8(code).unwrap(); - source_map_from_code(code).or(maybe_map) + source_map_from_code(&code).or(maybe_map) } else if let Ok(source) = self.load(specifier, None, false) { - let code = String::from_utf8(source.code.to_vec()).unwrap(); - source_map_from_code(code) + source_map_from_code(&source.code) } else { None } @@ -756,21 +754,14 @@ pub fn import_map_from_text( Ok(result.import_map) } -fn source_map_from_code(code: String) -> Option<Vec<u8>> { - let lines: Vec<&str> = code.split('\n').collect(); - if let Some(last_line) = lines.last() { - if last_line - .starts_with("//# sourceMappingURL=data:application/json;base64,") - { - let input = last_line.trim_start_matches( - "//# sourceMappingURL=data:application/json;base64,", - ); - let decoded_map = base64::decode(input) - .expect("Unable to decode source map from emitted file."); - Some(decoded_map) - } else { - None - } +fn source_map_from_code(code: &[u8]) -> Option<Vec<u8>> { + static PREFIX: &[u8] = b"//# sourceMappingURL=data:application/json;base64,"; + let last_line = code.rsplitn(2, |u| u == &b'\n').next().unwrap(); + if last_line.starts_with(PREFIX) { + let input = last_line.split_at(PREFIX.len()).1; + let decoded_map = base64::decode(input) + .expect("Unable to decode source map from emitted file."); + Some(decoded_map) } else { None } diff --git a/core/error.rs b/core/error.rs index c3bf6e861..f193c13e2 100644 --- a/core/error.rs +++ b/core/error.rs @@ -2,10 +2,10 @@ use crate::runtime::JsRuntime; use crate::source_map::apply_source_map; +use crate::source_map::get_source_line; use crate::url::Url; use anyhow::Error; use std::borrow::Cow; -use std::collections::HashMap; use std::collections::HashSet; use std::fmt; use std::fmt::Debug; @@ -192,15 +192,20 @@ impl JsError { let msg = v8::Exception::create_message(scope, exception); let mut exception_message = None; - let state_rc = JsRuntime::state(scope); - let state = state_rc.borrow(); - if let Some(format_exception_cb) = &state.js_format_exception_cb { - let format_exception_cb = format_exception_cb.open(scope); - let this = v8::undefined(scope).into(); - let formatted = format_exception_cb.call(scope, this, &[exception]); - if let Some(formatted) = formatted { - if formatted.is_string() { - exception_message = Some(formatted.to_rust_string_lossy(scope)); + // Nest this state borrow. A mutable borrow can occur when accessing `stack` + // in this outer scope, invoking `Error.prepareStackTrace()` which calls + // `op_apply_source_map`. + { + let state_rc = JsRuntime::state(scope); + let state = state_rc.borrow(); + if let Some(format_exception_cb) = &state.js_format_exception_cb { + let format_exception_cb = format_exception_cb.open(scope); + let this = v8::undefined(scope).into(); + let formatted = format_exception_cb.call(scope, this, &[exception]); + if let Some(formatted) = formatted { + if formatted.is_string() { + exception_message = Some(formatted.to_rust_string_lossy(scope)); + } } } } @@ -255,66 +260,73 @@ impl JsError { None => vec![], }; - let state_rc = JsRuntime::state(scope); - let state = state_rc.borrow(); - - // When the stack frame array is empty, but the source location given by - // (script_resource_name, line_number, start_column + 1) exists, this is - // likely a syntax error. For the sake of formatting we treat it like it - // was given as a single stack frame. - if frames.is_empty() { - let script_resource_name = msg - .get_script_resource_name(scope) - .and_then(|v| v8::Local::<v8::String>::try_from(v).ok()) - .map(|v| v.to_rust_string_lossy(scope)); - let line_number: Option<i64> = - msg.get_line_number(scope).and_then(|v| v.try_into().ok()); - let column_number: Option<i64> = msg.get_start_column().try_into().ok(); - if let (Some(f), Some(l), Some(c)) = - (script_resource_name, line_number, column_number) - { - // V8's column numbers are 0-based, we want 1-based. - let c = c + 1; - if let Some(source_map_getter) = &state.source_map_getter { - let (f, l, c, _) = apply_source_map( - f, - l, - c, - &mut HashMap::new(), - source_map_getter.as_ref(), - ); - frames = - vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))]; - } else { - frames = - vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))]; - } - } - } - let mut source_line = None; let mut source_line_frame_index = None; - if let Some(source_map_getter) = &state.source_map_getter { - for (i, frame) in frames.iter().enumerate() { - if let (Some(file_name), Some(line_number)) = - (&frame.file_name, frame.line_number) + { + let state_rc = JsRuntime::state(scope); + let state = &mut *state_rc.borrow_mut(); + + // When the stack frame array is empty, but the source location given by + // (script_resource_name, line_number, start_column + 1) exists, this is + // likely a syntax error. For the sake of formatting we treat it like it + // was given as a single stack frame. + if frames.is_empty() { + let script_resource_name = msg + .get_script_resource_name(scope) + .and_then(|v| v8::Local::<v8::String>::try_from(v).ok()) + .map(|v| v.to_rust_string_lossy(scope)); + let line_number: Option<i64> = + msg.get_line_number(scope).and_then(|v| v.try_into().ok()); + let column_number: Option<i64> = + msg.get_start_column().try_into().ok(); + if let (Some(f), Some(l), Some(c)) = + (script_resource_name, line_number, column_number) { - if !file_name.trim_start_matches('[').starts_with("deno:") { - // Source lookup expects a 0-based line number, ours are 1-based. - source_line = source_map_getter - .get_source_line(file_name, (line_number - 1) as usize); - source_line_frame_index = Some(i); - break; + // V8's column numbers are 0-based, we want 1-based. + let c = c + 1; + if let Some(source_map_getter) = &state.source_map_getter { + let (f, l, c) = apply_source_map( + f, + l, + c, + &mut state.source_map_cache, + source_map_getter.as_ref(), + ); + frames = + vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))]; + } else { + frames = + vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))]; } } } - } else if let Some(frame) = frames.first() { - if let Some(file_name) = &frame.file_name { - if !file_name.trim_start_matches('[').starts_with("deno:") { - source_line = msg - .get_source_line(scope) - .map(|v| v.to_rust_string_lossy(scope)); - source_line_frame_index = Some(0); + + if let Some(source_map_getter) = &state.source_map_getter { + for (i, frame) in frames.iter().enumerate() { + if let (Some(file_name), Some(line_number)) = + (&frame.file_name, frame.line_number) + { + if !file_name.trim_start_matches('[').starts_with("deno:") { + // Source lookup expects a 0-based line number, ours are 1-based. + source_line = get_source_line( + file_name, + line_number, + &mut state.source_map_cache, + source_map_getter.as_ref(), + ); + source_line_frame_index = Some(i); + break; + } + } + } + } else if let Some(frame) = frames.first() { + if let Some(file_name) = &frame.file_name { + if !file_name.trim_start_matches('[').starts_with("deno:") { + source_line = msg + .get_source_line(scope) + .map(|v| v.to_rust_string_lossy(scope)); + source_line_frame_index = Some(0); + } } } } diff --git a/core/ops_builtin_v8.rs b/core/ops_builtin_v8.rs index 7b7cfcc45..7f0c58212 100644 --- a/core/ops_builtin_v8.rs +++ b/core/ops_builtin_v8.rs @@ -752,14 +752,14 @@ fn op_apply_source_map( location: Location, ) -> Result<Location, Error> { let state_rc = JsRuntime::state(scope); - let state = state_rc.borrow(); + let state = &mut *state_rc.borrow_mut(); if let Some(source_map_getter) = &state.source_map_getter { let mut location = location; - let (f, l, c, _) = apply_source_map_( + let (f, l, c) = apply_source_map_( location.file_name, location.line_number.into(), location.column_number.into(), - &mut Default::default(), + &mut state.source_map_cache, source_map_getter.as_ref(), ); location.file_name = f; diff --git a/core/runtime.rs b/core/runtime.rs index 4ca1247f6..97c822848 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -17,6 +17,7 @@ use crate::modules::NoopModuleLoader; use crate::op_void_async; use crate::op_void_sync; use crate::ops::*; +use crate::source_map::SourceMapCache; use crate::source_map::SourceMapGetter; use crate::Extension; use crate::OpMiddlewareFn; @@ -163,6 +164,7 @@ pub(crate) struct JsRuntimeState { /// of the event loop. dyn_module_evaluate_idle_counter: u32, pub(crate) source_map_getter: Option<Box<dyn SourceMapGetter>>, + pub(crate) source_map_cache: SourceMapCache, pub(crate) pending_ops: FuturesUnordered<PendingOpFuture>, pub(crate) unrefed_ops: HashSet<i32>, pub(crate) have_unpolled_ops: bool, @@ -391,6 +393,7 @@ impl JsRuntime { has_tick_scheduled: false, js_wasm_streaming_cb: None, source_map_getter: options.source_map_getter, + source_map_cache: Default::default(), pending_ops: FuturesUnordered::new(), unrefed_ops: HashSet::new(), shared_array_buffer_store: options.shared_array_buffer_store, diff --git a/core/source_map.rs b/core/source_map.rs index 770b29cc7..6a261fa7d 100644 --- a/core/source_map.rs +++ b/core/source_map.rs @@ -17,83 +17,75 @@ pub trait SourceMapGetter { ) -> Option<String>; } -/// Cached filename lookups. The key can be None if a previous lookup failed to -/// find a SourceMap. -pub type CachedMaps = HashMap<String, Option<SourceMap>>; +#[derive(Debug, Default)] +pub struct SourceMapCache { + maps: HashMap<String, Option<SourceMap>>, + source_lines: HashMap<(String, i64), Option<String>>, +} pub fn apply_source_map<G: SourceMapGetter + ?Sized>( file_name: String, line_number: i64, column_number: i64, - mappings_map: &mut CachedMaps, + cache: &mut SourceMapCache, getter: &G, -) -> (String, i64, i64, Option<String>) { +) -> (String, i64, i64) { // Lookup expects 0-based line and column numbers, but ours are 1-based. let line_number = line_number - 1; let column_number = column_number - 1; - let default_pos = (file_name.clone(), line_number, column_number, None); - let maybe_source_map = get_mappings(&file_name, mappings_map, getter); - let (file_name, line_number, column_number, source_line) = - match maybe_source_map { - None => default_pos, - Some(source_map) => { - match source_map.lookup_token(line_number as u32, column_number as u32) - { + let default_pos = (file_name.clone(), line_number, column_number); + let maybe_source_map = + cache.maps.entry(file_name.clone()).or_insert_with(|| { + getter + .get_source_map(&file_name) + .and_then(|raw_source_map| SourceMap::from_slice(&raw_source_map).ok()) + }); + let (file_name, line_number, column_number) = match maybe_source_map { + None => default_pos, + Some(source_map) => { + match source_map.lookup_token(line_number as u32, column_number as u32) { + None => default_pos, + Some(token) => match token.get_source() { None => default_pos, - Some(token) => match token.get_source() { - None => default_pos, - Some(source_file_name) => { - // The `source_file_name` written by tsc in the source map is - // sometimes only the basename of the URL, or has unwanted `<`/`>` - // around it. Use the `file_name` we get from V8 if - // `source_file_name` does not parse as a URL. - let file_name = match resolve_url(source_file_name) { - Ok(m) if m.scheme() == "blob" => file_name, - Ok(m) => m.to_string(), - Err(_) => file_name, - }; - let source_line = - if let Some(source_view) = token.get_source_view() { - source_view - .get_line(token.get_src_line()) - .map(|s| s.to_string()) - } else { - None - }; - ( - file_name, - i64::from(token.get_src_line()), - i64::from(token.get_src_col()), - source_line, - ) - } - }, - } + Some(source_file_name) => { + // The `source_file_name` written by tsc in the source map is + // sometimes only the basename of the URL, or has unwanted `<`/`>` + // around it. Use the `file_name` we get from V8 if + // `source_file_name` does not parse as a URL. + let file_name = match resolve_url(source_file_name) { + Ok(m) if m.scheme() == "blob" => file_name, + Ok(m) => m.to_string(), + Err(_) => file_name, + }; + ( + file_name, + i64::from(token.get_src_line()), + i64::from(token.get_src_col()), + ) + } + }, } - }; - let source_line = source_line - .or_else(|| getter.get_source_line(&file_name, line_number as usize)); - (file_name, line_number + 1, column_number + 1, source_line) + } + }; + (file_name, line_number + 1, column_number + 1) } -fn get_mappings<'a, G: SourceMapGetter + ?Sized>( - file_name: &str, - mappings_map: &'a mut CachedMaps, - getter: &G, -) -> &'a Option<SourceMap> { - mappings_map - .entry(file_name.to_string()) - .or_insert_with(|| parse_map_string(file_name, getter)) -} +const MAX_SOURCE_LINE_LENGTH: usize = 150; -// TODO(kitsonk) parsed source maps should probably be cached in state in -// the module meta data. -fn parse_map_string<G: SourceMapGetter + ?Sized>( +pub fn get_source_line<G: SourceMapGetter + ?Sized>( file_name: &str, + line_number: i64, + cache: &mut SourceMapCache, getter: &G, -) -> Option<SourceMap> { - getter - .get_source_map(file_name) - .and_then(|raw_source_map| SourceMap::from_slice(&raw_source_map).ok()) +) -> Option<String> { + cache + .source_lines + .entry((file_name.to_string(), line_number)) + .or_insert_with(|| { + // Source lookup expects a 0-based line number, ours are 1-based. + let s = getter.get_source_line(file_name, (line_number - 1) as usize); + s.filter(|s| s.len() <= MAX_SOURCE_LINE_LENGTH) + }) + .clone() } |