From 068b3c66520ae0ac227fd2d3f2c65b3e54a96939 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 10 Oct 2022 10:07:35 +0200 Subject: [PATCH 1/7] ref: Persist SourceMap `name` in the cache. Anonymous functions are most of the time being called through a variable binding, which when minified gets a `name` mapping. This information from the parent/caller frame can be used as a good heuristic as fallback for a scope name. --- symbolic-sourcemapcache/Cargo.toml | 2 +- symbolic-sourcemapcache/src/lookup.rs | 13 +++++++++++++ symbolic-sourcemapcache/src/raw.rs | 13 +++++++++++-- symbolic-sourcemapcache/src/writer.rs | 17 ++++++++++++----- symbolic-sourcemapcache/tests/integration.rs | 6 +++++- 5 files changed, 42 insertions(+), 9 deletions(-) diff --git a/symbolic-sourcemapcache/Cargo.toml b/symbolic-sourcemapcache/Cargo.toml index 1b40ade18..eae75bdcd 100644 --- a/symbolic-sourcemapcache/Cargo.toml +++ b/symbolic-sourcemapcache/Cargo.toml @@ -12,7 +12,7 @@ A fast lookup cache for JavaScript Source Maps. edition = "2021" [dependencies] -js-source-scopes = "0.1.0" +js-source-scopes = { path = "../../js-source-scopes" } thiserror = "1.0.31" sourcemap = "6.1.0" tracing = "0.1.36" diff --git a/symbolic-sourcemapcache/src/lookup.rs b/symbolic-sourcemapcache/src/lookup.rs index 9da0ea559..2fca13f69 100644 --- a/symbolic-sourcemapcache/src/lookup.rs +++ b/symbolic-sourcemapcache/src/lookup.rs @@ -13,6 +13,8 @@ pub struct SourceLocation<'data> { line: u32, /// The source column. column: u32, + /// The `name` of the source location. + name: Option<&'data str>, /// The scope containing this source location. scope: ScopeLookupResult<'data>, } @@ -33,6 +35,11 @@ impl<'data> SourceLocation<'data> { self.column } + /// The `name` of this source location. + pub fn name(&self) -> Option<&'data str> { + self.name + } + /// The contents of the source line. pub fn line_contents(&self) -> Option<&'data str> { self.file().and_then(|file| file.line(self.line as usize)) @@ -169,6 +176,11 @@ impl<'data> SourceMapCache<'data> { .get(sl.file_idx as usize) .and_then(|raw_file| self.resolve_file(raw_file)); + let name = match sl.name_idx { + raw::NO_NAME_SENTINEL => None, + idx => self.get_string(idx), + }; + let scope = match sl.scope_idx { raw::GLOBAL_SCOPE_SENTINEL => ScopeLookupResult::Unknown, raw::ANONYMOUS_SCOPE_SENTINEL => ScopeLookupResult::AnonymousScope, @@ -181,6 +193,7 @@ impl<'data> SourceMapCache<'data> { file, line, column, + name, scope, }) } diff --git a/symbolic-sourcemapcache/src/raw.rs b/symbolic-sourcemapcache/src/raw.rs index 3e4cb44d0..2ce74bb78 100644 --- a/symbolic-sourcemapcache/src/raw.rs +++ b/symbolic-sourcemapcache/src/raw.rs @@ -12,7 +12,12 @@ pub const SOURCEMAPCACHE_MAGIC: u32 = u32::from_le_bytes(SOURCEMAPCACHE_MAGIC_BY pub const SOURCEMAPCACHE_MAGIC_FLIPPED: u32 = SOURCEMAPCACHE_MAGIC.swap_bytes(); /// The current Format version -pub const SOURCEMAPCACHE_VERSION: u32 = 1; +/// +/// # Version History +/// +/// - 2: Added `name` reference +/// - 1: Initial version +pub const SOURCEMAPCACHE_VERSION: u32 = 2; /// The SourceMapCache binary Header. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -59,6 +64,8 @@ impl From for MinifiedSourcePosition { /// Sentinel value used to denote unknown file. pub const NO_FILE_SENTINEL: u32 = u32::MAX; +/// Sentinel value used to denote no `name`. +pub const NO_NAME_SENTINEL: u32 = u32::MAX; /// Sentinel value used to denote unknown/global scope. pub const GLOBAL_SCOPE_SENTINEL: u32 = u32::MAX; /// Sentinel value used to denote anonymous function scope. @@ -74,6 +81,8 @@ pub struct OriginalSourceLocation { pub line: u32, /// The original column number. pub column: u32, + /// The optional `name` of this token (offset into string table). + pub name_idx: u32, /// The optional scope name (offset into string table). pub scope_idx: u32, } @@ -117,7 +126,7 @@ mod tests { assert_eq!(mem::size_of::(), 8); assert_eq!(mem::align_of::(), 4); - assert_eq!(mem::size_of::(), 16); + assert_eq!(mem::size_of::(), 20); assert_eq!(mem::align_of::(), 4); } } diff --git a/symbolic-sourcemapcache/src/writer.rs b/symbolic-sourcemapcache/src/writer.rs index 35dc4e009..07e28178f 100644 --- a/symbolic-sourcemapcache/src/writer.rs +++ b/symbolic-sourcemapcache/src/writer.rs @@ -8,7 +8,7 @@ use js_source_scopes::{ use sourcemap::DecodedMap; use watto::{Pod, StringTable, Writer}; -use super::raw::{self, ANONYMOUS_SCOPE_SENTINEL, GLOBAL_SCOPE_SENTINEL, NO_FILE_SENTINEL}; +use super::raw; use super::{ScopeLookupResult, SourcePosition}; /// A structure that allows quick resolution of minified source position @@ -153,21 +153,28 @@ impl SourceMapCacheWriter { let mut file_idx = token.get_src_id(); if file_idx >= files.len() as u32 { - file_idx = NO_FILE_SENTINEL; + file_idx = raw::NO_FILE_SENTINEL; } let scope_idx = match scope { ScopeLookupResult::NamedScope(name) => { - std::cmp::min(string_table.insert(name) as u32, GLOBAL_SCOPE_SENTINEL) + std::cmp::min(string_table.insert(name) as u32, raw::GLOBAL_SCOPE_SENTINEL) } - ScopeLookupResult::AnonymousScope => ANONYMOUS_SCOPE_SENTINEL, - ScopeLookupResult::Unknown => GLOBAL_SCOPE_SENTINEL, + ScopeLookupResult::AnonymousScope => raw::ANONYMOUS_SCOPE_SENTINEL, + ScopeLookupResult::Unknown => raw::GLOBAL_SCOPE_SENTINEL, + }; + + let name = token.get_name(); + let name_idx = match name { + Some(name) => string_table.insert(name) as u32, + None => raw::NO_NAME_SENTINEL, }; let sl = raw::OriginalSourceLocation { file_idx, line, column, + name_idx, scope_idx, }; diff --git a/symbolic-sourcemapcache/tests/integration.rs b/symbolic-sourcemapcache/tests/integration.rs index 3a15181a9..0b1439cf1 100644 --- a/symbolic-sourcemapcache/tests/integration.rs +++ b/symbolic-sourcemapcache/tests/integration.rs @@ -29,11 +29,15 @@ fn resolves_inlined_function() { assert_eq!(sl.column(), 2); assert_eq!(sl.scope(), ScopeLookupResult::NamedScope("bar")); + // NOTE: The last source position itself does not have a named scope, it truely is an + // anonymous function. However, the *call* itself has a `name` which we use in its place. + assert_eq!(sl.name(), Some("foo")); + let sl = cache.lookup(SourcePosition::new(0, 33)).unwrap(); assert_eq!(sl.file_name(), Some("../src/foo.js")); assert_eq!(sl.line(), 1); assert_eq!(sl.column(), 8); - assert_eq!(sl.scope(), ScopeLookupResult::NamedScope("foo")); + assert_eq!(sl.scope(), ScopeLookupResult::AnonymousScope); } #[test] From f1820c4a73e404ae0853fae33cf65377084b2013 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 10 Oct 2022 12:39:24 +0200 Subject: [PATCH 2/7] give better description for name --- symbolic-sourcemapcache/src/lookup.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/symbolic-sourcemapcache/src/lookup.rs b/symbolic-sourcemapcache/src/lookup.rs index 2fca13f69..70a255c6a 100644 --- a/symbolic-sourcemapcache/src/lookup.rs +++ b/symbolic-sourcemapcache/src/lookup.rs @@ -35,7 +35,9 @@ impl<'data> SourceLocation<'data> { self.column } - /// The `name` of this source location. + /// The `name` of this source location as it is defined in the SourceMap. + /// + /// This can be useful when inferring the name of a scope from the callers call expression. pub fn name(&self) -> Option<&'data str> { self.name } From d8b98dfd57f7a5f07fca5f2ad519242c154fb5f0 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 10 Oct 2022 13:25:38 +0200 Subject: [PATCH 3/7] log errors from parsing --- symbolic-sourcemapcache/src/writer.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/symbolic-sourcemapcache/src/writer.rs b/symbolic-sourcemapcache/src/writer.rs index 07e28178f..f197cf835 100644 --- a/symbolic-sourcemapcache/src/writer.rs +++ b/symbolic-sourcemapcache/src/writer.rs @@ -57,7 +57,16 @@ impl SourceMapCacheWriter { }; // parse scopes out of the minified source - let scopes = extract_scope_names(source); + let scopes = match extract_scope_names(source) { + Ok(scopes) => scopes, + Err(err) => { + let err: &dyn std::error::Error = &err; + tracing::error!(error = err, "failed parsing minified source"); + // even if the minified source failed parsing, we can still use the information + // from the sourcemap itself. + vec![] + } + }; // resolve scopes to original names let ctx = SourceContext::new(source).map_err(SourceMapCacheErrorInner::SourceContext)?; From f1412ef335ed1c3bc438d469a0614c49b6af6bf3 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Tue, 11 Oct 2022 12:50:59 +0200 Subject: [PATCH 4/7] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d603d1797..92cbb80e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Features**: - Added an Object type for Portable PDB files. ([#696](https://github.com/getsentry/symbolic/pull/696)) +- Version 2 of the sourcemapcache format additionally saves the names of source locations. ([#698](https://github.com/getsentry/symbolic/pull/698)) ## 9.2.1 From 5b9bedd8f9aabb1965e870ff117cc6f939cb3f7e Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Tue, 11 Oct 2022 13:05:42 +0200 Subject: [PATCH 5/7] use token.name in cabi --- symbolic-cabi/src/sourcemapcache.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/symbolic-cabi/src/sourcemapcache.rs b/symbolic-cabi/src/sourcemapcache.rs index 2186d2187..40e347b50 100644 --- a/symbolic-cabi/src/sourcemapcache.rs +++ b/symbolic-cabi/src/sourcemapcache.rs @@ -115,8 +115,8 @@ ffi_fn! { fn make_token_match(token: SourceLocation, context_lines: u32) -> *mut SymbolicSmTokenMatch { let function_name = match token.scope() { ScopeLookupResult::NamedScope(name) => name, - ScopeLookupResult::AnonymousScope => "", - ScopeLookupResult::Unknown => "", + ScopeLookupResult::AnonymousScope => token.name().unwrap_or(""), + ScopeLookupResult::Unknown => token.name().unwrap_or(""), }; let context_line = token.line_contents().unwrap_or_default(); From f1c717e0e2ed0c96dc58e81a6bbef6af29462612 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Tue, 11 Oct 2022 13:11:55 +0200 Subject: [PATCH 6/7] Revert "use token.name in cabi" This reverts commit 5b9bedd8f9aabb1965e870ff117cc6f939cb3f7e. --- symbolic-cabi/src/sourcemapcache.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/symbolic-cabi/src/sourcemapcache.rs b/symbolic-cabi/src/sourcemapcache.rs index 40e347b50..2186d2187 100644 --- a/symbolic-cabi/src/sourcemapcache.rs +++ b/symbolic-cabi/src/sourcemapcache.rs @@ -115,8 +115,8 @@ ffi_fn! { fn make_token_match(token: SourceLocation, context_lines: u32) -> *mut SymbolicSmTokenMatch { let function_name = match token.scope() { ScopeLookupResult::NamedScope(name) => name, - ScopeLookupResult::AnonymousScope => token.name().unwrap_or(""), - ScopeLookupResult::Unknown => token.name().unwrap_or(""), + ScopeLookupResult::AnonymousScope => "", + ScopeLookupResult::Unknown => "", }; let context_line = token.line_contents().unwrap_or_default(); From 52e9e3f3f01b439daeb730cc0049ce610db58405 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 17 Oct 2022 12:44:45 +0200 Subject: [PATCH 7/7] update js-source-scopes --- symbolic-sourcemapcache/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/symbolic-sourcemapcache/Cargo.toml b/symbolic-sourcemapcache/Cargo.toml index eae75bdcd..f7835648d 100644 --- a/symbolic-sourcemapcache/Cargo.toml +++ b/symbolic-sourcemapcache/Cargo.toml @@ -12,7 +12,7 @@ A fast lookup cache for JavaScript Source Maps. edition = "2021" [dependencies] -js-source-scopes = { path = "../../js-source-scopes" } +js-source-scopes = "0.2.0" thiserror = "1.0.31" sourcemap = "6.1.0" tracing = "0.1.36"