diff --git a/crates/symbolicator-js/src/interface.rs b/crates/symbolicator-js/src/interface.rs index 71489ff9d..48a1e68d9 100644 --- a/crates/symbolicator-js/src/interface.rs +++ b/crates/symbolicator-js/src/interface.rs @@ -229,9 +229,11 @@ pub struct JsFrame { #[serde(skip_serializing_if = "Option::is_none")] pub module: Option, - pub abs_path: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub abs_path: Option, - pub lineno: u32, + #[serde(skip_serializing_if = "Option::is_none")] + pub lineno: Option, #[serde(skip_serializing_if = "Option::is_none")] pub colno: Option, diff --git a/crates/symbolicator-js/src/lookup.rs b/crates/symbolicator-js/src/lookup.rs index dcc204ad8..c83d8d419 100644 --- a/crates/symbolicator-js/src/lookup.rs +++ b/crates/symbolicator-js/src/lookup.rs @@ -195,16 +195,18 @@ impl SourceMapLookup { for frame in &mut stacktrace.frames { // NOTE: some older JS SDK versions did not correctly strip a leading `async ` // prefix from the `abs_path`, which we will work around here. - if let Some(abs_path) = frame.abs_path.strip_prefix("async ") { - frame.abs_path = abs_path.to_owned(); + if let Some(abs_path) = frame.abs_path.as_ref().and_then(|s| s.strip_prefix("async ")) { + frame.abs_path = Some(abs_path.to_owned()); } - let abs_path = &frame.abs_path; - if self.modules_by_abs_path.contains_key(abs_path) { - continue; + + if let Some(abs_path) = &frame.abs_path { + if self.modules_by_abs_path.contains_key(abs_path) { + continue; + } + let cached_module = SourceMapModule::new(abs_path, None); + self.modules_by_abs_path + .insert(abs_path.to_owned(), cached_module); } - let cached_module = SourceMapModule::new(abs_path, None); - self.modules_by_abs_path - .insert(abs_path.to_owned(), cached_module); } } } diff --git a/crates/symbolicator-js/src/symbolication.rs b/crates/symbolicator-js/src/symbolication.rs index 16f716bc6..de5d9cc51 100644 --- a/crates/symbolicator-js/src/symbolication.rs +++ b/crates/symbolicator-js/src/symbolication.rs @@ -64,7 +64,7 @@ impl SourceMapService { .entry(raw_frame.platform.clone()) .or_default() += 1; errors.insert(JsModuleError { - abs_path: raw_frame.abs_path.clone(), + abs_path: raw_frame.abs_path.clone().unwrap_or_default(), kind: err, }); symbolicated_frames.push(raw_frame.clone()); @@ -102,24 +102,29 @@ async fn symbolicate_js_frame( should_apply_source_context: bool, stats: &mut SymbolicationStats, ) -> Result { + let Some(abs_path) = &raw_frame.abs_path else { + return Err(JsModuleErrorKind::InvalidAbsPath); + }; + // we check for a valid line (i.e. >= 1) first, as we want to avoid resolving / scraping the minified // file in that case. we frequently saw 0 line/col values in combination with non-js files, // and we want to avoid scraping a bunch of html files in that case. - let line = if raw_frame.lineno > 0 { - raw_frame.lineno - } else { - return Err(JsModuleErrorKind::InvalidLocation { - line: raw_frame.lineno, - col: raw_frame.colno, - }); + let line = match raw_frame.lineno { + Some(lineno) if lineno > 0 => lineno, + lineno => { + return Err(JsModuleErrorKind::InvalidLocation { + line: lineno.unwrap_or(0), + col: raw_frame.colno, + }); + } }; let col = raw_frame.colno.unwrap_or_default(); - let module = lookup.get_module(&raw_frame.abs_path).await; + let module = lookup.get_module(&abs_path).await; tracing::trace!( - abs_path = &raw_frame.abs_path, + abs_path = &abs_path, ?module, "Module for `abs_path`" ); @@ -154,7 +159,7 @@ async fn symbolicate_js_frame( .map(|entry| entry.sourcemap_url()) .ok() .flatten() - .unwrap_or_else(|| raw_frame.abs_path.clone()); + .unwrap_or_else(|| raw_frame.abs_path.clone().unwrap_or_default()); let (smcache, resolved_with, sourcemap_origin) = match &module.smcache { Some(smcache) => match &smcache.entry { @@ -229,31 +234,32 @@ async fn symbolicate_js_frame( if let Some(filename) = token.file_name() { let mut filename = filename.to_string(); - frame.abs_path = module + frame.abs_path = Some(module .source_file_base() .map(|base| join_paths(base, &filename)) - .unwrap_or_else(|| filename.clone()); + .unwrap_or_else(|| filename.clone())); if filename.starts_with("webpack:") { filename = fixup_webpack_filename(&filename); frame.module = Some(generate_module(&filename)); } - frame.in_app = is_in_app(&frame.abs_path, &filename); + let abs_path = frame.abs_path.clone().unwrap_or_default(); + frame.in_app = is_in_app(&abs_path, &filename); if frame.module.is_none() - && (frame.abs_path.starts_with("http:") - || frame.abs_path.starts_with("https:") - || frame.abs_path.starts_with("webpack:") - || frame.abs_path.starts_with("app:")) + && (abs_path.starts_with("http:") + || abs_path.starts_with("https:") + || abs_path.starts_with("webpack:") + || abs_path.starts_with("app:")) { - frame.module = Some(generate_module(&frame.abs_path)); + frame.module = Some(generate_module(&abs_path)); } frame.filename = Some(filename); } - frame.lineno = token.line().saturating_add(1); + frame.lineno = Some(token.line().saturating_add(1)); frame.colno = Some(token.column().saturating_add(1)); if !should_apply_source_context { @@ -264,7 +270,7 @@ async fn symbolicate_js_frame( if let Some(file_source) = file.source() { if let Err(err) = apply_source_context(&mut frame, file_source) { errors.insert(JsModuleError { - abs_path: raw_frame.abs_path.clone(), + abs_path: raw_frame.abs_path.clone().unwrap_or_default(), kind: err, }); } @@ -291,7 +297,7 @@ async fn symbolicate_js_frame( // It's arguable whether we should collect it, but this is what monolith does now, // and it might be useful to indicate incorrect sentry-cli rewrite behavior. errors.insert(JsModuleError { - abs_path: raw_frame.abs_path.clone(), + abs_path: raw_frame.abs_path.clone().unwrap_or_default(), kind: JsModuleErrorKind::MissingSourceContent { source: file_key .and_then(|key| key.abs_path().map(|path| path.to_string())) @@ -307,11 +313,14 @@ async fn symbolicate_js_frame( } fn apply_source_context(frame: &mut JsFrame, source: &str) -> Result<(), JsModuleErrorKind> { - let lineno = frame.lineno as usize; + let Some(lineno) = frame.lineno else { + return Ok(()); + }; + let column = frame.colno.map(|col| col as usize).unwrap_or_default(); if let Some((pre_context, context_line, post_context)) = - get_context_lines(source, lineno, column, None) + get_context_lines(source, lineno as usize, column, None) { frame.pre_context = pre_context; frame.context_line = Some(context_line); diff --git a/crates/symbolicator-js/tests/integration/snapshots/integration__sourcemap__invalid_abs_path.snap b/crates/symbolicator-js/tests/integration/snapshots/integration__sourcemap__invalid_abs_path.snap new file mode 100644 index 000000000..e230f0601 --- /dev/null +++ b/crates/symbolicator-js/tests/integration/snapshots/integration__sourcemap__invalid_abs_path.snap @@ -0,0 +1,22 @@ +--- +source: crates/symbolicator-js/tests/integration/sourcemap.rs +assertion_line: 380 +expression: response +--- +stacktraces: + - frames: + - filename: test.js + lineno: 1 + colno: 1 + data: + symbolicated: false +raw_stacktraces: + - frames: + - filename: test.js + lineno: 1 + colno: 1 + data: + symbolicated: false +errors: + - abs_path: "" + type: invalid_abs_path diff --git a/crates/symbolicator-js/tests/integration/snapshots/integration__sourcemap__invalid_lineno.snap b/crates/symbolicator-js/tests/integration/snapshots/integration__sourcemap__invalid_lineno.snap new file mode 100644 index 000000000..f825f988e --- /dev/null +++ b/crates/symbolicator-js/tests/integration/snapshots/integration__sourcemap__invalid_lineno.snap @@ -0,0 +1,22 @@ +--- +source: crates/symbolicator-js/tests/integration/sourcemap.rs +assertion_line: 397 +expression: response +--- +stacktraces: + - frames: + - filename: test.js + abs_path: http//example.com/test.min.js + data: + symbolicated: false +raw_stacktraces: + - frames: + - filename: test.js + abs_path: http//example.com/test.min.js + data: + symbolicated: false +errors: + - abs_path: http//example.com/test.min.js + type: invalid_location + line: 0 + col: ~ diff --git a/crates/symbolicator-js/tests/integration/sourcemap.rs b/crates/symbolicator-js/tests/integration/sourcemap.rs index 980567014..c99e4f37e 100644 --- a/crates/symbolicator-js/tests/integration/sourcemap.rs +++ b/crates/symbolicator-js/tests/integration/sourcemap.rs @@ -363,6 +363,40 @@ async fn test_malformed_abs_path() { assert_snapshot!(response); } +#[tokio::test] +async fn test_invalid_abs_path() { + let (symbolication, _cache_dir) = setup_service(|_| ()); + let (_srv, source) = sourcemap_server("", |_url, _query| json!([])); + + let frames = r#"[{ + "filename": "test.js", + "lineno": 1, + "colno": 1 + }]"#; + + let request = make_js_request(source, frames, "[]", String::from("release"), None); + let response = symbolication.symbolicate_js(request).await; + + assert_snapshot!(response); +} + + +#[tokio::test] +async fn test_invalid_lineno() { + let (symbolication, _cache_dir) = setup_service(|_| ()); + let (_srv, source) = sourcemap_server("", |_url, _query| json!([])); + + let frames = r#"[{ + "abs_path": "http//example.com/test.min.js", + "filename": "test.js" + }]"#; + + let request = make_js_request(source, frames, "[]", String::from("release"), None); + let response = symbolication.symbolicate_js(request).await; + + assert_snapshot!(response); +} + #[tokio::test] async fn test_fetch_error() { let (symbolication, _cache_dir) = setup_service(|_| ()); diff --git a/crates/symbolicli/src/main.rs b/crates/symbolicli/src/main.rs index d91bc9948..3b371da13 100644 --- a/crates/symbolicli/src/main.rs +++ b/crates/symbolicli/src/main.rs @@ -727,8 +727,8 @@ mod event { function: value.function, filename: value.filename, module: value.module, - abs_path: value.abs_path?, - lineno: value.lineno?, + abs_path: value.abs_path, + lineno: value.lineno, colno: value.colno, pre_context: value.pre_context, context_line: value.context_line, diff --git a/crates/symbolicli/src/output.rs b/crates/symbolicli/src/output.rs index 47c871641..552e9d74e 100644 --- a/crates/symbolicli/src/output.rs +++ b/crates/symbolicli/src/output.rs @@ -240,7 +240,8 @@ fn print_compact_js(mut response: CompletedJsSymbolicationResponse) { let filename = filename.unwrap_or_default(); row.add_cell(cell!(filename)); - row.add_cell(cell!(lineno)); + let line = lineno.map(|line| line.to_string()).unwrap_or_default(); + row.add_cell(cell!(line)); let col = colno.map(|col| col.to_string()).unwrap_or_default(); row.add_cell(cell!(col)); @@ -290,10 +291,19 @@ fn print_pretty_js(mut response: CompletedJsSymbolicationResponse) { table.add_row(row![r->" File:", name]); } - if let Some(col) = *colno { - table.add_row(row![r->" Line/Column:", format!("{lineno}:{col}")]); - } else { - table.add_row(row![r->" Line:", format!("{lineno}")]); + match (colno, lineno) { + (Some(col), Some(line)) => { + table.add_row(row![r->" Line/Column:", format!("{}:{}", line, col)]); + } + (Some(col), None) => { + table.add_row(row![r->" Column:", format!("{}", col)]); + } + (None, Some(line)) => { + table.add_row(row![r->" Line:", format!("{}", line)]); + } + (None, None) => { + table.add_row(row![r->" Line/Column:", "N/A"]); + } } table.add_empty_row();