diff --git a/CHANGELOG.md b/CHANGELOG.md index 5393203a4..166dc1ce5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Various fixes & improvements + +- In native symbolication, trying to download a source file from a + sourcelink now creates a candidate ([#1507](https://github.com/getsentry/symbolicator/pull/1510)). + ### Dependencies - Bump Native SDK from v0.7.7 to v0.7.8 ([#1510](https://github.com/getsentry/symbolicator/pull/1510)) diff --git a/crates/symbolicator-native/src/symbolication/module_lookup.rs b/crates/symbolicator-native/src/symbolication/module_lookup.rs index 3c8842204..70c1dbb07 100644 --- a/crates/symbolicator-native/src/symbolication/module_lookup.rs +++ b/crates/symbolicator-native/src/symbolication/module_lookup.rs @@ -7,8 +7,8 @@ use sentry::{Hub, SentryFutureExt}; use symbolic::debuginfo::ObjectDebugSession; use symbolicator_service::caching::{CacheEntry, CacheError}; use symbolicator_service::objects::{ - AllObjectCandidates, FindObject, FindResult, ObjectFeatures, ObjectHandle, ObjectPurpose, - ObjectsActor, + AllObjectCandidates, FindObject, FindResult, ObjectCandidate, ObjectFeatures, ObjectHandle, + ObjectPurpose, ObjectsActor, }; use symbolicator_service::source_context::get_context_lines; use symbolicator_service::types::{ObjectFileStatus, RawObjectInfo, Scope}; @@ -389,12 +389,14 @@ impl ModuleLookup { } /// Update the frame with source context, if available. - /// Returns an `Url` in case the source code has to be fetched. + /// + /// Return a triple of scope, URL, and module index + /// in case the source code has to be fetched. pub(crate) fn try_set_source_context( &self, debug_sessions: &DebugSessions<'_>, frame: &mut RawFrame, - ) -> Option<(Scope, url::Url)> { + ) -> Option<(Scope, url::Url, usize)> { let abs_path = frame.abs_path.as_ref()?; // Short-circuit here before accessing the source. Line number is required to resolve the context. @@ -425,7 +427,7 @@ impl ModuleLookup { None } else { // Let caller know this source code may be resolved from a remote URL. - filtered_url.map(|url| (session.0.clone(), url)) + filtered_url.map(|url| (session.0.clone(), url, entry.module_index)) } } @@ -472,6 +474,17 @@ impl ModuleLookup { .find(|entry| entry.module_index == this_module_index), } } + + /// Adds an `ObjectCandidate` to the module with index `idx`. + /// + /// If there is no module at that index, this is a no-op. + pub fn add_candidate(&mut self, idx: usize, candidate: &ObjectCandidate) { + let Some(entry) = self.modules.get_mut(idx) else { + return; + }; + + entry.object_info.candidates.merge_one(candidate); + } } #[cfg(test)] diff --git a/crates/symbolicator-native/src/symbolication/source_context.rs b/crates/symbolicator-native/src/symbolication/source_context.rs index 6cc2ad66e..6a0bc2102 100644 --- a/crates/symbolicator-native/src/symbolication/source_context.rs +++ b/crates/symbolicator-native/src/symbolication/source_context.rs @@ -1,9 +1,13 @@ use std::collections::HashMap; use futures::future; +use symbolicator_service::caching::{CacheEntry, CacheError}; +use symbolicator_service::objects::{ + ObjectCandidate, ObjectDownloadInfo, ObjectFeatures, ObjectUseInfo, +}; use symbolicator_service::types::{Scope, ScrapingConfig}; use symbolicator_service::utils::http::is_valid_origin; -use symbolicator_sources::HttpRemoteFile; +use symbolicator_sources::{HttpRemoteFile, RemoteFileUri, SourceId}; use crate::interface::{CompleteStacktrace, RawFrame}; @@ -21,23 +25,25 @@ impl SymbolicationActor { .fetch_sources(self.objects.clone(), stacktraces) .await; - // Map collected source contexts to frames and collect URLs for remote source links. - let mut remote_sources: HashMap<(Scope, url::Url), Vec<&mut RawFrame>> = HashMap::new(); + // Map collected source contexts to the index of the module + // and the list of frames they belong to and collect URLs + // for remote source links. + let mut remote_sources: HashMap<(Scope, url::Url), (usize, Vec<&mut RawFrame>)> = + HashMap::new(); { let debug_sessions = module_lookup.prepare_debug_sessions(); for trace in stacktraces { for frame in &mut trace.frames { - let Some(url) = + let Some((scope, url, module_idx)) = module_lookup.try_set_source_context(&debug_sessions, &mut frame.raw) else { continue; }; - if let Some(vec) = remote_sources.get_mut(&url) { - vec.push(&mut frame.raw) - } else { - remote_sources.insert(url, vec![&mut frame.raw]); - } + let (_idx, frames) = remote_sources + .entry((scope, url)) + .or_insert((module_idx, vec![])); + frames.push(&mut frame.raw); } } } @@ -45,32 +51,78 @@ impl SymbolicationActor { // Download remote sources and update contexts. if !remote_sources.is_empty() { let cache = self.sourcefiles_cache.as_ref(); - let futures = - remote_sources - .into_iter() - .map(|((source_scope, url), frames)| async move { - let mut remote_file = - HttpRemoteFile::from_url(url.clone(), scraping.verify_ssl); + let futures = remote_sources.into_iter().map( + |((source_scope, url), (module_idx, frames))| async move { + let mut remote_file = + HttpRemoteFile::from_url(url.clone(), scraping.verify_ssl); - if scraping.enabled && is_valid_origin(&url, &scraping.allowed_origins) { - remote_file.headers.extend( - scraping - .headers - .iter() - .map(|(key, value)| (key.clone(), value.clone())), - ); - } + if scraping.enabled && is_valid_origin(&url, &scraping.allowed_origins) { + remote_file.headers.extend( + scraping + .headers + .iter() + .map(|(key, value)| (key.clone(), value.clone())), + ); + } + + let uri = remote_file.uri(); + let res = cache + .fetch_file(&source_scope, remote_file.into(), true) + .await; - if let Ok(source) = cache - .fetch_file(&source_scope, remote_file.into(), true) - .await - { - for frame in frames { - ModuleLookup::set_source_context(&source, frame); - } + if let Ok(source) = res.as_ref() { + for frame in frames { + ModuleLookup::set_source_context(source, frame); } - }); - future::join_all(futures).await; + } + + (module_idx, Self::object_candidate_for_sourcelink(uri, res)) + }, + ); + + let candidates = future::join_all(futures).await; + + // Merge the candidates resulting from source downloads into the + // respective objects. + for (module_idx, candidate) in candidates { + module_lookup.add_candidate(module_idx, &candidate); + } + } + } + + // Creates an `ObjectCandidate` based on trying to download a + // source file from a link. + fn object_candidate_for_sourcelink( + location: RemoteFileUri, + res: CacheEntry, + ) -> ObjectCandidate { + let source = SourceId::new("sourcelink"); + let unwind = ObjectUseInfo::None; + let debug = ObjectUseInfo::None; + let download = match res { + Ok(_) => ObjectDownloadInfo::Ok { + features: ObjectFeatures { + has_sources: true, + ..Default::default() + }, + }, + // Same logic as in `create_candidate_info`. + Err(error) => match error { + CacheError::NotFound => ObjectDownloadInfo::NotFound, + CacheError::PermissionDenied(details) => ObjectDownloadInfo::NoPerm { details }, + CacheError::Malformed(_) => ObjectDownloadInfo::Malformed, + err => ObjectDownloadInfo::Error { + details: err.to_string(), + }, + }, + }; + + ObjectCandidate { + source, + location, + download, + unwind, + debug, } } } diff --git a/crates/symbolicator-native/tests/integration/snapshots/integration__public_sources__nuget_source.snap b/crates/symbolicator-native/tests/integration/snapshots/integration__public_sources__nuget_source.snap index 62add7066..e74260f67 100644 --- a/crates/symbolicator-native/tests/integration/snapshots/integration__public_sources__nuget_source.snap +++ b/crates/symbolicator-native/tests/integration/snapshots/integration__public_sources__nuget_source.snap @@ -1,6 +1,5 @@ --- -source: crates/symbolicator-service/tests/integration/public_sources.rs -assertion_line: 48 +source: crates/symbolicator-native/tests/integration/public_sources.rs expression: response.unwrap() --- stacktraces: @@ -56,4 +55,12 @@ modules: has_sources: true debug: status: ok - + - source: sourcelink + location: "https://raw.githubusercontent.com/mattjohnsonpint/TimeZoneConverter/dab355a1e878bfbfd4c659ddb568ca69961d579e/src/TimeZoneConverter/TZConvert.cs" + download: + status: ok + features: + has_debug_info: false + has_unwind_info: false + has_symbols: false + has_sources: true diff --git a/crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_only_source_links.snap b/crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_only_source_links.snap index a8912532e..42b435ab5 100644 --- a/crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_only_source_links.snap +++ b/crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_only_source_links.snap @@ -1,6 +1,5 @@ --- -source: crates/symbolicator-service/tests/integration/symbolication.rs -assertion_line: 226 +source: crates/symbolicator-native/tests/integration/symbolication.rs expression: response.unwrap() --- stacktraces: @@ -54,4 +53,12 @@ modules: location: "http://localhost:/symbols/source-links.pdb/0C380A12822140698565BEE6B3AC196Effffffff/source-links.src.zip" download: status: notfound - + - source: sourcelink + location: "https://raw.githubusercontent.com/getsentry/sentry-dotnet/b31b62192e6934ea04396456461f430e143cf4f9/samples/Sentry.Samples.Console.Basic/Program.cs" + download: + status: ok + features: + has_debug_info: false + has_unwind_info: false + has_symbols: false + has_sources: true diff --git a/crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_source_links.snap b/crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_source_links.snap index 5a0f66bcb..3f7488669 100644 --- a/crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_source_links.snap +++ b/crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_source_links.snap @@ -1,6 +1,5 @@ --- -source: crates/symbolicator-service/tests/integration/symbolication.rs -assertion_line: 201 +source: crates/symbolicator-native/tests/integration/symbolication.rs expression: response.unwrap() --- stacktraces: @@ -60,4 +59,12 @@ modules: location: "http://localhost:/symbols/source-links.pdb/37E9E8A61A8E404EB93C6902E277FF55ffffffff/source-links.src.zip" download: status: notfound - + - source: sourcelink + location: "https://raw.githubusercontent.com/dotnet/runtime/d099f075e45d2aa6007a22b71b45a08758559f80/src/libraries/Common/src/System/ThrowHelper.cs" + download: + status: ok + features: + has_debug_info: false + has_unwind_info: false + has_symbols: false + has_sources: true diff --git a/crates/symbolicator-service/src/objects/candidates.rs b/crates/symbolicator-service/src/objects/candidates.rs index 1aa24cc3e..b1aa372b6 100644 --- a/crates/symbolicator-service/src/objects/candidates.rs +++ b/crates/symbolicator-service/src/objects/candidates.rs @@ -222,25 +222,36 @@ impl AllObjectCandidates { /// `other` if they are not [`ObjectUseInfo::None`]. pub fn merge(&mut self, other: &AllObjectCandidates) { for other_info in &other.0 { - let key = (&other_info.source, &other_info.location); - let found_pos = self - .0 - .binary_search_by_key(&key, |candidate| (&candidate.source, &candidate.location)); - match found_pos { - Ok(index) => { - if let Some(info) = self.0.get_mut(index) { - info.download = other_info.download.clone(); - if other_info.unwind != ObjectUseInfo::None { - info.unwind = other_info.unwind.clone(); - } - if other_info.debug != ObjectUseInfo::None { - info.debug = other_info.debug.clone(); - } + self.merge_one(other_info); + } + } + + /// Merges another candidate into this collection. + /// + /// If `candidate` already existed in the collection all data which is present in both + /// will be overwritten by the data in `candidate`. Practically that means + /// [`ObjectCandidate::download`] will be overwritten by `candidate` and for + /// [`ObjectCandidate::unwind`] and [`ObjectCandidate::debug`] it will be overwritten by + /// `candidate` if they are not [`ObjectUseInfo::None`]. + pub fn merge_one(&mut self, candidate: &ObjectCandidate) { + let key = (&candidate.source, &candidate.location); + let found_pos = self + .0 + .binary_search_by_key(&key, |candidate| (&candidate.source, &candidate.location)); + match found_pos { + Ok(index) => { + if let Some(info) = self.0.get_mut(index) { + info.download = candidate.download.clone(); + if candidate.unwind != ObjectUseInfo::None { + info.unwind = candidate.unwind.clone(); + } + if candidate.debug != ObjectUseInfo::None { + info.debug = candidate.debug.clone(); } } - Err(index) => { - self.0.insert(index, other_info.clone()); - } + } + Err(index) => { + self.0.insert(index, candidate.clone()); } } } diff --git a/crates/symbolicator-sources/src/sources/http.rs b/crates/symbolicator-sources/src/sources/http.rs index 206dff3a2..f347509a7 100644 --- a/crates/symbolicator-sources/src/sources/http.rs +++ b/crates/symbolicator-sources/src/sources/http.rs @@ -72,7 +72,8 @@ impl HttpRemoteFile { HttpRemoteFile::new(source, location) } - pub(crate) fn uri(&self) -> RemoteFileUri { + /// Returns a [`RemoteFileUri`] for the file. + pub fn uri(&self) -> RemoteFileUri { match self.url() { Ok(url) => url.as_ref().into(), Err(_) => "".into(),