feat: source context links support

crates/symbolicator-service/src/services/module_lookup.rs

@@ -105,7 +105,15 @@ struct ModuleEntry {
 pub struct ModuleLookup {
     modules: Vec<ModuleEntry>,
     scope: Scope,
+    // TODO rename variable and type so it wouldn't get confused with source code. Maybe "Provider"?
     sources: Arc<[SourceConfig]>,
+    // TODO this replace with a cache & download handler, like SourceMapCache.
+    source_links_cache: HashMap<url::Url, String>,
+}
+
+pub(crate) enum SetContextLinesResult {
+    Ok,
+    SourceMissing(url::Url),
 }
 
 impl ModuleLookup {
@@ -154,6 +162,7 @@ impl ModuleLookup {
             modules,
             scope,
             sources,
+            source_links_cache: HashMap::new(),
         }
     }
 
@@ -343,7 +352,22 @@ impl ModuleLookup {
         }
     }
 
-    /// Look up the corresponding SymCache based on the instruction `addr`.
+    /// Fetches all remote source URLs.
+    #[tracing::instrument(skip_all)]
+    pub async fn fetch_source_links(&mut self, links: HashSet<url::Url>) {
+        // TODO implement properly
+        let client = reqwest::Client::new();
+        for url in links {
+            let builder = client.get(url.clone());
+            if let Ok(response) = builder.send().await {
+                if let Ok(text) = response.text().await {
+                    self.source_links_cache.insert(url, text);
+                }
+            }
+        }
+    }
+
+    /// Look up the corresponding SymCache based on the instruxction `addr`.
     pub fn lookup_cache(&self, addr: u64, addr_mode: AddrMode) -> Option<CacheLookupResult<'_>> {
         self.get_module_by_addr(addr, addr_mode).map(|entry| {
             let relative_addr = match addr_mode {
@@ -381,22 +405,59 @@ impl ModuleLookup {
             .collect()
     }
 
-    /// This looks up the source of the given line, plus `n` lines above/below.
-    pub fn get_context_lines(
+    /// Update the frame with source context, if available. In case the source code references an
+    /// URL that's not yet loaded, returns `SetContextLinesResult::SourceMissing(url)`.
+    pub(crate) fn set_context_lines(
         &self,
         debug_sessions: &HashMap<usize, Option<ObjectDebugSession<'_>>>,
-        frame: &RawFrame,
+        frame: &mut RawFrame,
         num_lines: usize,
-    ) -> Option<(Vec<String>, String, Vec<String>)> {
+    ) -> Option<SetContextLinesResult> {
         let abs_path = frame.abs_path.as_ref()?;
-        let lineno = frame.lineno? as usize;
+
+        // short-circuit here if we don't have the mandatory line number - no reason to load the source.
+        frame.lineno?;
 
         let entry = self.get_module_by_addr(frame.instruction_addr.0, frame.addr_mode)?;
         let session = debug_sessions.get(&entry.module_index)?.as_ref()?;
-        let source_descriptor = session.source_by_path(abs_path).ok()??;
-        // TODO: add support for source links via `source_descriptor.url()`
-        let source = source_descriptor.contents()?;
 
+        let source = session.source_by_path(abs_path).ok()??;
+
+        if let Some(text) = source.contents() {
+            return Self::set_context_lines_from_source(text, frame, num_lines);
+        }
+
+        if let Some(url) = source.url() {
+            // Only allow http:// and https:// URLs to prevent file-system reads.
+            // TODO maybe we want even stricter rules, e.g. only fetch from github/gitlab?
+            if url.starts_with("https://") || url.starts_with("http://") {
+                let url = url::Url::parse(url).ok()?;
+                return self.set_context_lines_from_url(url, frame, num_lines);
+            }
+        }
+
+        None
+    }
+
+    /// Update the frame with source context for the given URL.
+    pub(crate) fn set_context_lines_from_url(
+        &self,
+        url: url::Url,
+        frame: &mut RawFrame,
+        num_lines: usize,
+    ) -> Option<SetContextLinesResult> {
+        match self.source_links_cache.get(&url) {
+            Some(text) => Self::set_context_lines_from_source(text, frame, num_lines),
+            None => Some(SetContextLinesResult::SourceMissing(url)),
+        }
+    }
+
+    fn set_context_lines_from_source(
+        source: &str,
+        frame: &mut RawFrame,
+        num_lines: usize,
+    ) -> Option<SetContextLinesResult> {
+        let lineno = frame.lineno? as usize;
         let start_line = lineno.saturating_sub(num_lines);
         let line_diff = lineno - start_line;
 
@@ -405,10 +466,15 @@ impl ModuleLookup {
             .take(line_diff.saturating_sub(1))
             .map(|x| x.to_string())
             .collect();
-        let context = lines.next()?.to_string();
+        let context_line = lines.next()?.to_string();
         let post_context = lines.take(num_lines).map(|x| x.to_string()).collect();
 
-        Some((pre_context, context, post_context))
+        // Only set after we've collected all three, so we don't set the context partially
+        frame.pre_context = pre_context;
+        frame.context_line = Some(context_line);
+        frame.post_context = post_context;
+
+        Some(SetContextLinesResult::Ok)
     }
 
     /// Looks up the [`ModuleEntry`] for the given `addr` and `addr_mode`.

crates/symbolicator-service/src/services/symbolication/mod.rs

@@ -1,3 +1,4 @@
+use std::collections::HashSet;
 use std::sync::Arc;
 
 use symbolic::common::{split_path, DebugId, InstructionInfo, Language, Name};
@@ -9,7 +10,9 @@ use symbolicator_sources::{ObjectType, SourceConfig};
 
 use crate::caching::{Cache, CacheError};
 use crate::services::cficaches::CfiCacheActor;
-use crate::services::module_lookup::{CacheFileEntry, CacheLookupResult, ModuleLookup};
+use crate::services::module_lookup::{
+    CacheFileEntry, CacheLookupResult, ModuleLookup, SetContextLinesResult,
+};
 use crate::services::objects::ObjectsActor;
 use crate::services::ppdb_caches::PortablePdbCacheActor;
 use crate::services::sourcemap::SourceMapService;
@@ -138,20 +141,31 @@ impl SymbolicationActor {
 
         let debug_sessions = module_lookup.prepare_debug_sessions();
 
+        // Map collected source contexts to frames.
+        const NUM_LINES: usize = 5;
+        let mut remote_source_frames = Vec::new();
         for trace in &mut stacktraces {
             for frame in &mut trace.frames {
-                if let Some((pre_context, context_line, post_context)) =
-                    module_lookup.get_context_lines(&debug_sessions, &frame.raw, 5)
+                if let Some(SetContextLinesResult::SourceMissing(url)) =
+                    module_lookup.set_context_lines(&debug_sessions, &mut frame.raw, NUM_LINES)
                 {
-                    frame.raw.pre_context = pre_context;
-                    frame.raw.context_line = Some(context_line);
-                    frame.raw.post_context = post_context;
+                    remote_source_frames.push((&mut frame.raw, url));
                 }
             }
         }
+
         // explicitly drop this, so it does not borrow `module_lookup` anymore.
         drop(debug_sessions);
 
+        if !remote_source_frames.is_empty() {
+            let urls = HashSet::from_iter(remote_source_frames.iter().map(|v| v.1.clone()));
+            module_lookup.fetch_source_links(urls).await;
+
+            for pair in remote_source_frames {
+                module_lookup.set_context_lines_from_url(pair.1, pair.0, NUM_LINES);
+            }
+        }
+
         // bring modules back into the original order
         let modules = module_lookup.into_inner();
         record_symbolication_metrics(origin, metrics, &modules, &stacktraces);

crates/symbolicator-service/tests/integration/snapshots/integration__public_sources__nuget_source.snap

@@ -1,6 +1,6 @@
 ---
 source: crates/symbolicator-service/tests/integration/public_sources.rs
-assertion_line: 45
+assertion_line: 48
 expression: response.unwrap()
 ---
 stacktraces:
@@ -15,6 +15,18 @@ stacktraces:
         filename: TZConvert.cs
         abs_path: /_/src/TimeZoneConverter/TZConvert.cs
         lineno: 115
+        pre_context:
+          - "        {"
+          - "            return windowsTimeZoneId;"
+          - "        }"
+          - ""
+        context_line: "        throw new InvalidTimeZoneException("
+        post_context:
+          - "            $\"\\\"{ianaTimeZoneName}\\\" was not recognized as a valid IANA time zone name, or has no equivalent Windows time zone.\");"
+          - "    }"
+          - ""
+          - "    /// <summary>"
+          - "    /// Attempts to convert an IANA time zone name to the equivalent Windows time zone ID."
 modules:
   - debug_status: found
     features:

crates/symbolicator-service/tests/integration/snapshots/integration__symbolication__dotnet_integration.snap

@@ -1,6 +1,5 @@
 ---
 source: crates/symbolicator-service/tests/integration/symbolication.rs
-assertion_line: 257
 expression: response.unwrap()
 ---
 stacktraces:

crates/symbolicator-service/tests/integration/snapshots/integration__symbolication__dotnet_source_links.snap

@@ -0,0 +1,61 @@
+---
+source: crates/symbolicator-service/tests/integration/symbolication.rs
+assertion_line: 205
+expression: response.unwrap()
+---
+stacktraces:
+  - frames:
+      - status: symbolicated
+        original_index: 0
+        addr_mode: "rel:0"
+        instruction_addr: "0x1"
+        function_id: "0x7"
+        lang: csharp
+        filename: ThrowHelper.cs
+        abs_path: /_/src/libraries/Common/src/System/ThrowHelper.cs
+        lineno: 24
+        pre_context:
+          - "#endif"
+          - "            object? argument,"
+          - "            [CallerArgumentExpression(\"argument\")] string? paramName = null)"
+          - "        {"
+        context_line: "            if (argument is null)"
+        post_context:
+          - "            {"
+          - "                Throw(paramName);"
+          - "            }"
+          - "        }"
+          - ""
+modules:
+  - debug_status: found
+    features:
+      has_debug_info: true
+      has_unwind_info: false
+      has_symbols: false
+      has_sources: true
+    arch: unknown
+    type: pe_dotnet
+    debug_id: 37e9e8a6-1a8e-404e-b93c-6902e277ff55-a09672e1
+    debug_file: source-links.pdb
+    image_addr: "0x0"
+    candidates:
+      - source: local
+        location: "http://localhost:<port>/symbols/source-links.pdb/37E9E8A61A8E404EB93C6902E277FF55a09672e1/source-links.src.zip"
+        download:
+          status: notfound
+      - source: local
+        location: "http://localhost:<port>/symbols/source-links.pdb/37E9E8A61A8E404EB93C6902E277FF55ffffffff/source-links.pdb"
+        download:
+          status: ok
+          features:
+            has_debug_info: true
+            has_unwind_info: false
+            has_symbols: false
+            has_sources: true
+        debug:
+          status: ok
+      - source: local
+        location: "http://localhost:<port>/symbols/source-links.pdb/37E9E8A61A8E404EB93C6902E277FF55ffffffff/source-links.src.zip"
+        download:
+          status: notfound
+

crates/symbolicator-service/tests/integration/symbolication.rs

@@ -179,3 +179,28 @@ async fn test_dotnet_embedded_sources() {
 
     assert_snapshot!(response.unwrap());
 }
+
+#[tokio::test]
+async fn test_dotnet_source_links() {
+    let (symbolication, _cache_dir) = setup_service(|_| ());
+    let (_srv, source) = symbol_server();
+
+    let request = make_symbolication_request(
+        vec![source],
+        r#"[{
+          "type":"pe_dotnet",
+          "debug_file":"source-links.pdb",
+          "debug_id":"37e9e8a6-1a8e-404e-b93c-6902e277ff55-a09672e1"
+        }]"#,
+        r#"[{
+          "frames":[{
+            "instruction_addr": 1,
+            "function_id": 7,
+            "addr_mode":"rel:0"
+          }]
+        }]"#,
+    );
+    let response = symbolication.symbolicate(request).await;
+
+    assert_snapshot!(response.unwrap());
+}