Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow optional abs_path and lineno for JsFrame #1581

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
+142 −41
Open

fix: allow optional abs_path and lineno for JsFrame #1581

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b3dd3fc
fix: allow optional JsFrame.abs_path
samuelmaddock Jan 8, 2025
bb3b9d1
fix: allow optional JsFrame.lineno
samuelmaddock Jan 8, 2025
2b799fe
fix: tests
samuelmaddock Jan 8, 2025
94c1451
Update crates/symbolicator-js/src/lookup.rs
samuelmaddock Jan 9, 2025
76e9a06
Update crates/symbolicator-js/src/symbolication.rs
samuelmaddock Jan 9, 2025
4ae2c8c
skip context lookup when no lineno is set
samuelmaddock Jan 9, 2025
f0add7a
fix: use as_ref
samuelmaddock Jan 9, 2025
b381fe3
error for invalid abs_path
samuelmaddock Jan 9, 2025
b8844cb
Update crates/symbolicator-js/src/symbolication.rs
samuelmaddock Jan 10, 2025
69bbf08
Update crates/symbolicator-js/src/symbolication.rs
samuelmaddock Jan 10, 2025
4347816
Update crates/symbolicator-js/src/symbolication.rs
samuelmaddock Jan 10, 2025
1ff052e
check for valid abs path first
samuelmaddock Jan 10, 2025
12d1d5e
fix compilation errors
samuelmaddock Jan 10, 2025
41144b1
test: symbolicate js with invalid abs_path or lineno
samuelmaddock Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions crates/symbolicator-js/src/interface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,11 @@ pub struct JsFrame {
#[serde(skip_serializing_if = "Option::is_none")]
pub module: Option<String>,

pub abs_path: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub abs_path: Option<String>,

pub lineno: u32,
#[serde(skip_serializing_if = "Option::is_none")]
pub lineno: Option<u32>,

#[serde(skip_serializing_if = "Option::is_none")]
pub colno: Option<u32>,
Expand Down
18 changes: 10 additions & 8 deletions crates/symbolicator-js/src/lookup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
59 changes: 36 additions & 23 deletions crates/symbolicator-js/src/symbolication.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -105,21 +105,29 @@ async fn symbolicate_js_frame(
// 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 abs_path = match raw_frame.abs_path {
Some(ref abs_path) => abs_path,
None => {
return Err(JsModuleErrorKind::InvalidAbsPath);
samuelmaddock marked this conversation as resolved.
Show resolved Hide resolved
}
};
samuelmaddock marked this conversation as resolved.
Show resolved Hide resolved

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;
samuelmaddock marked this conversation as resolved.
Show resolved Hide resolved

tracing::trace!(
abs_path = &raw_frame.abs_path,
abs_path = &abs_path,
?module,
"Module for `abs_path`"
);
Expand Down Expand Up @@ -154,7 +162,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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use abs_path.to_owned() here now, I think, since we already checked for Some before.

Suggested change
.unwrap_or_else(|| raw_frame.abs_path.clone().unwrap_or_default());
.unwrap_or_else(|| abs_path.to_owned());

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to revert this in 12d1d5e to fix a compilation error

error[E0502]: cannot borrow `*raw_frame` as mutable because it is also borrowed as immutable
   --> crates/symbolicator-js/src/symbolication.rs:145:17
    |
105 |   ...ath = raw_frame
    |  __________-
106 | | ...path
    | |_______- immutable borrow occurs here
...
145 |   ...   apply_source_context(raw_frame, minified_source.contents(...
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
...
163 |   ...ap_or_else(|| abs_path.to_owned());
    |                    -------- immutable borrow later captured here by closure


let (smcache, resolved_with, sourcemap_origin) = match &module.smcache {
Some(smcache) => match &smcache.entry {
Expand Down Expand Up @@ -229,31 +237,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 {
Expand All @@ -264,7 +273,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,
});
}
Expand All @@ -291,7 +300,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()))
Expand All @@ -307,7 +316,11 @@ async fn symbolicate_js_frame(
}

fn apply_source_context(frame: &mut JsFrame, source: &str) -> Result<(), JsModuleErrorKind> {
let lineno = frame.lineno as usize;
if frame.lineno.is_none() {
return Ok(());
}
samuelmaddock marked this conversation as resolved.
Show resolved Hide resolved

let lineno = frame.lineno.map(|line| line as usize).unwrap_or_default();
samuelmaddock marked this conversation as resolved.
Show resolved Hide resolved
let column = frame.colno.map(|col| col as usize).unwrap_or_default();

if let Some((pre_context, context_line, post_context)) =
Expand Down
4 changes: 2 additions & 2 deletions crates/symbolicli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
20 changes: 15 additions & 5 deletions crates/symbolicli/src/output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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();
Expand Down
Loading